-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Add support for disjoint coupling maps to SabreLayout #9802
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
Add support for disjoint coupling maps to SabreLayout #9802
Conversation
Thank you for opening a new pull request. Before your PR can be merged it will first need to pass continuous integration tests and be reviewed. Sometimes the review process can be slow, so please be patient. While you're waiting, please feel free to review other open PRs. While only a subset of people are authorized to approve pull requests for merging, everyone is encouraged to review open pull requests. Doing reviews helps reduce the burden on the core team and helps make the project's code better for everyone. One or more of the the following people are requested to review this:
|
Pull Request Test Coverage Report for Build 4692536440
💛 - Coveralls |
c03da87
to
f0a498e
Compare
other_node = uuid_map[node.op.label] | ||
num_qubits = len(other_node.qargs) + len(node.qargs) | ||
new_op = Barrier(num_qubits, label=barrier_uuid) | ||
new_node = dag.replace_block_with_op([node, other_node], new_op, qubit_indices) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am potentially worried about this potentially introducing a cycle when combining a barrier across qubits. I think it's possible this could cause that which is why I left the cycle check enabled for this. But we should try to confirm this before merging.
I added some full path transpile() tests in 5631d96 This gives a good example to show this in practice. Look at the 3 component circuit test, the input circuit is: when running the output circuit looks like: |
For the tests I just added I have to tag them as |
5631d96
to
ed32409
Compare
connected_components = rx.weakly_connected_components(im_graph) | ||
component_qubits = [] | ||
for component in connected_components: | ||
component_qubits.append(set(qubit_map[x] for x in component)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A potential future optimization that I'm thinking of doing after this and #9840 merge is to skip the intermediate dag creation here, and go from connected qubits straight to a list of SabreDAGs. This should reduce the number of dag iterations we do and speed things up. But for the first iteration of this I'd like to keep everything unified on using full dags.
Now that Qiskit#9802 supports shared classical bits correctly this commit re-enables the tests disabled in the previous commit.
This commit adds support to the SabreLayout pass for targeting disjoint CouplingMap objects. The SABRE algorithm is written assuming a connected graph, so to enable targeting a disjoint coupling graph this commit decomposes the circuit into it's weakly connected components and then maps those to the connected components of the coupling map and runs the rust portion of the sabre code on each connected component of the coupling graph. The results for each subgraph of the target backend is then combined the pass is building the output dag. In general the biggest potential issue for output quality right now is the mapping function does not take into account the relative connectivity of the dag component and the coupling map component. The mapping is just done in order and it tries every component until it finds one that has enough qubits. In the future we should use a heuristic to try and pack the components based on what we expect will require the least amount of swaps, but we can attempt that in a future PR.
This commit fixes the handling of the barrier directive as the input DAG is split across multiple components. It is entirely valid for an input dag to have a multi-qubit barrier that spans multiple connected components. However, such barriers shouldn't be treated as multi-qubit operation for computing the connected components (as this result in the components being treated larger than they otherwise would be). To handle this edge case the logic introduced into this commit is prior to computing the connected components in the input dag we split each multi-qubit barrier into n single qubit barriers and assign a UUID label to each of the single qubit barriers. Then after we create each subgraph DAGCircuit object for each connected component we find all the barriers with a UUID and recombine them into a multi qubit barrier. This will retain the barriers characteristics for each connected component (as if it were in the original disjoint dag). Then in `SabreLayout` after we've built the output mapped dagcircuit we run the combination function one more time to combine the multiqubit dags across the different components, and in that process the UUID labels are removed. It is worth pointing out the downside with this approach is that it precludes preserving barriers with labels through transpile().
This commit fixes the handling of the separate_dag() function when the input DAGCircuit has shared classical bits between quantum connected components. In previous commits on this branch these would incorrectly be treated as a single connected component because from the perspective of rustworkx's weakly_connected_components() function there is no difference between the type of wires when computing the connected components of the graph. This commit fixes this by instead of computing the connected components of the DAG itself we create an interaction graph of just the quantum component and use that to find the connected components of qubits. From that we build subgraphs of the DAGCircuit for each connected component DAGCircuit. This ends up being less efficient, but produces the correct result by ignoring the classical component for computing the connected components.
1af8166
to
1ddcb03
Compare
Co-authored-by: Kevin Hartman <[email protected]>
…oint_coupling_maps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a few more small comments. I'm fine with the PR as is (e.g. if time is of the essence).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Now that Qiskit#9802 supports shared classical bits correctly this commit re-enables the tests disabled in the previous commit.
* Add support for disjoint coupling maps to SabreLayout This commit adds support to the SabreLayout pass for targeting disjoint CouplingMap objects. The SABRE algorithm is written assuming a connected graph, so to enable targeting a disjoint coupling graph this commit decomposes the circuit into it's weakly connected components and then maps those to the connected components of the coupling map and runs the rust portion of the sabre code on each connected component of the coupling graph. The results for each subgraph of the target backend is then combined the pass is building the output dag. In general the biggest potential issue for output quality right now is the mapping function does not take into account the relative connectivity of the dag component and the coupling map component. The mapping is just done in order and it tries every component until it finds one that has enough qubits. In the future we should use a heuristic to try and pack the components based on what we expect will require the least amount of swaps, but we can attempt that in a future PR. * Fix handling of barriers across separating components This commit fixes the handling of the barrier directive as the input DAG is split across multiple components. It is entirely valid for an input dag to have a multi-qubit barrier that spans multiple connected components. However, such barriers shouldn't be treated as multi-qubit operation for computing the connected components (as this result in the components being treated larger than they otherwise would be). To handle this edge case the logic introduced into this commit is prior to computing the connected components in the input dag we split each multi-qubit barrier into n single qubit barriers and assign a UUID label to each of the single qubit barriers. Then after we create each subgraph DAGCircuit object for each connected component we find all the barriers with a UUID and recombine them into a multi qubit barrier. This will retain the barriers characteristics for each connected component (as if it were in the original disjoint dag). Then in `SabreLayout` after we've built the output mapped dagcircuit we run the combination function one more time to combine the multiqubit dags across the different components, and in that process the UUID labels are removed. It is worth pointing out the downside with this approach is that it precludes preserving barriers with labels through transpile(). * Fix compatibility with Python < 3.9 * Adjust sorting order for mapping components * Add full path transpile() tests * Tag six component test as a slow test * Fix handling of splitting dags with shared classical bits This commit fixes the handling of the separate_dag() function when the input DAGCircuit has shared classical bits between quantum connected components. In previous commits on this branch these would incorrectly be treated as a single connected component because from the perspective of rustworkx's weakly_connected_components() function there is no difference between the type of wires when computing the connected components of the graph. This commit fixes this by instead of computing the connected components of the DAG itself we create an interaction graph of just the quantum component and use that to find the connected components of qubits. From that we build subgraphs of the DAGCircuit for each connected component DAGCircuit. This ends up being less efficient, but produces the correct result by ignoring the classical component for computing the connected components. * Add more test coverage and release notes * Don't route in SabreLayout with > 1 layout component with shared clbits When we run SabreLayout normally we're running both the layout and routing stage together. This is done for performance because the rust component is running routing internally so we avoid multiple back and forths between Python and Rust this way and it can lead to noticeable runtime improvements when doing this. However, in the case of > 1 circuit component that have any shared clbits we can not safetly route from the split dag components because the data dependency is lost for the classical component of the circuit is lost between the split components. To do routing without potentially reordering the data dependencies for the classical bits we need to run it on the combined dag all at once. For SabreLayout the easiest way to do that is to just have it work as a normal layout pass and let `SabreSwap` do the routing as it will have the full context and won't cause things to reorder incorrectly. This commit makes the change by checking if we have more than one component and any shared clbits between any components. If we do then we skip routing in SabreLayout and only return layout information. * Apply suggestions from code review Co-authored-by: Kevin Hartman <[email protected]> * Fix return type hint for map_components() * Cleanup variables and set usage in separate_dag * Remove duplicate lines in SabreLayout * Update error message text for routing_pass arg and disjoint cmap Co-authored-by: Kevin Hartman <[email protected]> * Improve docstring for map_components * Add comment explaining dag composition in run_pass_over_connected_components --------- Co-authored-by: Kevin Hartman <[email protected]>
* Add full path transpile() support for disjoint backends This commit finalizes the last piece of the full path transpile() support at all optimization levels. The primary piece to accomplish this was adding DenseLayout support for targeting a disjoint CouplingMap. With this piece added then all the default transpiler paths in the preset pass managers are able to support running with a disjoint CouplingMap. The biggest exception is the TrivialLayout pass which can result in an invalid layout being selected (where 2q connectivity crosses connected components). To handle this edge case a check is added to each routing stage to ensure the selected layout is valid for the coupling map and it is routable. This enables all the default paths at all 4 optimization levels for transpile() to be usable with disjoint connectivity. For optimization_level=0/TrivialLayout if the trivial layout is invalid the routing pass will raise an error saying as much. It's worth pointing out that NoiseAdaptiveLayout still doesn't support disjoint connectivity. However, since it's performance is poor and it's not used by default in any preset passmanager we can add this at a later date, but all other combinations of layout and routing methods should work (given the caveats with TrivialLayout) above. * Fix test determinism * Add future facing test for shared classical bits between components * Enable shared control flow test Now that #9802 supports shared classical bits correctly this commit re-enables the tests disabled in the previous commit. * Remove unused condition for faulty qubits backnedv1 path * Remove opt level 1 variant of test_chained_data_dependency * Use enumerate() in check_layout_isolated_to_component() Co-authored-by: Kevin Hartman <[email protected]> * Expand level 0 test coverage * Update test/python/compiler/test_transpiler.py Co-authored-by: Kevin Hartman <[email protected]> * s/check_layout_isolated_to_component/require_layout_isolated_to_component/g --------- Co-authored-by: Kevin Hartman <[email protected]>
* Add full path transpile() support for disjoint backends This commit finalizes the last piece of the full path transpile() support at all optimization levels. The primary piece to accomplish this was adding DenseLayout support for targeting a disjoint CouplingMap. With this piece added then all the default transpiler paths in the preset pass managers are able to support running with a disjoint CouplingMap. The biggest exception is the TrivialLayout pass which can result in an invalid layout being selected (where 2q connectivity crosses connected components). To handle this edge case a check is added to each routing stage to ensure the selected layout is valid for the coupling map and it is routable. This enables all the default paths at all 4 optimization levels for transpile() to be usable with disjoint connectivity. For optimization_level=0/TrivialLayout if the trivial layout is invalid the routing pass will raise an error saying as much. It's worth pointing out that NoiseAdaptiveLayout still doesn't support disjoint connectivity. However, since it's performance is poor and it's not used by default in any preset passmanager we can add this at a later date, but all other combinations of layout and routing methods should work (given the caveats with TrivialLayout) above. * Fix test determinism * Add future facing test for shared classical bits between components * Enable shared control flow test Now that Qiskit#9802 supports shared classical bits correctly this commit re-enables the tests disabled in the previous commit. * Remove unused condition for faulty qubits backnedv1 path * Remove opt level 1 variant of test_chained_data_dependency * Use enumerate() in check_layout_isolated_to_component() Co-authored-by: Kevin Hartman <[email protected]> * Expand level 0 test coverage * Update test/python/compiler/test_transpiler.py Co-authored-by: Kevin Hartman <[email protected]> * s/check_layout_isolated_to_component/require_layout_isolated_to_component/g --------- Co-authored-by: Kevin Hartman <[email protected]>
This commit fixes an issue in the `SabreLayout` pass when run on backends with a disjoint connectivity graph. The `SabreLayout` pass internally runs routing as part of its operation and as a performance efficiency we use that routing output by default. However, in the case of a disjoint coupling map we are splitting the circuit up into different subcircuits to map that to the connected components on the backend. Doing final routing as part of that split context is no sound because depsite the quantum operations being isolated to each component, other operations could have a dependency between the components. This was previously discovered during the development of Qiskit#9802 to be the case with classical data/bits, but since merging that it also has come up with barriers. To prevent any issues in the future this commit changes the SabreLayout pass to never use the routing output during the layout search when there is >1 connected component because we *need* to rerun routing on the full circuit after applying the layout. Fixes Qiskit#9995
* Don't run routing in SabreLayout with split components This commit fixes an issue in the `SabreLayout` pass when run on backends with a disjoint connectivity graph. The `SabreLayout` pass internally runs routing as part of its operation and as a performance efficiency we use that routing output by default. However, in the case of a disjoint coupling map we are splitting the circuit up into different subcircuits to map that to the connected components on the backend. Doing final routing as part of that split context is no sound because depsite the quantum operations being isolated to each component, other operations could have a dependency between the components. This was previously discovered during the development of #9802 to be the case with classical data/bits, but since merging that it also has come up with barriers. To prevent any issues in the future this commit changes the SabreLayout pass to never use the routing output during the layout search when there is >1 connected component because we *need* to rerun routing on the full circuit after applying the layout. Fixes #9995 * Remove unused shared_clbits logic * Remove swap and routing assertions from disjoint sabre layout tests * Fix lint
* Add support for disjoint coupling maps to SabreLayout This commit adds support to the SabreLayout pass for targeting disjoint CouplingMap objects. The SABRE algorithm is written assuming a connected graph, so to enable targeting a disjoint coupling graph this commit decomposes the circuit into it's weakly connected components and then maps those to the connected components of the coupling map and runs the rust portion of the sabre code on each connected component of the coupling graph. The results for each subgraph of the target backend is then combined the pass is building the output dag. In general the biggest potential issue for output quality right now is the mapping function does not take into account the relative connectivity of the dag component and the coupling map component. The mapping is just done in order and it tries every component until it finds one that has enough qubits. In the future we should use a heuristic to try and pack the components based on what we expect will require the least amount of swaps, but we can attempt that in a future PR. * Fix handling of barriers across separating components This commit fixes the handling of the barrier directive as the input DAG is split across multiple components. It is entirely valid for an input dag to have a multi-qubit barrier that spans multiple connected components. However, such barriers shouldn't be treated as multi-qubit operation for computing the connected components (as this result in the components being treated larger than they otherwise would be). To handle this edge case the logic introduced into this commit is prior to computing the connected components in the input dag we split each multi-qubit barrier into n single qubit barriers and assign a UUID label to each of the single qubit barriers. Then after we create each subgraph DAGCircuit object for each connected component we find all the barriers with a UUID and recombine them into a multi qubit barrier. This will retain the barriers characteristics for each connected component (as if it were in the original disjoint dag). Then in `SabreLayout` after we've built the output mapped dagcircuit we run the combination function one more time to combine the multiqubit dags across the different components, and in that process the UUID labels are removed. It is worth pointing out the downside with this approach is that it precludes preserving barriers with labels through transpile(). * Fix compatibility with Python < 3.9 * Adjust sorting order for mapping components * Add full path transpile() tests * Tag six component test as a slow test * Fix handling of splitting dags with shared classical bits This commit fixes the handling of the separate_dag() function when the input DAGCircuit has shared classical bits between quantum connected components. In previous commits on this branch these would incorrectly be treated as a single connected component because from the perspective of rustworkx's weakly_connected_components() function there is no difference between the type of wires when computing the connected components of the graph. This commit fixes this by instead of computing the connected components of the DAG itself we create an interaction graph of just the quantum component and use that to find the connected components of qubits. From that we build subgraphs of the DAGCircuit for each connected component DAGCircuit. This ends up being less efficient, but produces the correct result by ignoring the classical component for computing the connected components. * Add more test coverage and release notes * Don't route in SabreLayout with > 1 layout component with shared clbits When we run SabreLayout normally we're running both the layout and routing stage together. This is done for performance because the rust component is running routing internally so we avoid multiple back and forths between Python and Rust this way and it can lead to noticeable runtime improvements when doing this. However, in the case of > 1 circuit component that have any shared clbits we can not safetly route from the split dag components because the data dependency is lost for the classical component of the circuit is lost between the split components. To do routing without potentially reordering the data dependencies for the classical bits we need to run it on the combined dag all at once. For SabreLayout the easiest way to do that is to just have it work as a normal layout pass and let `SabreSwap` do the routing as it will have the full context and won't cause things to reorder incorrectly. This commit makes the change by checking if we have more than one component and any shared clbits between any components. If we do then we skip routing in SabreLayout and only return layout information. * Apply suggestions from code review Co-authored-by: Kevin Hartman <[email protected]> * Fix return type hint for map_components() * Cleanup variables and set usage in separate_dag * Remove duplicate lines in SabreLayout * Update error message text for routing_pass arg and disjoint cmap Co-authored-by: Kevin Hartman <[email protected]> * Improve docstring for map_components * Add comment explaining dag composition in run_pass_over_connected_components --------- Co-authored-by: Kevin Hartman <[email protected]>
* Add full path transpile() support for disjoint backends This commit finalizes the last piece of the full path transpile() support at all optimization levels. The primary piece to accomplish this was adding DenseLayout support for targeting a disjoint CouplingMap. With this piece added then all the default transpiler paths in the preset pass managers are able to support running with a disjoint CouplingMap. The biggest exception is the TrivialLayout pass which can result in an invalid layout being selected (where 2q connectivity crosses connected components). To handle this edge case a check is added to each routing stage to ensure the selected layout is valid for the coupling map and it is routable. This enables all the default paths at all 4 optimization levels for transpile() to be usable with disjoint connectivity. For optimization_level=0/TrivialLayout if the trivial layout is invalid the routing pass will raise an error saying as much. It's worth pointing out that NoiseAdaptiveLayout still doesn't support disjoint connectivity. However, since it's performance is poor and it's not used by default in any preset passmanager we can add this at a later date, but all other combinations of layout and routing methods should work (given the caveats with TrivialLayout) above. * Fix test determinism * Add future facing test for shared classical bits between components * Enable shared control flow test Now that Qiskit#9802 supports shared classical bits correctly this commit re-enables the tests disabled in the previous commit. * Remove unused condition for faulty qubits backnedv1 path * Remove opt level 1 variant of test_chained_data_dependency * Use enumerate() in check_layout_isolated_to_component() Co-authored-by: Kevin Hartman <[email protected]> * Expand level 0 test coverage * Update test/python/compiler/test_transpiler.py Co-authored-by: Kevin Hartman <[email protected]> * s/check_layout_isolated_to_component/require_layout_isolated_to_component/g --------- Co-authored-by: Kevin Hartman <[email protected]>
* Don't run routing in SabreLayout with split components This commit fixes an issue in the `SabreLayout` pass when run on backends with a disjoint connectivity graph. The `SabreLayout` pass internally runs routing as part of its operation and as a performance efficiency we use that routing output by default. However, in the case of a disjoint coupling map we are splitting the circuit up into different subcircuits to map that to the connected components on the backend. Doing final routing as part of that split context is no sound because depsite the quantum operations being isolated to each component, other operations could have a dependency between the components. This was previously discovered during the development of Qiskit#9802 to be the case with classical data/bits, but since merging that it also has come up with barriers. To prevent any issues in the future this commit changes the SabreLayout pass to never use the routing output during the layout search when there is >1 connected component because we *need* to rerun routing on the full circuit after applying the layout. Fixes Qiskit#9995 * Remove unused shared_clbits logic * Remove swap and routing assertions from disjoint sabre layout tests * Fix lint
Summary
This commit adds support to the SabreLayout pass for targeting disjoint
CouplingMap objects. The SABRE algorithm is written assuming a connected
graph, so to enable targeting a disjoint coupling graph this commit
decomposes the circuit into it's weakly connected components and then
maps those to the connected components of the coupling map and runs
the rust portion of the sabre code on each connected component of the
coupling graph. The results for each subgraph of the target backend is
then combined the pass is building the output dag.
In general the biggest potential issue for output quality right now is
the mapping function does not take into account the relative connectivity
of the dag component and the coupling map component. The mapping is just
done in order and it tries every component until it finds one that has
enough qubits. In the future we should use a heuristic to try and pack
the components based on what we expect will require the least amount of
swaps, but we can attempt that in a future PR.
Details and comments
This is still a very rough draft of this feature. Right now it does not handle barriers correctly at allThis should be fixed by: b485d6abecause an input circuit can have a multi-qubit barrier could correctly span more qubits than in a single
connected component and this will incorrectly treat that as a single dag component resulting in the circuit
being incorrectly mapped (or not being mappable at all). This prevents this from being used in the preset
pass managers at all because we need to insert a multiqubit barrier before the final measurement to prevent
routing past a measurement.
I pushed this up primarily as a way to save my work between systems and share the basic approach while I'mstill developing the feature to fix these edge cases. But in isolation for circuits without barriers (or circuits where
the barrier width is less than or equal to the width of the coupling graph's connected components) this seems to
work correctly.
TODO:
This PR is based on top of #9710 in its current form, to see the contents of what will be this PR you can look at this diff: mtreinish/qiskit-core@disjoint-coupling-map...mtreinish:qiskit-core:layout_and_route_disjoint_coupling_maps