feat+fix!:(RFC) Fewer, tho occasionally more, order edges when inlining

[acl-cqc]

Two parts here.

1. Currently the algorithm is that any node linked via an order edge to a DFG being inlined, becomes linked to every input-successor/output-predecessor (by any edge type, not just order); and any node inside the DFG linked via an order edge to the input/output, becomes linked to every predecessor/successor (not just order) of the DFG. This kinda embodies the idea that the DFG executes "atomically" (except for nodes in the DFG not linked from the Input, e.g. Const/LoadConstant), i.e. quite a conservative idea of what nodes might have side-effects...

Instead, link only to order successors/predecessors. Thus, treating Order edges as its own edge type, which is kinda consistent with how we treat other edge types.

2. Also includes a fix that exists on its own in fix: More edges in InlineDFG #3072

• TODO: if we inline a DFG with internal order edges (from Input/to Output) when there are no order edges to/from the DFG itself. Previously the internal Order-targets would be ordered after all value inputs to the DFG. Now they'll have no Order dependencies. What should we do? Connect these to the containing Input?

Is this breaking? This is somewhat debatable. The spec says, at least:

all nodes in the graph are necessarily evaluated, in some order (perhaps parallel)
respecting the Dataflow and Order edges. This may include interleaving or
overlapping evaluation of siblings and descendants as long as said edges
are respected.

but I haven't been through the whole spec to see if it says anything else as well. The idea that DFGs (and hence Calls, etc.) execute "atomically" has been rather implicit and the question may be, what code is there that assumes this. One definite problem is RemoveRedundantOrderEdges pass, which will need to be updated not to touch order edges whose source/target is Input, Output, or any container node or a Call. Guppy programs should be ok, as they always have neat chains of order edges; Hugrs constructed by other means may not and might be broken by this PR (or might have already been broken before this PR) - where being "broken" means "may lead to wrong program results depending on topsorts taken by the compiler", emphasis on the may.

Possible mitigations?

• Rename DFG and Call to WeaklyOrderedDFG and WeaklyOrderedCall at the same time ;-). Then everyone will know to look very carefully at what they're doing ;).
• Add metadata: weakly_ordered=true enables the new policy, false/absent keeps the old policy. InlineCall needs to merge metadata from Call and callee. Switch guppy to add the metadata. Nice that this gives a fallback, but the long-term strategy to move the metadata to default-true (and then eventually remove it) is unclear.
• Just hope that everyone is using guppy

I've had a look at InQuanto's non-guppy code for generating Hugrs. They take a "flat circuit" function from pytket, (Qubit^N,Bool^M) -> (Qubit^N',Bool^M') and wire up N QAllocs and M Const(false)s to the inputs, and N' QFree's and M' results to the outputs. No order edges...

[hugrbot]

Hey there and thank you for opening this pull request! 👋

We follow the Conventional Commits convention for PR titles. It looks like your title needs some adjustment.

The title should have a type prefix, followed by a colon. The most important ones are:

• feat: for new features
• fix: for bug fixes

If the PR contains a breaking change, use feat!: or fix!: instead and include a "BREAKING CHANGE:" footer in the description of the pull request.

You may also include a (scope) after the type prefix.

Expand this message for the full list of tags.

• feat: New feature
• fix: Bug fix
• docs: Documentation-only changes
• style: Changes that do not affect the meaning of the code (white-space, formatting, missing semicolons, etc)
• refactor: A code change that neither fixes a bug nor adds a feature
• perf: Performance improvements
• test: Adding missing or correcting existing tests
• ci: Changes to our CI configuration files and scripts
• chore: Other changes that do not alter public APIs or code behaviour
• revert: Reverts a previous commit

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 81.17%. Comparing base (e588703) to head (3d25704).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3058   +/-   ##
=======================================
  Coverage   81.16%   81.17%           
=======================================
  Files         240      240           
  Lines       45030    44975   -55     
  Branches    38788    38731   -57     
=======================================
- Hits        36549    36508   -41     
- Misses       6507     6508    +1     
+ Partials     1974     1959   -15     

Flag	Coverage Δ
python	88.88% <ø> (+<0.01%)	⬆️
rust	79.93% <100.00%> (+<0.01%)	⬆️

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.

[acl-cqc]

I am starting to think that perhaps unparametrizing the test - with a fixture returning the Hugr, and all 5 relevant nodes/ops, allowing individual tests to add extra order edges, apply the patch, and test what Order edges are present after, might be clearer. But, a lot longer, I think...

[cqc-alec]

Interesting. It isn't clear to me that the existing behaviour is wrong. Doesn't an order edge mean that nothing in the target node should execute until everything in the source node has executed -- even if one or both of those nodes is a DFG?

Or am I missing the point?

[acl-cqc]

Interesting. It isn't clear to me that the existing behaviour is wrong. Doesn't an order edge mean that nothing in the target node should execute until everything in the source node has executed -- even if one or both of those nodes is a DFG?

Or am I missing the point?

Yes and no. Prior to this PR, an order edge indeed means "nothing in the target (of the Order edge) should execute until everything in the source". That is, every DFG includes an implicit order-barrier across the inputs and outputs (not quite a normal barrier: dependencies solely by value edges are not barriered). Note there is no way not to have that barrier!

Post this PR, there is no such barrier: Input/Output/DFG are transparent and just forward edges (the edge into the DFG feeds directly to the corresponding output edge(s) from the Input, only). If you want barriers, add them explicitly.

Which is why this is "breaking", but I'm not sure (need to make certain about) to what extent the spec actually says the former (and needs updating), or whether the spec is unclear (needs changing, i.e. this is a clarification), or whether this is actually a pure "fix"

[cqc-alec]

Interesting. It isn't clear to me that the existing behaviour is wrong. Doesn't an order edge mean that nothing in the target node should execute until everything in the source node has executed -- even if one or both of those nodes is a DFG?
Or am I missing the point?

Yes and no. Prior to this PR, an order edge indeed means "nothing in the target (of the Order edge) should execute until everything in the source". That is, every DFG includes an implicit order-barrier across the inputs and outputs (not quite a normal barrier: dependencies solely by value edges are not barriered). Note there is no way not to have that barrier!

Post this PR, there is no such barrier: Input/Output/DFG are transparent and just forward edges (the edge into the DFG feeds directly to the corresponding output edge(s) from the Input, only). If you want barriers, add them explicitly.

Which is why this is "breaking", but I'm not sure (need to make certain about) to what extent the spec actually says the former (and needs updating), or whether the spec is unclear (needs changing, i.e. this is a clarification), or whether this is actually a pure "fix"

Either way, the spec does need updating to make it clear what an order edge to or from a container node means. I must confess I don't really know what it means if the container node is not atomic with respect to order. (Even if it wasn't made explicit I had always assumed this.)