Skip to content

MetaType __props__ based equality overrides class custom implementations #2185

@ricardoV94

Description

@ricardoV94

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.

Metadata

Metadata

Assignees

No one assigned

    Type

    No type
    No fields configured for issues without a type.

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions