-
Notifications
You must be signed in to change notification settings - Fork 2
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
refactor: port HUGR codegen to use HUGR python builder #257
Comments
I'm planning to do this in multiple steps, starting from nodes and ports before removing the whole Node and port subclassesGuppy defines multiple subclasses of its abstract graph TB
style Port stroke-dasharray: 5 5
Port --> InPort
Port --> OutPort
InPort --> InPortV
OutPort --> OutPortV
InPort --> InPortCF
OutPort --> OutPortCF
graph TB
style Node stroke-dasharray: 5 5
Node --> VNode
Node --> CFNode
style DFContainingNode stroke-dasharray: 5 5
Node --> DFContainingNode
DFContainingNode --> DFContainingVNode
VNode --> DFContainingVNode
DFContainingNode --> BlockNode
CFNode --> BlockNode
The the value
Fat nodes and portsGuppy's This is somewhat similar to
Proposed changes to guppy / hugr-pyI'd like to propose the following:
The hugr classes on both sides are fairly similar, so hopefully we shouldn't need to change |
Thank you, that's a great overview of the existing setup in Guppy! I think your migration plan makes sense 👍
Do you mean
👍
This would be great, but just do confirm: Do these
I don't have a strong opinion on this, I'm fine with whatever is easier for you. Note there might be some rough edges around types since the current ports use Guppy types instead of Hugr types. I think the transition shouldn't be too bad, but there might be some issues with type args, structs etc... |
D'oh!
Right. If we keep the slim ports then we can keep the conversion between types in guppy's |
Can they just stay in Guppy and contain a hugr port? Not including them in hugr-py was a conscious simplicity choice in the user-facing API
Yes |
Those would be closely linked with the |
This is an in-progress transplant of guppy from using an ad-hoc `Hugr` definition into the builder from the `hugr` library. The main mismatch between the old and new builders is that guppy used to lazily add typed ports to the graph nodes as it needed. Since the new builder requires operations to define their signature, we end up having to edit the code everywhere from `guppylang.compiler` to the `guppylang.prelude` definitions. My first goal is to get this working, and then I will try and split some changes from this PR (although most of it will have to be merged monolithically). Current test status: - 390 $\color{green}\text{passed}$ - 14 $\color{yellow}\text{skipped}$ - 0 $\color{red}\text{failed}$ These issues currently breake some tests. I'll fix them before merging the PR - CQCL/hugr#1319 ~~Required to store the module names in the hugr metadata~~ - CQCL/hugr#1424 ~~Required for pytket and llvm integration~~ - ~~Release hugr and tket2 with the latest changes~~ Closes #257. Closes #85 BREAKING CHANGE: Removed `guppylang.hugr_builder.hugr.Hugr`, compiling a module returns a `hugr.Hugr` instead. --------- Co-authored-by: Mark Koch <[email protected]>
Blocked by non-optional tasks in tracking issue: CQCL/hugr#486Now blocked by #280 to avoid bad merge conflicts. (Base on working branch for that issue if starting before that is merged)The text was updated successfully, but these errors were encountered: