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

Incorrect inter-graph edge validation error #1625

Closed
mark-koch opened this issue Nov 4, 2024 · 0 comments · Fixed by #1635 or #1610
Closed

Incorrect inter-graph edge validation error #1625

mark-koch opened this issue Nov 4, 2024 · 0 comments · Fixed by #1635 or #1610
Assignees
Labels
bug Something isn't working

Comments

@mark-koch
Copy link
Contributor

mark-koch commented Nov 4, 2024

The following Hugr is expected to be valid

from hugr.build.dfg import Dfg
from hugr import tys, ops

dfg = Dfg(tys.Tuple(tys.Bool, tys.Bool))
bool1, _unused_bool2 = dfg.add_op(ops.UnpackTuple(), *dfg.inputs())
cond = dfg.add_conditional(bool1)
with cond.add_case(0) as case:
    case.set_outputs(bool1)
with cond.add_case(1) as case:
    case.set_outputs(bool1)
dfg.set_outputs(*cond.outputs())

but fails validation with

Error validating HUGR: Missing state order between the external inter-graph source Node(3) and the ancestor of the target Node(4). In an external inter-graph edge from Node(3) (Port(Outgoing, 0)) to Node(7) (Port(Incoming, 0)).

Note that it's important that the external source comes from an UnpackTuple node where the second out-port is not connected. Something seems to go wrong when the builder adds the implicit order edge from the UnpackTuple to the Conditional node. Either the builder does something wrong here or validation handles state order edges incorrectly if the last port of the source node is not connected

@mark-koch mark-koch added the bug Something isn't working label Nov 4, 2024
@aborgna-q aborgna-q self-assigned this Nov 5, 2024
aborgna-q added a commit that referenced this issue Nov 5, 2024
github-merge-queue bot pushed a commit that referenced this issue Nov 6, 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
ss2165 pushed a commit that referenced this issue 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 issue 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
bug Something isn't working
Projects
None yet
2 participants