feat(hugr-py): python hugr builder #1098

ss2165 · 2024-05-22T14:06:22Z

First steps of #486

Uses cli tool to validate.
Just covers DFG building for now but should be extensible in principle.
Op types are declared explicitly, no "inference".
Hugr does some light stable index maintenance, but rewriting is not prioritised as a use-case.
Both inline nested graph building and full graph insertion are expected use cases so are demonstrated.

cqc-alec

Looks good, I mainly focused on the test code and left a few comments.

hugr-py/src/hugr/utils.py

cqc-alec · 2024-05-22T14:24:26Z

hugr-py/tests/test_hugr_build.py

+    row = [BOOL_T, QB_T]
+    h = Dfg.endo(row)
+    a, b = h.inputs()
+    t = h.make_tuple([a, b], row)


Needing to specify the types here (and below) is potentially annoying but I understand it may be technically hard to avoid.

it is annoying - unfortunately to avoid it generically we need to either add an interface for querying ops for their signature, or take guppy's approach of tracking types where possible while building. I prefer the former approach because it's less flaky but it is quite a lot of extra code, and liable to many edge cases for polymorphic/generic ops.

cqc-alec · 2024-05-22T14:29:20Z

hugr-py/tests/test_hugr_build.py

+def test_multi_out():
+    h = Dfg([INT_T] * 2, [INT_T] * 2)
+    a, b = h.inputs()
+    a, b = h.add_op(DIV_OP, [a, b])[:2]


Could we just write [:] here (assuming we can't omit the indexing)?

we don't know in general how many ports it has, in fact without a stop value an error is raised

annoyingly python is unwilling to just stop an iterator based on how many values it needs to unpack in to - instead it complains that the iterator wants to keep going

cqc-alec · 2024-05-22T14:30:22Z

hugr-py/tests/test_hugr_build.py

+NOT_OP = DummyOp(
+    # TODO get from YAML
+    sops.CustomOp(
+        parent=-1,


Would probably friendlier for users not to need to say parent = -1; is it avoidable?

yes, probably, I just didn't worry about the op interface much yet. We can either generically wrap all serialised ops or change the behaviour of the pydantic models a bit.

cqc-alec · 2024-05-22T14:53:51Z

hugr-py/tests/test_hugr_build.py

+def test_add_op():
+    h = Dfg.endo([BOOL_T])
+    (a,) = h.inputs()
+    nt = h.add_op(NOT_OP, [a])


Did you consider a varargs style (add_op(op, *args)) so that you could write add_op(NOT_OP, a) here?

aborgna-q

Looks clean!

hugr-py/src/hugr/utils.py

aborgna-q · 2024-05-22T15:05:58Z

hugr-py/src/hugr/hugr.py

+class ToPort(Protocol):
+    def to_port(self) -> "OutPort": ...


What is the definition of a ToPort? It's implemented for OutPort and Node, but not for Port nor InPort.

should really be ToOutPort, used so you can use a node in place of the single out port of the node (common for single return ops)

hugr-py/src/hugr/hugr.py

aborgna-q · 2024-05-22T15:24:21Z

hugr-py/src/hugr/hugr.py

+
+
+@dataclass()
+class Dfg:


Move this to another file?

hugr-py/tests/test_hugr_build.py

aborgna-q · 2024-05-22T15:31:32Z

hugr-py/src/hugr/hugr.py

+
+
+@dataclass()
+class Dfg:


Should we think here about a commands-based DFG construction similar to tket's?
(E.g. dfg.add(CNot(target=a, control=b)))

see de22b5a...

codecov · 2024-05-24T15:51:46Z

Codecov Report

Attention: Patch coverage is 94.89489% with 17 lines in your changes are missing coverage. Please review.

Project coverage is 87.04%. Comparing base (96cd3e3) to head (dc71629).

Files	Patch %	Lines
hugr-py/src/hugr/_hugr.py	94.88%	15 Missing ⚠️
hugr-py/src/hugr/utils.py	95.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1098      +/-   ##
==========================================
+ Coverage   86.87%   87.04%   +0.17%     
==========================================
  Files          91       93       +2     
  Lines       18746    19079     +333     
  Branches    18353    18353              
==========================================
+ Hits        16285    16607     +322     
- Misses       1611     1622      +11     
  Partials      850      850

Flag	Coverage Δ
python	`89.39% <94.89%> (+6.18%)`	⬆️
rust	`86.95% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mark-koch

I think so far, the API in its current form should be usable by Guppy.

Below are some thoughts:

Having to specify all types by hand is a bit annoying but should be fine for Guppy since we have figured out all types once we do Hugr generation.
Still, if I had to use to builder to create a Hugr by hand, I would probably get annoyed by the verbosity (e.g. having to specify all element types of a tuple I want to unpack)
Guppy won't be able to use the DFG pattern, were a subgraph is constructed independently and then inserted into a larger graph since we might want to insert non-local edges. I assume we can still use add_op to add a DFG op and then build the contents? Maybe we want an add_dfg method on Hugr as well?
How will the builder handle Const-/Function-/State-/CF-edges? This was one of the main pain points of the Guppy Hugr builder and I think this might complicate the API quite a bit.
Is it the builder's job to ensure that nodes are serialised in the correct order (e.g. input/output are the first children of a DFG)? This becomes relevant if we want to nest graphs by hand and add our own IO nodes.
Will the builder take care of inserting state edges when creating a non-local edge?

mark-koch · 2024-05-28T10:16:35Z

hugr-py/src/hugr/hugr.py

+    _serial_op: T
+
+    def to_serial(self, node: Node, hugr: "Hugr") -> SerialOp:
+        return SerialOp(root=self._serial_op)  # type: ignore


Suggested change

return SerialOp(root=self._serial_op) # type: ignore

return SerialOp(root=self._serial_op.model_copy()) # type: ignore

NodeData.to_serial mutates the model by setting the parent, so you need to create a copy every time you serialise

mark-koch · 2024-05-28T10:37:34Z

hugr-py/src/hugr/hugr.py

+    def to_port(self) -> "OutPort":
+        return OutPort(self, 0)


Maybe to_port should only be allowed if the node only has a single out port?

hugr-py/tests/test_hugr_build.py

hugr-py/src/hugr/hugr.py

mark-koch · 2024-05-28T11:17:40Z

hugr-py/src/hugr/hugr.py

+    @overload
+    def __getitem__(self, index: int) -> OutPort: ...
+    @overload
+    def __getitem__(self, index: slice) -> Iterable[OutPort]: ...


Why Iterable[OutPort] instead of Iterator[OutPort], or Generator[OutPort]?

Is there a good reason why this should be an iterator instead of a list?

I think should be Iterator yes

I figured since it is backed by a range it might as well be lazy like range

mark-koch · 2024-05-28T11:55:26Z

hugr-py/src/hugr/hugr.py

+    def set_outputs(self, *args: ToPort) -> None:
+        self._wire_up(self.output_node, args)
+
+    def make_tuple(self, tys: Sequence[Type], *args: ToPort) -> Node:


How many of these DFG.add_something(...) methods do we want? I agree they are convenient, but they could also just be commands. Looking at the examples, we could also have DFG.not(...) and DFG.divmod(...), but that would probably go too far?

I'm not sure how we should weigh uniformity vs convenience here?

I like the use of Commands, will do

ss2165 · 2024-05-28T12:05:36Z

@mark-koch

Guppy won't be able to use the DFG pattern, were a subgraph is constructed independently and then inserted into a larger graph since we might want to insert non-local edges.

If I understand correctly, this test:

hugr/hugr-py/tests/test_hugr_build.py

Line 225 in 600f18d

def test_build_inter_graph():

shows what you are after? (also answers your other question about state order, auto insertion is possible but not sure if good idea yet)

mark-koch · 2024-05-28T12:08:21Z

Ah yes, sorry I must have missed that. That's what I want 👍

commit 22ce1b7 Author: Seyon Sivarajah <[email protected]> Date: Fri May 24 15:08:11 2024 +0100 fix paths commit 60407bb Author: Seyon Sivarajah <[email protected]> Date: Fri May 24 14:32:36 2024 +0100 empty commit commit 53fd488 Author: Seyon Sivarajah <[email protected]> Date: Fri May 24 14:23:02 2024 +0100 [REVERTME] add ss/pybuild to CI req

so it can be a feature branch

This reverts commit 6c32c28.

passing run: https://github.com/CQCL/hugr/actions/runs/9289115228/job/25562276103 --------- Co-authored-by: Agustín Borgna <[email protected]>

🤖 I have created a release *beep* *boop* --- ## [0.3.0](hugr-py-v0.2.1...hugr-py-v0.3.0) (2024-07-03) ### ⚠ BREAKING CHANGES * * `add_child_op`(`_with_parent`), etc., gone; use `add_child_node`(`_with_parent`) with an (impl Into-)OpType. * `get_nodetype` gone - use `get_optype`. * `NodeType` gone - use `OpType` directly. * Various (Into<)Option<ExtensionSet> params removed from builder methods especially {cfg_,dfg_}builder. * `input_extensions` removed from serialization schema. * the Signature class is gone, but it's not clear how or why you might have been using it... * TailLoop node and associated builder functions now require specifying an ExtensionSet; extension/validate.rs deleted; some changes to Hugrs validated/rejected when the `extension_inference` feature flag is turned on * Type::validate takes extra bool (allow_rowvars); renamed {FunctionType, PolyFuncType}::(validate=>validate_var_len). ### Features * Allow "Row Variables" declared as List<Type> ([#804](#804)) ([3ea4834](3ea4834)) * **hugr-py:** add builders for Conditional and TailLoop ([#1210](#1210)) ([43569a4](43569a4)) * **hugr-py:** add CallIndirect, LoadFunction, Lift, Alias ([#1218](#1218)) ([db09193](db09193)), closes [#1213](#1213) * **hugr-py:** add values and constants ([#1203](#1203)) ([f7ea178](f7ea178)), closes [#1202](#1202) * **hugr-py:** automatically add state order edges for inter-graph edges ([#1165](#1165)) ([5da06e1](5da06e1)) * **hugr-py:** builder for function definition/declaration and call ([#1212](#1212)) ([af062ea](af062ea)) * **hugr-py:** builder ops separate from serialised ops ([#1140](#1140)) ([342eda3](342eda3)) * **hugr-py:** CFG builder ([#1192](#1192)) ([c5ea47f](c5ea47f)), closes [#1188](#1188) * **hugr-py:** define type hierarchy separate from serialized ([#1176](#1176)) ([10f4c42](10f4c42)) * **hugr-py:** only require input type annotations when building ([#1199](#1199)) ([2bb079f](2bb079f)) * **hugr-py:** python hugr builder ([#1098](#1098)) ([23408b5](23408b5)) * **hugr-py:** store children in node weight ([#1160](#1160)) ([1cdaeed](1cdaeed)), closes [#1159](#1159) * **hugr-py:** ToNode interface to treat builders as nodes ([#1193](#1193)) ([1da33e6](1da33e6)) * Validate Extensions using hierarchy, ignore input_extensions, RIP inference ([#1142](#1142)) ([8bec8e9](8bec8e9)) ### Bug Fixes * Add some validation for const nodes ([#1222](#1222)) ([c05edd3](c05edd3)) * **hugr-py:** more ruff lints + fix some typos ([#1246](#1246)) ([f158384](f158384)) * **py:** get rid of pydantic config deprecation warnings ([#1084](#1084)) ([52fcb9d](52fcb9d)) ### Documentation * **hugr-py:** add docs link to README ([#1259](#1259)) ([d2a9148](d2a9148)) * **hugr-py:** build and publish docs ([#1253](#1253)) ([902fc14](902fc14)) * **hugr-py:** docstrings for builder ([#1231](#1231)) ([3e4ac18](3e4ac18)) ### Code Refactoring * Remove "Signature" from hugr-py ([#1186](#1186)) ([65718f7](65718f7)) * Remove NodeType and input_extensions ([#1183](#1183)) ([ea5213d](ea5213d)) --- 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: hugrbot <[email protected]> Co-authored-by: Seyon Sivarajah <[email protected]>

ss2165 requested review from mark-koch, cqc-alec and aborgna-q May 22, 2024 14:06

Base automatically changed from ss/cli to main May 22, 2024 14:09

ss2165 force-pushed the ss2165/pybuild branch from ced5263 to dfcec50 Compare May 22, 2024 14:11

ss2165 changed the title ~~feat: (RFC) python hugr builder~~ feat(python): (RFC) python hugr builder May 22, 2024

ss2165 force-pushed the ss2165/pybuild branch from dfcec50 to b26748e Compare May 22, 2024 14:52

cqc-alec reviewed May 22, 2024

View reviewed changes

ss2165 force-pushed the ss2165/pybuild branch from b26748e to 1cd9598 Compare May 22, 2024 14:55

aborgna-q reviewed May 22, 2024

View reviewed changes

ss2165 force-pushed the ss2165/pybuild branch 6 times, most recently from b25993c to 5543473 Compare May 24, 2024 15:44

mark-koch reviewed May 28, 2024

View reviewed changes

ss2165 force-pushed the ss2165/pybuild branch 4 times, most recently from e431005 to 79fc717 Compare May 29, 2024 15:41

ss2165 marked this pull request as ready for review May 29, 2024 17:51

ss2165 requested a review from a team as a code owner May 29, 2024 17:51

ss2165 requested review from zrho and aborgna-q and removed request for zrho May 29, 2024 17:51

ss2165 changed the title ~~feat(python): (RFC) python hugr builder~~ feat(hugr-py): python hugr builder May 29, 2024

ss2165 and others added 22 commits May 31, 2024 17:20

differentiate ports and connected links correctly

cc6719e

import Self from typing extensions

bcb2669

make mypy happy

3052c2d

chmod +x

df796c7

fix: bimap existing key overwrite

53cca4a

minor comments

9802c58

separate sub ports in to internal type

329f13a

rename ToPort to Wire

dd9e351

feat: inter graph + state order edge

0d55959

put add_dfg in hugr

5ba5ad9

return Iterator from getitem

9ebab76

remove special tuple methods

27014ba

copy model when serialising

f02a0b8

implement from_serial

1862c4e

fixup binary building (feature name changed)

14f5933

build cli package

cecebc1

revert CI to main

b8a2642

[REVERTME] enable CI on PRs in to this branch

de9d577

so it can be a feature branch

Revert "[REVERTME] enable CI on PRs in to this branch"

b751e0d

This reverts commit 6c32c28.

ci: use binary build stage in python CI (#1135)

13e54f6

passing run: https://github.com/CQCL/hugr/actions/runs/9289115228/job/25562276103 --------- Co-authored-by: Agustín Borgna <[email protected]>

feat: put hugr.py behind an underscore

13fcde3

ss2165 force-pushed the ss2165/pybuild branch from 47f95c4 to 13fcde3 Compare May 31, 2024 16:22

add direction to _Port

dc71629

ss2165 added this pull request to the merge queue May 31, 2024

Merged via the queue into main with commit 23408b5 May 31, 2024
22 checks passed

ss2165 deleted the ss2165/pybuild branch May 31, 2024 16:31

hugrbot mentioned this pull request May 31, 2024

chore(main): release hugr-py 0.3.0 #1092

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(hugr-py): python hugr builder #1098

feat(hugr-py): python hugr builder #1098

ss2165 commented May 22, 2024 •

edited

Loading

cqc-alec left a comment

cqc-alec May 22, 2024

ss2165 May 22, 2024

cqc-alec May 22, 2024

ss2165 May 22, 2024

ss2165 May 22, 2024

cqc-alec May 22, 2024

ss2165 May 22, 2024

cqc-alec May 22, 2024

aborgna-q left a comment

aborgna-q May 22, 2024

ss2165 May 22, 2024

aborgna-q May 22, 2024

aborgna-q May 22, 2024

ss2165 May 22, 2024

codecov bot commented May 24, 2024 •

edited

Loading

mark-koch left a comment

mark-koch May 28, 2024

mark-koch May 28, 2024

mark-koch May 28, 2024

mark-koch May 28, 2024

ss2165 May 29, 2024

mark-koch May 28, 2024

ss2165 May 29, 2024

ss2165 commented May 28, 2024

mark-koch commented May 28, 2024

	return SerialOp(root=self._serial_op) # type: ignore
	return SerialOp(root=self._serial_op.model_copy()) # type: ignore

feat(hugr-py): python hugr builder #1098

feat(hugr-py): python hugr builder #1098

Conversation

ss2165 commented May 22, 2024 • edited Loading

cqc-alec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aborgna-q left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented May 24, 2024 • edited Loading

Codecov Report

mark-koch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ss2165 commented May 28, 2024

mark-koch commented May 28, 2024

ss2165 commented May 22, 2024 •

edited

Loading

codecov bot commented May 24, 2024 •

edited

Loading