-
Notifications
You must be signed in to change notification settings - Fork 13
feat(py): ComposablePass protocol and ComposedPass for hugr-py
#2636
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
hugr-py/src/hugr/hugr/pass.py
Outdated
| @classmethod | ||
| def from_dict(cls, dictionary: dict) -> Self: ... | ||
|
|
||
| def to_dict(self) -> dict: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't support encoding/decoding every pass, as some may have non encodable context.
Should we do a best effort to_dict encoding, and signal when the result is not complete? (so handlers downstream can get some partial info about it), or outright error-out if not supported?
For the from_dict implementation, we'll need some kind of pass registry that knows a set of passes it can decode. We can think about that later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would be an example of a pass with a non-encodable context? A user defined pass where the user passes a function into the builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do you think I should remove the to/from_dict for now? Not sure pass serialisation belongs in the initial work for Guppy optimisation.
We should add it at some point for nexus integration though I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think it needs more design discussion.
I'm also guessing that we'd want to return a dataclass rather than an arbitrary dict, to keep some structure...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good point. I'll remove it for now.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2636 +/- ##
==========================================
- Coverage 83.46% 83.38% -0.08%
==========================================
Files 262 261 -1
Lines 51297 50562 -735
Branches 46858 46033 -825
==========================================
- Hits 42813 42160 -653
+ Misses 6105 6055 -50
+ Partials 2379 2347 -32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
After chatting to Alec it may be worth removing the |
Makes sense. Passes are more generic here than just rewrites over specific sets of gates, so fixing a closed set of optypes here seems counter-productive. |
|
Using |
ComposablePass protocol for hugr-pyComposablePass protocol for hugr-py
hugr-py/src/hugr/hugr/__init__.py
Outdated
| """The main HUGR structure.""" | ||
|
|
||
| from .base import Hugr, NodeData | ||
| from .composable_pass import ComposablePass |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Open to better suggestions for this module name. I opted to not use pass.py as pass is a keyword in Python.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Though maybe I'd move it out of hugr.hugr, and into its own thing?
hugr.passes.ComposablePass?
|
C.I. failure to do with coverage checks or something? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C.I. failure to do with coverage checks or something?
Yeah, it's complaining that there are no tests here.
You can see the codecov report linked from the message.
We could add a test with a dummy no-op pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 01b7126
Some maybe the isinstance checks are not needed. I also made ComposablePass runtime checkable.
Co-authored-by: Agustín Borgna <[email protected]>
aborgna-q
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| def then(self, other: ComposablePass) -> ComposablePass: | ||
| return ComposedPass([self, other]) | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let it use the default implementation (we want to test that rather than test code)
| def then(self, other: ComposablePass) -> ComposablePass: | |
| return ComposedPass([self, other]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This suggestion wasn't implemented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤖 I have created a release *beep* *boop* --- ## [0.14.2](hugr-py-v0.14.1...hugr-py-v0.14.2) (2025-11-13) ### Features * **cli, python:** programmatic interface to cli with python bindings ([#2677](#2677)) ([0fd0332](0fd0332)) * ComposablePass protocol and ComposedPass for hugr-py (unstable) ([[#2636](https://github.com/CQCL/hugr/issues/2636)](https://github.com/CQCL/hugr/pull/2636)) ([45dc3fc](45dc3fc)) * return description output to python on error ([#2681](#2681)) ([f483146](f483146)) * track package descriptions when loading ([#2639](#2639)) ([349dd61](349dd61)) ### Documentation * Fix typo in docstring. ([#2656](#2656)) ([a1ce622](a1ce622)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: Seyon Sivarajah <[email protected]>
Suggestion missing from #2636, makes sure we test the actual implementation rather than the dummy one. drive-by: Improve `name` impl for ComposedPass
Suggestion missing from #2636, makes sure we test the actual implementation rather than the dummy one. drive-by: Improve `name` impl for ComposedPass
closes #2617
Mostly follows Agustin's suggestion see eeb4c50
I'm also added some
to/from_dictmethods as well as asupported_opsproperty. EDIT: removed these additional methods for now.BEGIN_COMMIT_OVERRIDE
feat: ComposablePass protocol and ComposedPass for hugr-py (unstable) (#2636)
END_COMMIT_OVERRIDE