Port the full VF2Layout pass to Rust #14056
Conversation
This commit provides a full path Rust implementation of the VF2Layout pass that performs the full pass logic in Rust. We had most of the pass in Rust already by leveraging rustworkx for its vf2 implementation and the scoring logic in Rust. So all that we did in Python was create the graphs as inputs to vf2, building the error map and other rust view objects (since the scoring in Rust predates our rust data model in Qiskit) and then process each found layout for final output. This means the performance improvement expected by this commit is not large since most of the heavy lifting was already in Rust. Instead what this really enables is running the pass in a Python-free context, mainly in a C API for executing this pass which will come in a follow up. This rust implementation does not cover the full feature set of what the Python implementation offers. Mainly in the case of no target, coupling graph randomization (which is a shame we forgot to remove in 2.0, since it almost always produces worse results), or a user provided error map we rely on the existing Python implementation. This is a tradeoff because these use cases are non-standard and having a Rust interface for running in that mode provides little benefit for the interfaces we're building. Fixes Qiskit#12277
mtreinish
added
on hold
|
One or more of the following people are relevant to this code:
|
This commit address some of the panics and failures seen in scheduling tests. The source of the errors was two fold, the first was the layout definition coming from the mapping is sparse but we were using nlayout which doesn't handle that well so we ended up with several sentinel values persiting to scoring when scoring the layout which caused errors. The second issue was we were missing the special handling of free qubits that was added as a performance optimization in Qiskit#9148 which was causing other failures since some of the code was expecting that.
This commit fixes several issues all stemming from the same root cause. There are multiple mappings of indices to qubits to keep track of and the previous commits in this PR branch were not leveraging the type system to keep these sane. This resulted in getting the mapping between representations incorrect at various points that was causing failures and/or panics when the mappings were not resolved correctly. This commit fixes the issue by moving to a typed representation of the layout as soon as posible. This forces all the places that reference a potential layout to reason about the various mappings correctly.
Now that the pass runs in rust we don't emit python logs for the running of the pass. This commit removes the test log assertions since they're no longer present.
This commit fixes the logic in the free qubit mapping and also skips running vf2 if there are no qubits in the interaction graph and there are free qubits.
Pull Request Test Coverage Report for Build 14313701929Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
The earlier approaches were querying the target for each gate while iterating over all the trials. This ends up doing far more hashmap lookups (2x for each gate on a node or edge for each layout) this ends up dominating the runtime. This also fixes an inconsistency in how the scoring worked because we weren't correctly accounting for the number of interactions in the circuit.
This commit fixes a bug that slipped in during an earlier fix where the sort order for the free qubits mapping was accidentally flipped. The intent is to put the lowest error free qubit at the end of the vec so it's the first popped from the stack when mapping it to a qubit with solely single qubit operations. However the comparison got reversed accidentally and that was causing the highest error rate qubits to be used instead. This commit fixes this oversight and fixes the one fialing visualization test that was using the wrong qubit compared to the expected one.
|
The one test that is failing: I think what is happening is that there is a |
|
The performance here is looking pretty good without any real tuning, especially considering most of the heavy lifting was in rust already, |
mtreinish
changed the title
|
I took this out of WIP because everything is working now and I think it's ready for review. There is still some internal cleanup to the code I think we can do and I'll work on this but I don't want to block on internal code organization because I expect there will be a long tail of changes built on top of this (vf2 post layout, other optimizations, etc). |
This commit ports the disjoint layout utils into rust. The eventual goal of this is to enable porting the full DenseLayout and SabreLayout pass to rust so it's a standalone function. This will enable us exposing these passes as a C function. Since the eventual goal of this functionality is to expose it as a C function this attempts to do as much as possible without a py token. A standalone rust entrypoint is added which will serve as what is called from the standalone rust passes in the future, this is then wrapped in a python entrypoint. One minor note is the visibility of the run pass over components entrypoint is `pub(crate)`, but the intent is for it to be `pub`. This visibility was limited here to avoid changing the target's visbility level (which is `pub(crate)` for some reason). This is already being fixed in Qiskit#14056 and when that merges we can update this PR.
There was a problem hiding this comment.
I have spent quite a bit of time looking at the code, and it makes good sense to me. The only thing that I still don't fully understand is that the free qubits are treated a bit differently in the directed and the undirected cases. Could you please explain the reason for that?
| fn build_average_error_map(target: &Target) -> ErrorMap { | ||
| let qargs_count = target.qargs().unwrap().count(); | ||
| let mut error_map = ErrorMap::new(Some(qargs_count)); | ||
| for qargs in target.qargs().unwrap().flatten() { |
There was a problem hiding this comment.
Is it always safe to call target.qargs().unwrap() here?
There was a problem hiding this comment.
It should be, if .qargs() returns None we handle that before this function is ever called. It's done in build_coupling_map() at L262 it returns the None with the ? and that short circuit returns at L371 or L422 (depending on the graph type).
| return Ok(map_free_qubits( | ||
| im_graph_data.free_nodes, | ||
| HashMap::new(), | ||
| &im_graph_data.reverse_im_graph_node_map, | ||
| &avg_error_map, | ||
| target, | ||
| )); |
There was a problem hiding this comment.
Why is map_free_qubits only called in the undirected case?
There was a problem hiding this comment.
In VF2 and VF2Post there are 3 modes of operations with code shared between the two passes.
- Undirected Graph w/ average error rates
- Directed Graph w/ average error rates (VF2Layout only)
- Directed Graph using target error rates (VF2PostLayout only)
in mode 3 we can't use the free qubits handling because the vf2_mapping function is passed a node and edge matcher that validates the target supports the circuit's operations on a virtual qubit. This is required because in that mode we validate that the new layout is valid per the target and we don't watch vf2 to consider a match if it leaves qubits free that don't support the operations that the free circuit qubits use.
When it comes to mode 2 we could in theory support this optimization but the code doesn't because the directed interaction graph generation is shared between modes 2 and 3. Also mode 2 in general is a bit weird and we don't ever use it by default. So I didn't implement it here because I didn't want to change the behavior from Python for this PR.
There was a problem hiding this comment.
Fwiw, in my follow-on patch series, I have done this free-qubit mapping for variant 2, because it let me deduplicate some logic (though it's not all that hard to reinstate it).
There was a problem hiding this comment.
Thanks @mtreinish for the explanation, seeing the bigger picture definitely helps.
| # We can't use the rust fast path because we have a seed set, or no target so continue with | ||
| # the python path |
There was a problem hiding this comment.
I feel like this is actually possible certainly for the seed, and (with greater annoyance) also for the coupling map, but I wouldn't necessarily bother in this PR now. It'd be good to remove all this extra Python code at some point, though.
There was a problem hiding this comment.
TBH, I was thinking instead it would be better to deprecate these options on the pass and support target only moving forward and no randomization moving forward. For VF2Layout I could see the argument for maybe still supporting coupling map but for VF2PostLayout it doesn't really make any sense.
I only wanted to concentrate on the common path that we use in the preset pass managers for this PR though.
There was a problem hiding this comment.
Yeah, I generally agree. My main concern is getting rid of the extra Python code, mostly - it's unpleasant that we're maintaining two separate paths through VF2. I don't feel like we need to do anything about it right now.
|
Btw (for those who aren't Matt, who already knows this): I've been working on a follow-on patch series that goes on top of this PR, and reworks a lot of how we do VF2 from quite a low level. From what I saw of working with it, the Rust-space code here is a faithful port of the Python-space code, and while there's potentially ways to reduce some duplication in the constructions of the interactions and couplings, what's here works fine. |
There was a problem hiding this comment.
Thanks @mtreinish for fully porting the VF2Layout pass to Rust, I completely agree with @jakelishman that this is a faithful port of the Python code, approving!
Summary
This commit provides a full path Rust implementation of the VF2Layout pass that performs the full pass logic in Rust. We had most of the pass in Rust already by leveraging rustworkx for its vf2 implementation and the scoring logic in Rust. So all that we did in Python was create the graphs as inputs to vf2, building the error map and other rust view objects (since the scoring in Rust predates our rust data model in Qiskit) and then process each found layout for final output. This means the performance improvement expected by this commit is not large since most of the heavy lifting was already in Rust. Instead what this really enables is running the pass in a Python-free context, mainly in a C API for executing this pass which will come in a follow up.
This rust implementation does not cover the full feature set of what the Python implementation offers. Mainly in the case of no target, coupling graph randomization (which is a shame we forgot to remove in 2.0, since it almost always produces worse results), or a user provided error map we rely on the existing Python implementation. This is a tradeoff because these use cases are non-standard and having a Rust interface for running in that mode provides little benefit for the interfaces we're building.
Details and comments
Fixes #12277
TODO: