[Oxidize BasisTranslator]: Move basis_search and BasisSearchVisitor to rust.
#12811
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
raynelfss
added
on hold
|
One or more of the following people are relevant to this code:
|
|
Although the performance gains are very niche, here are some benchmarks: All benchmarks:
| Change | Before [ffead590] <move-target~1^2> | After [b6d36cf1] <move-basis-search-and-visit> | Ratio | Benchmark (Parameter) |
|----------|---------------------------------------|--------------------------------------------------|---------|-----------------------------------------------------------------------------------------------------------|
| | 8.20±0s | 6.84±0.1s | ~0.83 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread([0, 1]) |
| | 8.19±0s | 6.70±0.02s | ~0.82 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile([0, 1]) |
| | 111±0.3ms | 113±0.4ms | 1.02 | quantum_info.RandomCliffordBench.time_random_clifford('4,1500') |
| | 126±0.8ms | 126±0.3ms | 1.00 | quantum_info.RandomCliffordBench.time_random_clifford('3,2000') |
| | 104±0.2ms | 104±0.2ms | 1.00 | quantum_info.RandomCliffordBench.time_random_clifford('5,1000') |
| | 72.8±0.4ms | 72.9±0.03ms | 1.00 | quantum_info.RandomCnotDihedralBench.time_random_cnotdihedral('1,2000') |
| | 46.1±0.06ms | 46.2±0.2ms | 1.00 | quantum_info.RandomCnotDihedralBench.time_random_cnotdihedral('3,1200') |
| | 39.5±0.07ms | 39.3±0.04ms | 1.00 | quantum_info.RandomCnotDihedralBench.time_random_cnotdihedral('4,1000') |
| | 32.3±0.04ms | 32.3±0.2ms | 1.00 | quantum_info.RandomCnotDihedralBench.time_random_cnotdihedral('5,800') |
| | 29.5±0.07ms | 29.3±0.4ms | 1.00 | quantum_info.RandomCnotDihedralBench.time_random_cnotdihedral('6,700') |
| | 17.9±0.4ms | 18.0±0.2ms | 1.00 | random_circuit_hex.BenchRandomCircuitHex.time_ibmq_backend_transpile(10) |
| | 41 | 41 | 1.00 | random_circuit_hex.BenchRandomCircuitHex.track_depth_ibmq_backend_transpile(10) |
| | 49 | 49 | 1.00 | random_circuit_hex.BenchRandomCircuitHex.track_depth_ibmq_backend_transpile(12) |
| | 169 | 169 | 1.00 | random_circuit_hex.BenchRandomCircuitHex.track_depth_ibmq_backend_transpile(14) |
| | 17 | 17 | 1.00 | random_circuit_hex.BenchRandomCircuitHex.track_depth_ibmq_backend_transpile(4) |
| | 25 | 25 | 1.00 | random_circuit_hex.BenchRandomCircuitHex.track_depth_ibmq_backend_transpile(6) |
| | 33 | 33 | 1.00 | random_circuit_hex.BenchRandomCircuitHex.track_depth_ibmq_backend_transpile(8) |
| | 144±0.3ms | 143±0.7ms | 0.99 | quantum_info.RandomCliffordBench.time_random_clifford('1,3000') |
| | 133±1ms | 132±0.5ms | 0.99 | quantum_info.RandomCliffordBench.time_random_clifford('2,2500') |
| | 80.5±0.5ms | 79.8±0.7ms | 0.99 | quantum_info.RandomCliffordBench.time_random_clifford('6,700') |
| | 56.6±0.2ms | 56.2±0.07ms | 0.99 | quantum_info.RandomCnotDihedralBench.time_random_cnotdihedral('2,1500') |
| | 24.4±0.2ms | 24.2±0.3ms | 0.99 | random_circuit_hex.BenchRandomCircuitHex.time_ibmq_backend_transpile(12) |
| | 37.8±0.4ms | 37.3±0.2ms | 0.99 | random_circuit_hex.BenchRandomCircuitHex.time_ibmq_backend_transpile(14) |
| | 8.72±0.1ms | 8.55±0.2ms | 0.98 | random_circuit_hex.BenchRandomCircuitHex.time_ibmq_backend_transpile(6) |
| | 13.2±0.2ms | 12.9±0.07ms | 0.98 | random_circuit_hex.BenchRandomCircuitHex.time_ibmq_backend_transpile(8) |
| | 5.93±0.1ms | 5.63±0.1ms | 0.95 | random_circuit_hex.BenchRandomCircuitHex.time_ibmq_backend_transpile(4) |
| - | 4.20±0s | 3.25±0s | 0.77 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile([0]) |
| - | 4.21±0s | 3.24±0s | 0.77 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread([0]) |
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.These changes seem to perform best without multithreading as serializing objects from rust to python is a bit complex at times. |
raynelfss
force-pushed
the
move-basis-search-and-visit
branch
from
78b5f1a
to
e7af19d
Compare
Pull Request Test Coverage Report for Build 11224488724Warning: 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 |
raynelfss
force-pushed
the
move-basis-search-and-visit
branch
4 times, most recently
from
bf84dc0
to
2091413
Compare
raynelfss
force-pushed
the
move-basis-search-and-visit
branch
2 times, most recently
from
e03ef64
to
6972a74
Compare
raynelfss
force-pushed
the
move-basis-search-and-visit
branch
2 times, most recently
from
0b25327
to
d3309a0
Compare
raynelfss
force-pushed
the
move-basis-search-and-visit
branch
3 times, most recently
from
9674018
to
6e2f011
Compare
3 tasks
- Add rust counterpart for `basis_search`. - Consolidated the `BasisSearchVisitor` into the function due to differences in rust behavior.
- Due to the nature of `hashbrown` we must use owned Strings instead of `&str`.
- Remove import of `random` in `basis_translator`.
raynelfss
force-pushed
the
move-basis-search-and-visit
branch
from
6e2f011
to
d96de8a
Compare
raynelfss
added
Changelog: None
raynelfss
added a commit
to raynelfss/qiskit
that referenced
this pull request
Sep 27, 2024
Fixes Port `BasisTranslator` to Rust Qiskit#12246 This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.) Methodology: The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (Qiskit#12292), `EquivalenceLibrary`(Qiskit#12585) and the `BasisTranslator` methods `basis_search` (Qiskit#12811) and `compose_transforms`(Qiskit#13137). All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design. By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made. Changes: - Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation. - Convert the `target_basis` into a set manually from python before sending it into the Rust space. - Remove the exposure of `basis_search` and `compose_transforms` to python. - Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance. - Change inner method's visibility for `basis_search` and `compose_transform` modules in rust. - Expose the exception imports from `Target` to the `accelerate` crate. - Expose `DAGCircuit::copy_empty_like` to the rest of the crates. - Remove all of the unused imports in the Python-side `BasisTranslator`. Blockers: - [ ] Qiskit#12811
1 task
mtreinish
reviewed
Oct 4, 2024
There was a problem hiding this comment.
Overall this LGTM, thanks for doing this. I left a few minor comments inline but overall this is a pretty straightforward porting of the Python search algorithm into Rust. I'm just glad we had that rust search interface in rustworkx-core as it makes this quite simply to move into rust.
| source: &mut HashMap<usize, usize>, | ||
| ) { | ||
| let mut save_index = usize::MAX; | ||
| for edge_data in graph.edge_weights().flatten() { |
There was a problem hiding this comment.
Maybe leave a comment here that flatten() is for iterating over defined edges and skipping any set with None. It took me a second to figure out what this was doing, it's a clever workaround though (Option<T> implementing Iterator is always confusing to me at first).
- Rename `EquivalenceLibrary`'s `mut_graph` method to `graph_mut` to keep consistent with rust naming conventions. - Use `&HashSet<String>` instead of `HashSet<&str>` to avoid extra conversion. - Use `u32::MAX` as num_qubits for dummy node. - Use for loop instead of foreachj to add edges to dummy node. - Add comment explaining usage of flatten in `initialize_num_gates_remain_for_rule`. - Remove stale comments.
mtreinish
previously approved these changes
Oct 7, 2024
There was a problem hiding this comment.
I missed this in my earlier review but the directory structure here is a bit weird relative to the rest of the modules in the crate. The double nesting of basis/basis_translator seems like 1 too many. However, this directory structure is already in use from the compose_transforms() PR. We can clean this up in a follow up PR. Realistically we're going to want to reorganize things in the near future to have a dedicated transpiler pass module or crate.
That all being said I left one nit inline. I'll apply the suggestion and reapprove then enqueue for merge.
mtreinish
approved these changes
Oct 7, 2024
raynelfss
added a commit
to raynelfss/qiskit
that referenced
this pull request
Oct 8, 2024
Fixes Port `BasisTranslator` to Rust Qiskit#12246 This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.) Methodology: The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (Qiskit#12292), `EquivalenceLibrary`(Qiskit#12585) and the `BasisTranslator` methods `basis_search` (Qiskit#12811) and `compose_transforms`(Qiskit#13137). All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design. By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made. Changes: - Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation. - Convert the `target_basis` into a set manually from python before sending it into the Rust space. - Remove the exposure of `basis_search` and `compose_transforms` to python. - Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance. - Change inner method's visibility for `basis_search` and `compose_transform` modules in rust. - Expose the exception imports from `Target` to the `accelerate` crate. - Expose `DAGCircuit::copy_empty_like` to the rest of the crates. - Remove all of the unused imports in the Python-side `BasisTranslator`. Blockers: - [ ] Qiskit#12811
github-merge-queue bot
pushed a commit
that referenced
this pull request
Oct 31, 2024
…Rust. (#13237) * Initial: Move the rest of the `BasisTranslator` to Rust. Fixes Port `BasisTranslator` to Rust #12246 This is the final act of the efforts to move the `BasisTranslator` transpiler pass into Rust. With many of the parts of this pass already living in Rust, the following commits attempt to bring in the final changes to allow complete operation of this pass in the Rust space (with of course some interaction with Python.) Methodology: The way this works is by keeping the original `BasisTranslator` python class, and have it store the rust-space counterpart (now called `CoreBasisTranslator`) which will perform all of the operations leveraging the existent Rust API's available for the `Target` (#12292), `EquivalenceLibrary`(#12585) and the `BasisTranslator` methods `basis_search` (#12811) and `compose_transforms`(#13137). All of the inner methods will have private visibility and will not be accessible to `Python` as they're intended to be internal by design. By removing the extra layers of conversion we should be seeing a considerable speed-up, alongside all of the other incremental improvements we have made. Changes: - Add the pyo3 class/struct `BasisTranslator` that will contain allof the main data used by the transpiler pass to perform its operation. - Convert the `target_basis` into a set manually from python before sending it into the Rust space. - Remove the exposure of `basis_search` and `compose_transforms` to python. - Change `basis_search` so that it accepts references to `HashSet` instances instead of accepting a `HashSet<&str>` instance. - Change inner method's visibility for `basis_search` and `compose_transform` modules in rust. - Expose the exception imports from `Target` to the `accelerate` crate. - Expose `DAGCircuit::copy_empty_like` to the rest of the crates. - Remove all of the unused imports in the Python-side `BasisTranslator`. Blockers: - [ ] #12811 * Fix: Redundancies with serialization methods - Remove extra copies of `target`, `target_basis`, `equiv_lib`, and `min_qubits`. - Remove unnecessary mutability in `apply_transforms` and `replace_node`. * Refactor: Remove `BasisTranslator` struct, use pymethod. - Using this method avoids the creation of a datastructure in rust and the overhead of deserializing rust structures which can be overly slow due to multiple cloning. With this update, since the `BasisTranslator` never mutates, it is better to not store anything in Rust. * Lint: Ignore too_many_args flag from clippy on `basis_translator::run()` * Fix: Remove redundant clone * Fix: Leverage using `unwrap_operation` when taking op_nodes from the dag. - Add function signature in `base_run`. * Update crates/accelerate/src/basis/basis_translator/mod.rs Co-authored-by: Elena Peña Tapia <[email protected]> * Adapt to #13164 - Use `QuantumCircuit._has_calibration_for()` when trying to obtain calibrations from a `QuantumCircuit` due to the deprecation of the `Pulse` package. --------- Co-authored-by: Elena Peña Tapia <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Builds on #12585
The following commits add a rust-side version of
basis_searchthat is also exposed toBasisTranslatorin Python. Due to structural restrictions inrustworkx-core's version ofdijkstra_search, these commits also consolidate thebasis_searchmethod and theBasisSearchVisitorinto one method.Details and comments
The
BasisTranslatoraims to map a circuit's gates into a specified target basis by performing a dijkstra search in a providedEquivalenceLibrary. Because of #12585 moving said structure mainly to rust, these commits aim to add thebasis_searchmethod of this transpiler pass to operate in rust space as well, which could save the overhead of converting the rustEquivalenceLibrary'spetgraph::StableDiGraphto aPyDiGraph.It is worth to note that the following changes have been made:
BasisSearchVisitorand_basis_searchhave been consolidated into one methodbasis_search, due to the visitor's data living in a different space, which would imply moving some of therustworkx-core'sdijkstra_searchretrieved graph data moving outside of its expected lifetimes.basis_seachis an entirely rust-based method. However, it is exposed temporarily to Python viapy_basis_searchto avoid cloning ofsource_basisandtarget_basissets. This exposure however will be removed once PortBasisTranslatorto Rust #12246 is fully realized.Known issues:
When running the testtest.python.transpiler.test_basis_translator.TestBasisExamples.test_cx_bell_to_ecrthere's a posibility that the resulting circuit will randomly be:┌────────────┐ ┌─────────────┐┌──────┐┌──────────┐ q_0: ───┤ U(π/2,0,π) ├───┤ U(0,0,-π/2) ├┤0 ├┤ U(π,0,π) ├ ┌──┴────────────┴──┐└─────────────┘│ Ecr │└──────────┘ q_1: ┤ U(-π/2,-π/2,π/2) ├───────────────┤1 ├──────────── └──────────────────┘ └──────┘or:┌────────────┐ ┌─────────────┐┌──────────┐┌──────┐ q_0: ───┤ U(π/2,0,π) ├──┤ U(0,0,-π/2) ├┤ U(π,0,0) ├┤0 ├ ┌──┴────────────┴─┐└─────────────┘└──────────┘│ Ecr │ q_1: ┤ U(π/2,-π/2,π/2) ├───────────────────────────┤1 ├ └─────────────────┘ └──────┘This is suspectedly due toFixed by e7b3cb0rustworkx-corenot being able to decide between these two equivalent alternatives.Blockers:
EquivalenceLibrary#12585This branch will be rebased once each blocker merges. Feel free to leave any comments or review. Thank you 🚀