Add collection constructor operations and rewrite analyses to remove side effects - #719
Conversation
eb8680
left a comment
There was a problem hiding this comment.
fvsof's behavior changes for terms that contain collections: the collection construction operations now appear in the term free variables.
This seems like a big breaking change, are we sure it's necessary? I would expect lots of places here and downstream to depend on tests like if not fvsof(x): ... to gate partial evaluation behavior.
|
No, it's not, but we should think about what semantics we want for |
|
This PR now doesn't change the behavior of |
eb8680
left a comment
There was a problem hiding this comment.
How does this affect extensibility? With evaluate, defdata, nested_type etc the story is clear - a downstream user can register type-specific implementations that make them compatible with new third-party types. We don't want this new mechanism to break that and require edits to effectful to support new types.
I'd also worry that this is proliferating rather than consolidating traversal logic (#682 ) - isn't this morally duplicating evaluate's type-unpacking behavior? How do we avoid that, or at least enforce consistency across traversals?
| yield intp | ||
|
|
||
|
|
||
| class DataclassConstrOperation(Operation): ... |
There was a problem hiding this comment.
Should share a common base with CollectionConstrOperation
|
|
||
|
|
||
| @CollectionConstrOperation.define | ||
| def as_tuple(*args) -> tuple: |
There was a problem hiding this comment.
This type rule is wrong/too weak, I would expect this to break typeof for tuples
There was a problem hiding this comment.
typeof runs CollectionConstrOperations rather than handling them. This allows us to keep the smarter type checking behavior from nested_type. I don't think we have rich enough types at the moment to type check the tuple constructor.
| pass | ||
|
|
||
|
|
||
| class CollectionConstrOperation(Operation): ... |
There was a problem hiding this comment.
I think this needs at least one type parameter for the return type
|
|
||
|
|
||
| @functools.cache | ||
| def _as_type(typ: type, operation_type=CollectionConstrOperation): |
There was a problem hiding this comment.
I wonder if this should be a public classmethod of one of the new Operation classes. It seems like the right way to access these operations is to look them up by return type rather than by name, and to have an extension API that allows someone to register new ones as with to the singledispatch-based extensibility of evaluate and defdata.
|
|
||
|
|
||
| @CollectionConstrOperation.define | ||
| def as_list[T](*args: T) -> list[T]: |
There was a problem hiding this comment.
As I wrote in another comment, I think we probably want to look these up by type (in a singledispatch registry that is subtyping-aware) rather than by name to preserve our extensibility story
There was a problem hiding this comment.
These operations aren't the same as their corresponding constructors (e.g. tuple takes a single sequence argument, not a sequence of arguments). I think we only need a single sequence constructor though (probably as_tuple).
|
Right now, This PR doesn't change the status quo around Re, traversal logic, I think this is a simplification because more analyses can become pure handlers of |
Closes #715, #624
I did not pick up the changes from #680, although they're of a similar flavor.
Rewrites
fvsofandsizesofin the jax module to remove their side-effects. This makes them compatible with memoization from #716.fvsof's behavior changes for terms that contain collections: the collection construction operations now appear in the term free variables.