pymc-devs / pymc-devs/pytensor
MetaType `__props__` based equality overrides class custom implementations
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 644
- Forks
- 208
- Avg merge
- 2d 14h
- Merged PRs (30d)
- 16
Description
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 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
- Remove the
output_types_preference-basedScalarOp__hash__/__eq__(and revisit the wholeoutput_types_preferencemechanism — it's a hack). MakeScalarOpidentity props-based. - Make
MetaTypedefer 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__; dropBaseSubtensor'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.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start with MetaType in graph/utils.py, then inspect ScalarOp, ScalarInnerGraphOp/Composite, SymbolicOp.init_subclass, IfElse, and BaseSubtensor. Trace how props, hash, and eq interact before changing them. Done means the proposed redundant implementations and workaround are removed without merging distinct Composite identities.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend
- Issue type
- Refactor
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 35/100