feat(hugr-py): add builders for Conditional and TailLoop #1210

ss2165 · 2024-06-20T15:18:57Z

Closes #1204

Including a short hand for if/else style conditionals

Includes some other simplifications and refactors, reccommend going commit by commit

and make cfg outputs optional

also common up some functions and use tys.Sum where applicable

codecov · 2024-06-20T15:22:28Z

Codecov Report

Attention: Patch coverage is 96.47577% with 8 lines in your changes missing coverage. Please review.

Project coverage is 86.88%. Comparing base (f7ea178) to head (4b5cd90).

Files	Patch %	Lines
hugr-py/src/hugr/_ops.py	95.65%	4 Missing ⚠️
hugr-py/src/hugr/_cond_loop.py	97.10%	2 Missing ⚠️
hugr-py/src/hugr/_dfg.py	95.23%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1210      +/-   ##
==========================================
+ Coverage   86.73%   86.88%   +0.14%     
==========================================
  Files          97       98       +1     
  Lines       18351    18530     +179     
  Branches    16827    16827              
==========================================
+ Hits        15917    16099     +182     
+ Misses       1660     1657       -3     
  Partials      774      774

Flag	Coverage Δ
python	`91.30% <96.47%> (+1.21%)`	⬆️

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.

ss2165 · 2024-06-20T15:24:25Z

hugr-py/src/hugr/_ops.py

        )

-    def __call__(self, value: Wire) -> Command:


tag takes a whole row, not just one wire, so specialisation not needed

cqc-alec

Looks good, just a few comments.

General comment (not specific to this PR) that the Python code could do with more docstrings.

cqc-alec · 2024-06-20T15:41:14Z

hugr-py/src/hugr/_cfg.py

@@ -97,6 +97,8 @@ def add_block(self, input_types: TypeRow) -> Block:
        )
        return new_block

+    # TODO insert_block


TODO now or later?

later - I'm not convinced it's a common enough use case to do immediately

cqc-alec · 2024-06-20T15:42:33Z

hugr-py/src/hugr/_cond_loop.py

+        self.cases[case_id] = new_case.parent_node
+        return new_case
+
+    # TODO insert_case


TODO now or later?

later - I'm not convinced it's a common enough use case to do immediately

cqc-alec · 2024-06-20T15:44:34Z

hugr-py/src/hugr/_dfg.py

-        mapping = self.hugr.insert_hugr(dfg.hugr, self.parent_node)
-        self._wire_up(mapping[dfg.parent_node], args)
-        return mapping[dfg.parent_node]
+        return self._insert_nested_impl(dfg, *args)


Why is this delegated to an internal method?

the internal method is generic, the surface ones are concrete for cases we know
about. Allows special casing in future if necessary.

cqc-alec · 2024-06-20T16:00:36Z

hugr-py/src/hugr/_dfg.py

+    ) -> TailLoop:
+        from ._cond_loop import TailLoop
+
+        rest = rest or []


Why is this necessary?

vestigial, will remove

cqc-alec · 2024-06-20T16:01:41Z

hugr-py/tests/test_cond_loop.py

+
+    _validate(h.hugr, True)
+
+    # TODO rewrite with context managers


TODO now or later?

later, it's an optional task in the tracking issue and not necessary for the
first milestone (guppy port)

cqc-alec · 2024-06-21T10:08:10Z

hugr-py/src/hugr/_dfg.py

+        self._wire_up(cond.parent_node, args)
+        return cond
+
+    def insert_conditional(self, cond: Conditional, *args: Wire) -> Node:


Add a test?

cqc-alec · 2024-06-21T10:24:21Z

hugr-py/src/hugr/_cond_loop.py

+            assert outputs == self.parent_op._outputs, "Mismatched case outputs."
+
+    def add_case(self, case_id: int) -> Case:
+        assert case_id in self.cases, f"Case {case_id} out of possible range."


I think some of these asserts should be raises instead (maybe ValueError?), since they occur through use of the API beyond our control.

cqc-alec · 2024-06-21T10:29:17Z

hugr-py/src/hugr/_cond_loop.py

+
+
+class Else(_IfElse):
+    def finish(self) -> Node:


It would be nice if it were possible to finish an _IfElse with a trivial Else without having to explicitly add the Else.

Yeah I thought about this but unfortunately the form of the "trivial else" depends on linearity of types, so instead of making too many assumptions decided to leave it explicit for now.

cqc-alec · 2024-06-21T10:37:36Z

hugr-py/src/hugr/_dfg.py

+        self._wire_up(tl.parent_node, (*just_inputs, *rest))
+        return tl
+
+    def insert_tail_loop(self, tl: TailLoop, *args: Wire) -> Node:


Add a test?

ss2165 · 2024-06-21T10:53:20Z

Docstrings will be added before the tracking issue is closed: #486

🤖 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 added 8 commits June 20, 2024 10:09

add_cfg without inputs

881d9c3

and make cfg outputs optional

make dfg consistent with other ops

d0bc866

feat(hugr-py): add conditional building

b1665d9

also common up some functions and use tys.Sum where applicable

make constant loading easier

532a82b

endo constructor for functiontype

d0f1ec5

feat: convenient constructors for if-else workflows

e2d6644

feat: add tail loop building

9863375

add complex loop test

7c56560

ss2165 requested a review from a team as a code owner June 20, 2024 15:18

ss2165 requested review from acl-cqc and cqc-alec and removed request for acl-cqc June 20, 2024 15:18

revert accidental annotation removal

939772c

ss2165 commented Jun 20, 2024

View reviewed changes

cqc-alec reviewed Jun 21, 2024

View reviewed changes

ss2165 added 3 commits June 21, 2024 12:05

refactor: turn conditional asserts in to exceptions

c8105bb

remove vestigial line

9166f7f

add insertion tests

4b5cd90

ss2165 requested a review from cqc-alec June 21, 2024 11:25

cqc-alec approved these changes Jun 21, 2024

View reviewed changes

ss2165 added this pull request to the merge queue Jun 21, 2024

Merged via the queue into main with commit 43569a4 Jun 21, 2024
20 checks passed

ss2165 deleted the ss/scf branch June 21, 2024 11:40

hugrbot mentioned this pull request Jun 21, 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): add builders for Conditional and TailLoop #1210

feat(hugr-py): add builders for Conditional and TailLoop #1210

ss2165 commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading

ss2165 Jun 20, 2024

cqc-alec left a comment

cqc-alec Jun 20, 2024

ss2165 Jun 21, 2024

cqc-alec Jun 20, 2024

ss2165 Jun 21, 2024

cqc-alec Jun 20, 2024

ss2165 Jun 21, 2024

cqc-alec Jun 20, 2024

ss2165 Jun 21, 2024

cqc-alec Jun 20, 2024

ss2165 Jun 21, 2024

cqc-alec Jun 21, 2024

cqc-alec Jun 21, 2024

cqc-alec Jun 21, 2024

ss2165 Jun 21, 2024

cqc-alec Jun 21, 2024

ss2165 commented Jun 21, 2024

feat(hugr-py): add builders for Conditional and TailLoop #1210

feat(hugr-py): add builders for Conditional and TailLoop #1210

Conversation

ss2165 commented Jun 20, 2024 • edited Loading

codecov bot commented Jun 20, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

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

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 Jun 21, 2024

ss2165 commented Jun 20, 2024 •

edited

Loading

codecov bot commented Jun 20, 2024 •

edited

Loading