feat(pytket-decoder): Decode unsupported subgraphs from barriers #1182
Conversation
aborgna-q
changed the title
aborgna-q
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## ab/pytket-circ-ctx #1182 +/- ##
======================================================
- Coverage 79.46% 79.35% -0.11%
======================================================
Files 155 156 +1
Lines 19293 19883 +590
Branches 18203 18793 +590
======================================================
+ Hits 15332 15779 +447
- Misses 3063 3194 +131
- Partials 898 910 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aborgna-q
force-pushed
the
ab/decode-enc-circuits
branch
from
ab376ea to
d49412e
Compare
aborgna-q
force-pushed
the
ab/decode-unsupported-subgraphs
branch
2 times, most recently
from
fa50775 to
5271a8b
Compare
aborgna-q
force-pushed
the
ab/decode-enc-circuits
branch
from
d49412e to
b6b0bb7
Compare
aborgna-q
force-pushed
the
ab/decode-unsupported-subgraphs
branch
from
5271a8b to
48c25f3
Compare
acl-cqc
left a comment
acl-cqc
left a comment
There was a problem hiding this comment.
Appreciate this is a draft, good to see how things go together.
Concerned about linking. As ever, linking is never urgent....until someone needs it, and then it's too far off that we have to put in some specific hack/workaround....
And, really wondering whether in-place would be better. Building a new Hugr obviously is the only option for "standalone"/inline opaque-subgraphs, but this has significant concerns (and requires more data storage, etc., in your earlier PR). Can we get away with restricting the standalone-subgraph version in some way that makes it easy (e.g. here you allow only one opaque-subgraph, that may not be viable) and then do the external-subgraph version separately in place? (well, clone the hugr, obvs!)
Another concern that I guess this raises, is do we really want to be able to mix and match external and standalone in one circuit???
tket/src/serialize/pytket/decoder.rs
Outdated
| OpaqueSubgraphPayloadType::External { id } => { | ||
| match (self.opaque_subgraphs, self.original_hugr) { | ||
| (Some(subgraphs), Some(original_hugr)) if subgraphs.contains(*id) => { | ||
| let hugr = subgraphs[*id].extract_subgraph(original_hugr, id.to_string()); |
There was a problem hiding this comment.
I suspect that this won't work if there are, say, function edges into the opaque subgraph
There was a problem hiding this comment.
Yeah, I was working on this in a WIP branch.
We need to get the subgraph's function_inputs, and deny subgraphs with non-local value edges in the encoder.
tket/src/serialize/pytket/circuit.rs
Outdated
| }; | ||
|
|
||
| let mut decoder = PytketDecoderContext::<H>::new(serial_circuit, hugr, target, options)?; | ||
| decoder.register_opaque_subgraphs(&self.opaque_subgraphs, self.hugr); |
There was a problem hiding this comment.
Would these fit better within the DecodeOptions rather than the Context?
Whilst the boundary between Options and Config is clear (Config is basically static), I'm not clear other what scope you'd reuse an Options. signature and input_params look specific to one pytket circuit, whereas the extension registry is probably specific to an EncodedCircuit (as would the opaque-subgraphs). Does it make sense to establish DecodeOptions as per-EncodedCircuit (moving signature/input-params back out to Context, and putting opaque-subgraphs stuff in Options)?
| .filter(|c| *c != entrypoint_function) | ||
| .map(|c| (c, module)); | ||
|
|
||
| // Insert the hugr's entrypoint region directly into the region being built, |
There was a problem hiding this comment.
Ok so we need a big TODO here or something, when linking comes along that'll do the lifting here, moreover this approach will duplicate the non-entrypoint functions for each subgraph (when there are multiple subgraphs), which would be invalid if any were public. (Also, hang on, if it's an external subgraph, any such functions won't be in the opaque_hugr anyway as that was called by extract_subgraph....)
It'd be great if we could hold off doing this a bit longer (we inline the qubit taking functions first anyway right) and use linking the first time...
One approach would be to record which source (opaque_hugr) functions had gone to what Node in the target Hugr, and then use link_by_node to use the copies already made in the new Hugr.
But this is also a place where, if we're using External subgraphs, doing this in-place on a single "host" hugr would work much better (then the things you insert are only nodes straight from pytket), and maybe it makes sense to only try to give decent results (/support multiple subcircuits, etc.) in that situation.
There was a problem hiding this comment.
Ok, thinking about this a bit more.
- When you are using standalone/inline Hugrs,
insert_forestis probably appropriate: we have to add the whole of the standalone Hugr, including any module-level functions; and there is no sharing between them. In the future we can switchinsert_foresttoinsert_link_hugr(taking shape in feat: (WIP) Addinsert_link_hugrto add entrypoint subtree AND link hugr#2555) to allow sharing of public functions.- Alternatively we can use
link_by_nodewithNodeLinkingDirective::add()for each toplevel, that has the same effect.
- Alternatively we can use
- When you are using external Hugrs, we should use
link_by_node...- The first time we insert a subgraph of the original host hugr, add all the toplevel functions (as you are doing with
insert_forest), but record the node in the target (/new/being-constructed) Hugr to which each function in the source corresponds. - On subsequent calls, add all the functions in the source Hugr but with
NodeLinkingDirective::UseExistingof the node recorded the first time.
- The first time we insert a subgraph of the original host hugr, add all the toplevel functions (as you are doing with
That'll keep all the right links but avoid duplicating functions (in fact, it's better than the still-to-come linking-by-name routines as it'll avoid duplicating even private functions).
However note this means that you can't have a method on UnsupportedSubgraph returning the Hugr to insert (also you can't use extract_subgraph). You'll have to either return a Hugr and a map of NodeLinkingDirectives, or have a method to perform insertion of the UnsupportedSubgraph into the target.
| /// Helper to compute the expected register counts before generating a [`PytketDecodeErrorInner::UnexpectedNodeOutput`] error | ||
| /// when registering the outputs of an unsupported subgraph. | ||
| fn make_unexpected_node_out_error<'ty>( | ||
| config: &PytketDecoderConfig<impl HugrView>, |
There was a problem hiding this comment.
could this be a method on PytketDecoderConfig ??
There was a problem hiding this comment.
It's a bit specific to this context. The main utility is exhausting the iterator of types we had in the function to count how many bits/qubits we were expecting on the output.
There's a similarly named function in decoder.rs, for a function were the types iterator is partially consumed, so we need to adds things to a partial accumulator.
Again, it's quite specific to its context.
I don't think this will be called from anywhere else.
| )?; | ||
| } | ||
| None => { | ||
| // This is an unsupported wire, so we just register the edge id to the wire. |
There was a problem hiding this comment.
Ok, so when we have multiple opaque subgraphs...we will need to process those in topsort order (sources of unsupported edges before targets)...
There was a problem hiding this comment.
Added some logic to handle this. If the outgoing port doesn't exist yet we keep track of the targets and connect them later.
aborgna-q
changed the base branch from
ab/decode-enc-circuits
to
ab/pytket-circ-ctx
aborgna-q
force-pushed
the
ab/decode-unsupported-subgraphs
branch
from
7430bbd to
abd2a91
Compare
aborgna-q
force-pushed
the
ab/decode-unsupported-subgraphs
branch
from
7118a1d to
003b830
Compare
aborgna-q
force-pushed
the
ab/decode-unsupported-subgraphs
branch
from
ad2a6c0 to
c921d3a
Compare
aborgna-q
force-pushed
the
ab/decode-unsupported-subgraphs
branch
from
6be9967 to
d49f38f
Compare
aborgna-q
force-pushed
the
ab/decode-unsupported-subgraphs
branch
from
d49f38f to
58819ea
Compare
|
Closing this and creating a single PR instead |
…1211) For previous reviews, see - #1164 - #1182 Comments from those have ben addressed already. This PR adds an `EncoderCircuit` in between the encoding/decoding process `Hugr <-> tket_json_rs::SerialCircuit`. This new instruction carries multiple `SerialCircuits` associated with the Hugr region they encode. This lets us replace the old regions with modified ones in the original hugr, after optimising the SerialCircuits. The opaque barriers that represent sections of the original hugr not encodable as pytket commands now have two payload variants: - `OpaqueSubgraphPayload::Inline` as before encodes the unsupported hugr envelope, plus some boundary edge ids that we can use to reconstruct links between different inline payloads. This variant does not support all circuits. Subgraphs must be flat and contain no non-local edges (just as before, but now we detect and error out on those cases). - `OpaqueSubgraphPayload::External`. This references a subgraph in the original hugr with a simple ID. This allows us to re-use the nodes from the initial hugr, keeping all the hierarchy and edges. The second case only works for `Hugr`s due to `HugrBuilder` limitations, but it's the usecase we'll use for tket1 optimisation. The first one is kept for cases where we need to extract standalone pytket circuits. For testing, this includes - Several new roundtrip tests in `serialize::pytket::tests`, including error tests - An integration test in `tket1-passes` that encodes a flat hugr, applies a pass from pytket, and reassembles it inline. BREAKING CHANGE: `OpConvertError` renamed to `PytketEncodeOpError` BREAKING CHANGE: Some minor changes to `PytketDecodeError` variant attributes. BREAKING CHANGE: `fn_name` moved from `DecodeOptions` to `DecodeInsertionTarget::Function`
Re-inserts the unsupported subgraphs into the new HUGR when decoding a pytket circuit.
As of this draft creation, this should work correctly when decoding single pytket circuits (no nested circuits inside unsupported regions) that only have flat unsupported regions (with no nested regions nor external function calls).
Depends on