pytensor icon indicating copy to clipboard operation
pytensor copied to clipboard

Make props related functionality a part of the Op baseclass

Open ricardoV94 opened this issue 6 months ago • 2 comments

Description

This is in MetaType, but AFAICT only ops ever have props

https://github.com/pymc-devs/pytensor/blob/17748b7d3419e78e7dfad3dc768908ac240aa6df/pytensor/graph/utils.py#L193-L234

Generating them dynamically like this means linters / mypy don't believe in op._props() or op._props_dict(). I don't know why this has to be implemented as methods in the MetaClass instead of just in the Op baseclass. Would be great if someone gave a try at it. It also means we could probably get rid of the whole MetaType class, since the only other thing is a string convenience, which definitely need not be defined there.

May also be worth investigating if we can use frozen dataclasses for Ops and automate the props logic (see aesara discussion, but that needs not be done together with addressing the main issue here.

ricardoV94 avatar Jun 10 '25 08:06 ricardoV94

@ricardoV94 I am giving it a go in this PR inside my fork [so I don't make noise around here].

Some learnings:

  • Type also inherits from MetaType through MetaObject. This class acts as super of a bunch of other types.
    • We would then need to add props also in this class, not only in Op
  • Some tests [e.g. CustomOpNoProps] would need to be hacked to retain previous behavior. This is a bit convoluted, because all Ops should have __props__.
  • We would have to patch some MyOp definitions inside tests like this one to explicitly include __props__

PabloRoque avatar Jul 16 '25 09:07 PabloRoque

I don't think we need props in those other classes, let me know if they are actually used anywhere.

Can __props__ be None by default and make do work as before? If None we hash and match on identity as default Python does.

ricardoV94 avatar Jul 16 '25 10:07 ricardoV94