Skip to content

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Base core graph classes on attrs or dataclasses #72

Closed
brandonwillard opened this issue Oct 4, 2020 · 4 comments
Closed

Base core graph classes on attrs or dataclasses #72

brandonwillard opened this issue Oct 4, 2020 · 4 comments
Labels
enhancement New feature or request

Comments

@brandonwillard
Copy link
Member

In line with #37, we should consider basing the core graph objects (e.g. theano.gof.graph.[Node, Variable, Apply], theano.gof.op.[PureOp, Op], and all their subclasses) on some form of flexible, (relatively) immutable, easily convertible data type. The packages attrs and the newly built-in dataclasses provide much—if not all—of what we need.

Specifically, both packages provide automatic recursive conversion into tuples and dicts and easily configurable defaults for things like equality, ordering, repr, hashing, etc. The attrs goes a little further and can perform automatic input validation and conversion, as well as use __slots__ for a relative performance gain.

More importantly, both provide convenient mutation functions (i.e. dataclasses.replace and attrs.evolve) that fit well with the existing object identity model and the pervasive requirement for object cloning. In this setting, graph object mutation could be turned into a considerably easier—and universal—process!

@majidaldo
Copy link

Q: is it possible to keep treating these classes like data ('fix' the attribs?)? Over time, classes tend to acquire attribute-like methods.
Some of your 'pythological' work would also be interesting here.

@majidaldo
Copy link

Thought: ImO, the core graph objects would have to be pretty abstract... no theano-specifics like is_somethingrelatedtoexecution().

@majidaldo
Copy link

#37 is related right? attrs has built in mgt for eq.

@brandonwillard
Copy link
Member Author

#37 is related right? attrs has built in mgt for eq.

Yes, it could be related, but it's likely that we'll need/want custom __eq__-based handling for these objects, because it's very easy to induce costly graph comparisons and RecursionErrors with a standard __eq__ implementation.

Thought: ImO, the core graph objects would have to be pretty abstract... no theano-specifics like is_somethingrelatedtoexecution().

It looks like you might be referring to a requirement that we need to implement: i.e. immutability of the core graph classes. That is a motivating factor behind the interest in attrs and dataclasses.

We can impose some immutability constraints using __slots__, and gain some performance benefits, as well. Unfortunately, __slots__ will break a lot of existing (and undesirable) implementations in the codebase, but that's the core work involved in this issue.

Symbolic PyMC uses __slots__ consistently within its MetaSymbolType types/classes, but it imposes some design difficulties and demands a lot of consistency from implementations. More specifically, inheritance is complicated by __slots__, and some of that is handled by the class construction code and the conventions imposed by MetaSymbolType.

If you look at MetaType in Theano-PyMC, you'll notice that it's similar to MetaSymbolType, but it uses a __props__ member instead of __slots__. Even so, __props__ isn't used consistently—or at all—by many Ops, so we can't get much out of that.

Both of those metaclasses do things similar to what attrs and dataclasses do, so it would be nice to use those packages instead of our own handwritten metaclass logic.

@brandonwillard brandonwillard removed this from the 2.0 milestone May 19, 2021
@aesara-devs aesara-devs locked and limited conversation to collaborators May 19, 2021

This issue was moved to a discussion.

You can continue the conversation there. Go to discussion →

Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants