pymc-devs / pymc-devs/pytensor

MetaType `__props__` based equality overrides class custom implementations

Open
#2,185 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Op implementation
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 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

  1. 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.
  2. 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).
  3. Remove SymbolicOp.__init_subclass__'s __hash__/__eq__ re-assignment.
  4. 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.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.