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

fix: Update number of ports for PartialOps, and sanitize orderd edges #1635

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

aborgna-q
Copy link
Collaborator

The number of ports in a PartialOp dataflow node didn't get updated after connecting it.
We didn't see any problem with this because by default the number of output ports is grown when connecting new outputs, so the count ended up being correct.

#1625 found a bug where an UnpackTuple didn't connect its last output, so the serialization though the node had less output ports than it should have, and connected the order edge at an incorrect offset.

As part of this change I added various checks for the order edge index. We use a special -1 offset to identify them, so we should panic when that is seen on invalid places.

  • drive-by: Add a Hugr::has_link method
  • drive-by: Avoid adding duplicated order edges

Closes #1625

@aborgna-q aborgna-q requested a review from a team as a code owner November 5, 2024 16:43
@aborgna-q aborgna-q requested a review from ss2165 November 5, 2024 16:43
# https://github.com/CQCL/hugr/issues/1625
def test_dfg_unpack() -> None:
dfg = Dfg(tys.Tuple(tys.Bool, tys.Bool))
bool1, _unused_bool2 = dfg.add_op(ops.UnpackTuple(), *dfg.inputs())
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Notice that we don't use the node's 2nd output.

@aborgna-q aborgna-q added this pull request to the merge queue Nov 6, 2024
Merged via the queue into main with commit 81a1385 Nov 6, 2024
19 checks passed
@aborgna-q aborgna-q deleted the ab/fix/hugr-build-validation branch November 6, 2024 13:35
ss2165 pushed a commit that referenced this pull request Nov 22, 2024
…#1635)

The number of ports in a `PartialOp` dataflow node didn't get updated
after connecting it.
We didn't see any problem with this because by default the number of
output ports is grown when connecting new outputs, so the count ended up
being correct.

#1625 found a bug where an `UnpackTuple` didn't connect its last output,
so the serialization though the node had less output ports than it
should have, and connected the order edge at an incorrect offset.

As part of this change I added various checks for the order edge index.
We use a special `-1` offset to identify them, so we should panic when
that is seen on invalid places.

- drive-by: Add a `Hugr::has_link` method
- drive-by: Avoid adding duplicated order edges

Closes #1625
github-merge-queue bot pushed a commit that referenced this pull request Dec 16, 2024
🤖 I have created a release *beep* *boop*
---


##
[0.10.0](hugr-py-v0.9.0...hugr-py-v0.10.0)
(2024-12-16)


### ⚠ BREAKING CHANGES

* Removed the extension registry argument from `validate` calls. Removed
the extension registry argument from operation instantiation methods.
Removed most extension-specific test registries. Use `EMPTY_REG`,
`PRELUDE_REGISTRY`, or `STD_REG` instead.
* `extension_reqs` field in FunctionType and Extension renamed to
`runtime_reqs`
* Array type and operations have been moved out of `prelude` and into a
new `collections.array` extension. (py) `list_type` method replaced with
`List` class. Removed `Array` type variant from the serialization
format.
* `collections` extension renamed to `collections.list`
* The `LoadFunction::signature` field is removed. Replace uses with
`DataflowOpTrait::signature()`.
* Array `scan` and `repeat` ops get an additional type parameter
specifying the extension requirements of their input functions.
Furthermore, `repeat` is no longer part of `ArrayOpDef` but is instead
specified via a new `ArrayScan` struct.

### Features

* Add `LoadNat` operation to enable loading generic `BoundedNat`s into
runtime values ([#1763](#1763))
([6f035d6](6f035d6)),
closes [#1629](#1629)
* Add array `repeat` and `scan` ops
([#1633](#1633))
([649589c](649589c)),
closes [#1627](#1627)
* Automatically add the custom op's extension to its 'runtime_reqs' set
([#1787](#1787))
([3ef5bd9](3ef5bd9))
* Don't require explicit extension registers for validation
([#1784](#1784))
([b517dc3](b517dc3))
* Make array repeat and scan ops generic over extension reqs
([#1716](#1716))
([4c1c6ee](4c1c6ee))
* Move arrays from prelude into new extension
([#1770](#1770))
([187ea8f](187ea8f))
* Rename `collections` extension to `collections.list`
([#1764](#1764))
([eef239f](eef239f))
* rename `extension_reqs` to `runtime_reqs`
([#1776](#1776))
([5f5bce4](5f5bce4))


### Bug Fixes

* hugr-py not adding extension-reqs on custom ops
([#1759](#1759))
([97ba7f4](97ba7f4))
* **py:** allow conditional cases to be defined out of order
([#1599](#1599))
([583d21d](583d21d))
* Replace `LoadFunction::signature` with `LoadFunction::instantiation`
([#1756](#1756))
([5b50d1d](5b50d1d))
* Resolve types in `Value`s and custom consts
([#1779](#1779))
([080eaae](080eaae))
* Update number of ports for PartialOps, and sanitize orderd edges
([#1635](#1635))
([81a1385](81a1385)),
closes [#1625](#1625)

---
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: Seyon Sivarajah <[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.

Incorrect inter-graph edge validation error
2 participants