Closed ss2165 closed 1 month ago
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:
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
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 *PortV
variants include their type and and a non-null offset.
In the case of Node
s, 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.
Guppy's Node
s and value Port
s 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 FatNode
s, but here I believe we can always access the base hugr so it's not that necessary to keep the data around.
Node
s 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.
I'd like to propose the following:
hugr-py
, but drop the specialised parent-node classes as these are already covered by ParentBuilder
's subclasses.The hugr classes on both sides are fairly similar, so hopefully we shouldn't need to change hugr-py
much more after this.
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...
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.
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
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.
~Blocked by non-optional tasks in tracking issue: https://github.com/CQCL/hugr/issues/486~
~Now blocked by https://github.com/CQCL/guppylang/issues/280 to avoid bad merge conflicts. (Base on working branch for that issue if starting before that is merged)~