Description
Beep beep, I'm bot generated and verbose
Summary
An Op's identity (__hash__/__eq__) should come from a single source. Today a few Ops define both __props__ and a bespoke __hash__/__eq__, which is the same thing done two ways. The main offender is ScalarOp's output_types_preference-based identity, which blocks a cleaner MetaType and forces a workaround in SymbolicOp.
Background
MetaType (graph/utils.py) auto-generates props-based __hash__/__eq__ for any Op declaring __props__, even when a base already provides a custom implementation. OpFromGraph/SymbolicOp need an fgraph-aware identity, so SymbolicOp.__init_subclass__ re-assigns __hash__/__eq__ after class creation to defeat MetaType — a hook we'd like to remove.
The clean design is: MetaType defers to an inherited custom (non-autogenerated) __hash__/__eq__ instead of shadowing it with a props-based one. Then OpFromGraph/SymbolicOp identity is inherited naturally and the hook disappears.
That change is currently unsafe because of one inconsistency:
ScalarOp.__hash__/__eq__ key on output_types_preference, not __props__ — a hack. ScalarInnerGraphOp/Composite (__props__ = ("fgraph",)) would inherit that under MetaType-defer and lose the inner-graph distinction, merging distinct Composites. (Composite itself is fine: its fgraph is a FrozenFunctionGraph, so the props-based hash is hashable and sufficient — it's the victim, not the cause.)
There's a bit of subclass dependency hell, we want one method for one set of ops but not the other. Ideally we clean this output_types_preference altogether away and solve it, but if it can't we have to split the dependency
Two minor, independent redundancies:
IfElse defines a __hash__ byte-identical to the props-based one → redundant.
BaseSubtensor uses a props-based hash plus a slice-hashability shim for Python < 3.12 (redundant on 3.12+, where slices are hashable).
Proposed cleanup
- Remove the
output_types_preference-based ScalarOp __hash__/__eq__ (and revisit the whole output_types_preference mechanism — it's a hack). Make ScalarOp identity props-based.
- Make
MetaType defer to an inherited custom __hash__/__eq__ (don't shadow with the props-based default when a base provides an explicit, non-autogenerated one).
- Remove
SymbolicOp.__init_subclass__'s __hash__/__eq__ re-assignment.
- Tidy-ups: delete
IfElse's redundant __hash__/__eq__; drop BaseSubtensor's slice shim once Python < 3.12 is no longer supported.
Composite/ScalarInnerGraphOp needs no change.
Context
Surfaced while converting MvNormalRV to a SymbolicRVOp (symbolic random variables built on SymbolicOp), where the SymbolicOp.__init_subclass__ hash hook was needed purely to work around MetaType shadowing the fgraph-aware identity.
Description
Beep beep, I'm bot generated and verbose
Summary
An Op's identity (
__hash__/__eq__) should come from a single source. Today a few Ops define both__props__and a bespoke__hash__/__eq__, which is the same thing done two ways. The main offender isScalarOp'soutput_types_preference-based identity, which blocks a cleanerMetaTypeand forces a workaround inSymbolicOp.Background
MetaType(graph/utils.py) auto-generates props-based__hash__/__eq__for any Op declaring__props__, even when a base already provides a custom implementation.OpFromGraph/SymbolicOpneed an fgraph-aware identity, soSymbolicOp.__init_subclass__re-assigns__hash__/__eq__after class creation to defeat MetaType — a hook we'd like to remove.The clean design is:
MetaTypedefers to an inherited custom (non-autogenerated)__hash__/__eq__instead of shadowing it with a props-based one. ThenOpFromGraph/SymbolicOpidentity is inherited naturally and the hook disappears.That change is currently unsafe because of one inconsistency:
ScalarOp.__hash__/__eq__key onoutput_types_preference, not__props__— a hack.ScalarInnerGraphOp/Composite(__props__ = ("fgraph",)) would inherit that under MetaType-defer and lose the inner-graph distinction, merging distinct Composites. (Compositeitself is fine: itsfgraphis aFrozenFunctionGraph, so the props-based hash is hashable and sufficient — it's the victim, not the cause.)There's a bit of subclass dependency hell, we want one method for one set of ops but not the other. Ideally we clean this output_types_preference altogether away and solve it, but if it can't we have to split the dependency
Two minor, independent redundancies:
IfElsedefines a__hash__byte-identical to the props-based one → redundant.BaseSubtensoruses a props-based hash plus a slice-hashability shim for Python < 3.12 (redundant on 3.12+, where slices are hashable).Proposed cleanup
output_types_preference-basedScalarOp__hash__/__eq__(and revisit the wholeoutput_types_preferencemechanism — it's a hack). MakeScalarOpidentity props-based.MetaTypedefer to an inherited custom__hash__/__eq__(don't shadow with the props-based default when a base provides an explicit, non-autogenerated one).SymbolicOp.__init_subclass__'s__hash__/__eq__re-assignment.IfElse's redundant__hash__/__eq__; dropBaseSubtensor's slice shim once Python < 3.12 is no longer supported.Composite/ScalarInnerGraphOpneeds no change.Context
Surfaced while converting
MvNormalRVto aSymbolicRVOp(symbolic random variables built onSymbolicOp), where theSymbolicOp.__init_subclass__hash hook was needed purely to work around MetaType shadowing the fgraph-aware identity.