Skip to content
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

Closed
ss2165 opened this issue Jun 21, 2024 · 5 comments · Fixed by #366
Closed

refactor: port HUGR codegen to use HUGR python builder #257

ss2165 opened this issue Jun 21, 2024 · 5 comments · Fixed by #366
Assignees

Comments

@ss2165
Copy link
Member

ss2165 commented Jun 21, 2024

Blocked by non-optional tasks in tracking issue: CQCL/hugr#486

Now blocked by #280 to avoid bad merge conflicts. (Base on working branch for that issue if starting before that is merged)

@aborgna-q
Copy link
Collaborator

aborgna-q commented Jul 24, 2024

I'm planning to do this in multiple steps, starting from nodes and ports before removing the whole hugr_builder.hugr.Hugr class.
There are some incompatibilities that we have to address:

Node and port subclasses

Guppy defines multiple subclasses of its abstract Node and Port classes to provide some type safety and add some handy class methods when possible. (Dashed boxes are abstract classes).

graph TB
  style Port stroke-dasharray: 5 5
  Port --> InPort
  Port --> OutPort
  InPort --> InPortV
  OutPort --> OutPortV
  InPort --> InPortCF
  OutPort --> OutPortCF
Loading
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
Loading

The the value *PortV variants include their type and and a non-null offset.
In the case of Nodes, each subclass adds a myriad of extra runtime checks to each method call, and type specialisations when dealing with ports.

hugr-py has no such class hierarchy, aside from OutPort and InPort.
ParentBuilder could maybe be viewed as a DFContainingNode equivalent, as it implements ToNode and has similar uses.

Fat nodes and ports

Guppy's Nodes and value Ports store their OpType and Type (respectively).
This is used throughout guppy to both validate that we're doing the correct thing as well as to easily access the values without explicitly passing the hugr around.

This is somewhat similar to hugr-llvm's FatNodes, but here I believe we can always access the base hugr so it's not that necessary to keep the data around.

Nodes can also be mutated, adding/removing ports to it and updating their operation based on the new ports. I believe this can be delegated to the hugr-py builder's current implementation, so we won't need this feature.

Proposed changes to guppy / hugr-py

I'd like to propose the following:

  • Bring in the value/cfg port and node variants (with better names) to hugr-py, but drop the specialised parent-node classes as these are already covered by ParentBuilder's subclasses.
  • Drop the fat nodes and ports in guppy. We should be able to modify the call sites to query the hugr instead.
    • We could go either way here. I don't think tracking the types would change the code too much here.

The hugr classes on both sides are fairly similar, so hopefully we shouldn't need to change hugr-py much more after this.

@mark-koch
Copy link
Collaborator

Thank you, that's a great overview of the existing setup in Guppy! I think your migration plan makes sense 👍

Proposed changes to guppy / hugr-cli

Do you mean hugr-py instead of hugr-cli?

Bring in the value/cfg port and node variants (with better names) to hugr-cli

👍

but drop the specialised parent-node classes as these are already covered by ParentBuilder's subclasses.

This would be great, but just do confirm: Do these ParentBuilder subclasses support non-local edges? @ss2165

Drop the fat nodes and ports in guppy. We should be able to modify the call sites to query the hugr instead. We could go either way here. I don't think tracking the types would change the code too much here.

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...

@aborgna-q
Copy link
Collaborator

aborgna-q commented Jul 24, 2024

Do you mean hugr-py instead of hugr-cli?

D'oh!

Note there might be some rough edges around types since the current ports use Guppy types instead of Hugr types.

Right. If we keep the slim ports then we can keep the conversion between types in guppy's Hugr for now.
I will address the type definitions next.

@ss2165
Copy link
Member Author

ss2165 commented Jul 24, 2024

Bring in the value/cfg port and node variants (with better names) to hugr-py

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

Do these ParentBuilder subclasses support non-local edges? @ss2165

Yes

@aborgna-q
Copy link
Collaborator

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

Those would be closely linked with the ParentBuilder variants. If DFG.add_node doesn't return a ValueNode then ValueNode is not really useful anywhere.

github-merge-queue bot pushed a commit that referenced this issue Aug 21, 2024
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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants