Skip to content

Conversation

@aborgna-q
Copy link
Collaborator

@aborgna-q aborgna-q commented May 2, 2025

Adds entrypoint definitions to hugr-py. Has similar caveats to #2147.

The main issue here was builders that initialized a hugr for a dataflow op without knowing its output types. The automatic machinery that wraps the op in a function definition needs to connect the outputs to the new function's output once we know the types, so I had to add some extra machinery to DataflowOp to signal back to the hugr that it should connect things.
This feels a bit hacky, perhaps there's a cleaner way to do it...

Blocked by #2147.

BREAKING CHANGE: Hugrs now define an entrypoint in addition to a module root.

@aborgna-q aborgna-q mentioned this pull request May 2, 2025
4 tasks
@codecov
Copy link

codecov bot commented May 2, 2025

Codecov Report

Attention: Patch coverage is 83.64486% with 35 lines in your changes missing coverage. Please review.

Project coverage is 82.02%. Comparing base (125b341) to head (84938e4).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
hugr-py/src/hugr/ops.py 57.77% 19 Missing ⚠️
hugr-py/src/hugr/hugr/base.py 88.18% 13 Missing ⚠️
hugr-py/src/hugr/build/dfg.py 91.66% 2 Missing ⚠️
hugr-py/src/hugr/build/function.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main    #2148    +/-   ##
========================================
  Coverage   82.02%   82.02%            
========================================
  Files         234      234            
  Lines       41431    41587   +156     
  Branches    37532    37532            
========================================
+ Hits        33984    34112   +128     
- Misses       5474     5502    +28     
  Partials     1973     1973            
Flag Coverage Δ
python 85.49% <83.64%> (-0.14%) ⬇️
rust 81.65% <ø> (ø)

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@aborgna-q aborgna-q force-pushed the ab/python-entrypoints branch from 69ea505 to 634d8fa Compare May 6, 2025 15:19
@aborgna-q aborgna-q added this to the hugr-rs 0̶.̶1̶6̶ 0.20 milestone May 6, 2025
@aborgna-q aborgna-q force-pushed the ab/python-entrypoints branch from 634d8fa to e0a9dca Compare May 7, 2025 10:49
@aborgna-q aborgna-q force-pushed the ab/python-entrypoints branch from e0a9dca to e770ef9 Compare May 7, 2025 11:14
github-merge-queue bot pushed a commit that referenced this pull request May 8, 2025
Adds the concept of "entrypoint" to the HUGRs, detaching the "main node
of interest" from the hierarchy root.

Now hugrs will **always** have a `Module` at their root (appropriately
called `HugrView::module_root`), and any manipulation is mostly focused
around the entrypoint instead.

Closes #2029 
Closes #2010

before:
```mermaid
graph LR

subgraph 0 ["(0) DFG"]

direction LR

1["(1) Input"]

2["(2) Output"]

3["(3) test.quantum.CX"]

4["(4) test.quantum.CX"]

1--"0:0<br>qubit"-->3

1--"1:1<br>qubit"-->3

3--"0:1<br>qubit"-->4

3--"1:0<br>qubit"-->4

3-."2:2".->4

4--"0:0<br>qubit"-->2

4--"1:1<br>qubit"-->2

end
```
after:
```mermaid
graph LR

subgraph 0 ["(0) Module"]

direction LR

subgraph 1 ["(1) FuncDefn: #quot;main#quot;"]

direction LR

2["(2) Input"]

3["(3) Output"]

subgraph 4 ["(4) [**DFG**]"]

direction LR

style 4 stroke:#832561,stroke-width:3px

5["(5) Input"]

6["(6) Output"]

7["(7) test.quantum.CX"]

8["(8) test.quantum.CX"]

5--"0:0<br>qubit"-->7

5--"1:1<br>qubit"-->7

7--"0:1<br>qubit"-->8

7--"1:0<br>qubit"-->8

7-."2:2".->8

8--"0:0<br>qubit"-->6

8--"1:1<br>qubit"-->6

end

2--"0:0<br>qubit"-->4

2--"1:1<br>qubit"-->4

4--"0:0<br>qubit"-->3

4--"1:1<br>qubit"-->3

end

end
```

# How does this affect...

### ...HugrView?

`::root` has been split into `module_root` and `entrypoint`. In general,
you'll want to use the latter (as described in the docs).
`nodes()` is still defined, but now there's also `descendants(node)` and
`entry_descendants()`, which should be preferred.
### ...builders?

The change should be transparent for hugr building operations. When you
start an specific builder or call `Hugr::new(op)` we add additional
nodes as necessary so that the new entrypoint is correctly defined
inside a module;
- If `op` is a module, we just leave it at the top.
- If `op` can be defined in a module, we put it below the root.
- If `op` is a dataflow operation, we define a "main" function with the
same signature and put it there.
- More exotic operations are not allowed, you'll need to build the
container yourself and change the entrypoint afterwards.

### ...SiblingGraph, SiblingMut, and DescendantsGraph?

The structs are no more. Instead, `HugrView` has two new methods:
- `with_entrypoint(&self, Node) -> Rerooted<&Self>`
- `with_entrypoint_mut(&mut self, Node) -> Rerooted<&mut Self>`
The new wrapper implements `HugrMut`, so it can be used transparently.

The main benefit of this is that `Rerooted` still contains all the nodes
that are not descendants from the entrypoint. So external edges are
still well-defined, and rewrite operations may still put look at the
root module and define things there if needed.

### ...serialization?

The hugr json now has an `entrypoint` field. If missing, we assume it's
the root.
For backwards compatibility, if the serialized root is not a module we
will transparently wrap it as described before so older jsons will
continue working without modifications.

hugr-module encoding is left as a TODO 
### ...packages and envelopes?

Now every HUGR can be put in a package without modifications!
Ideally we'll now be able to remove the hugr <-> json methods and only
use envelopes. (That's a TODO).
### ...mermaid/dot renders?

The entrypoint node is now highlighted in purple, and its title shown in
bold between brackets. See the example above.
### ...passes?

Things should work as normal. New passes should look at the entrypoint
and its descendants when looking for things to rewrite, instead of the
whole hugr.

# TODOs not in this PR:

- [ ] Python support. #2148
- [ ] `hugr-module` support #2156
- [ ] Deprecate/remove hugr json methods, always use envelopes instead.
#2159
- [ ] Update the spec.

BREAKING CHANGE: `Hugr`s now have an entrypoint node.
BREAKING CHANGE: Removed `SiblingGraph` / `SiblingMut` /
`DescendantsGraph`

---------

Co-authored-by: Seyon Sivarajah <[email protected]>
Base automatically changed from ab/entrypoints to main May 8, 2025 12:30
@aborgna-q aborgna-q force-pushed the ab/python-entrypoints branch from e770ef9 to 73f3775 Compare May 8, 2025 12:45
@aborgna-q aborgna-q marked this pull request as ready for review May 8, 2025 13:46
@aborgna-q aborgna-q requested a review from a team as a code owner May 8, 2025 13:46
@aborgna-q aborgna-q requested a review from jake-arkinstall May 8, 2025 13:46
@ss2165 ss2165 requested review from ss2165 and removed request for jake-arkinstall May 15, 2025 15:31
@aborgna-q aborgna-q force-pushed the ab/python-entrypoints branch from b24c05f to 5032dd3 Compare May 15, 2025 15:32
new = cls.__new__(cls)
new.hugr = hugr
new.parent_node = root.to_node()
[inp, out] = hugr.children(root)[:2]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will error if the root doesn't have io nodes? e.g. if the root was module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It now checks that the op is a dataflow parent and raises a better error otherwise

#
# Most traversals and rewrite operations start from this node.
#
# This node may be of any optype, and is a descendant of the module
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on the init errors it can't be any optype, only some valid ones

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also missing the change we did on the rust side restricting entrypoints to only region parents (#2173).

I fixed that, so now the logic is a bit different.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume these changes have been tested (maybe attach an image to the PR?)

Copy link
Collaborator Author

@aborgna-q aborgna-q May 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do test snapshots of these.

Here's a render of test_basic_cfg. I added a little change to highlight the entrypoint region too.

hugr dot

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...we should probably find a less awful color tbh

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: better colorscheme
hugr dot

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a follow up task for "entrypoint only" rendering, should make debugging a lot easier

Comment on lines 154 to 157
match df_op:
case CFG():
pass
case Conditional():
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these could be in a single CFG() | Conditional()

@aborgna-q aborgna-q force-pushed the ab/python-entrypoints branch from 8870628 to 84938e4 Compare May 16, 2025 12:42
@aborgna-q aborgna-q enabled auto-merge May 16, 2025 12:44
@aborgna-q aborgna-q added this pull request to the merge queue May 16, 2025
Merged via the queue into main with commit ef8ea5e May 16, 2025
27 checks passed
@aborgna-q aborgna-q deleted the ab/python-entrypoints branch May 16, 2025 12:53
github-merge-queue bot pushed a commit that referenced this pull request May 16, 2025
Followup to #2148

- Adds envelope reading/writting methods to `Hugr` (py) that mimic
Package.
- Deprecates `to_json` and `load_json`.
- Uses envelopes in python tests.

Note that `EnvelopeConfig.BINARY` is still set to JSON.
Using `hugr-module` there currently causes errors.
github-merge-queue bot pushed a commit that referenced this pull request May 16, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.12.0](hugr-py-v0.11.5...hugr-py-v0.12.0)
(2025-05-16)


### ⚠ BREAKING CHANGES

* Hugrs now define an `entrypoint` in addition to a module root.
* `std.collections.array` is now a linear type, even if the contained
elements are copyable. Use the new `std.collections.value_array` for an
array with the previous copyable semantics.
* `std.collections.array.get` now also returns the passed array as an
extra output.
* `ArrayOpBuilder` was moved from
`hugr_core::std_extensions::collections::array::op_builder` to
`hugr_core::std_extensions::collections::array`.
* Functions that manipulate runtime extension sets have been removed
from the Rust and Python code. Extension set parameters were removed
from operations.
* `values` field in `Extension` and `ExtensionValue` struct/class
removed in rust and python. Use 0-input ops that return constant values.

### Features

* Entrypoints in `hugr-py`
([#2148](#2148))
([ef8ea5e](ef8ea5e))
* **hugr-py:** Add `to/from_bytes/str` to Hugr, using envelopes
([#2228](#2228))
([9985143](9985143))
* Improved array lowering
([#2109](#2109))
([1bc91c1](1bc91c1))
* Remove description on opaque ops.
([#2197](#2197))
([f6163bf](f6163bf))
* remove ExtensionValue
([#2093](#2093))
([70881b7](70881b7))
* Removed runtime extension sets.
([#2145](#2145))
([cd7ef68](cd7ef68))

---
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: Agustín Borgna <[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 this pull request may close these issues.

3 participants