[Oxidize BasisTranslator] Add rust-native compose_transforms()
#13137
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
- Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation. - Remove panic statement when checking for example gates. - Use qreg when adding an instruction to the mock_dag in `compose_transforms`. - Add missing comparison check in for loop to compare the mapped instructions. - Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract. - Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`.
- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.
|
One or more of the following people are relevant to this code:
|
raynelfss
added
performance
Changelog: None
Pull Request Test Coverage Report for Build 11073229544Warning: 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 |
|
Here are some benchmarks All benchmarks:
| Change | Before [81063a79] <fix-abi3-issues~1> | After [e8c957df] <rust-compose-trans> | Ratio | Benchmark (Parameter) |
|----------|-----------------------------------------|-----------------------------------------|---------|----------------------------------------------------------------------------------------------------------|
| | 306±1ms | 303±2ms | 0.99 | passes.MultipleBasisPassBenchmarks.time_basis_translator(14, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id']) |
| | 443±3ms | 437±3ms | 0.99 | passes.MultipleBasisPassBenchmarks.time_basis_translator(20, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id']) |
| | 188±0.8ms | 182±2ms | 0.97 | passes.MultipleBasisPassBenchmarks.time_basis_translator(14, 1024, ['rz', 'x', 'sx', 'cx', 'id']) |
| | 217±0.7ms | 207±0.6ms | 0.96 | passes.MultipleBasisPassBenchmarks.time_basis_translator(20, 1024, ['u', 'cx', 'id']) |
| | 102±0.5ms | 98.1±1ms | 0.96 | passes.MultipleBasisPassBenchmarks.time_basis_translator(5, 1024, ['rx', 'ry', 'rz', 'r', 'rxx', 'id']) |
| | 266±4ms | 253±0.5ms | 0.95 | passes.MultipleBasisPassBenchmarks.time_basis_translator(20, 1024, ['rz', 'x', 'sx', 'cx', 'id']) |
| | 155±3ms | 146±0.6ms | 0.94 | passes.MultipleBasisPassBenchmarks.time_basis_translator(14, 1024, ['u', 'cx', 'id']) |
| | 68.6±1ms | 62.6±0.2ms | 0.91 | passes.MultipleBasisPassBenchmarks.time_basis_translator(5, 1024, ['rz', 'x', 'sx', 'cx', 'id']) |
| - | 57.5±0.4ms | 51.8±1ms | 0.9 | passes.MultipleBasisPassBenchmarks.time_basis_translator(5, 1024, ['u', 'cx', 'id']) |
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED.All benchmarks:
| Change | Before [81063a79] <fix-abi3-issues~1> | After [e8c957df] <rust-compose-trans> | Ratio | Benchmark (Parameter) |
|----------|-----------------------------------------|-----------------------------------------|---------|-----------------------------------------------------------------------------------------------------------|
| | 5.91±0s | 5.13±0.01s | ~0.87 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile([0, 1]) |
| | 5.93±0.02s | 5.11±0s | ~0.86 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread([0, 1]) |
| - | 2.53±0.01s | 2.26±0.01s | 0.89 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile([0]) |
| - | 2.56±0.01s | 2.25±0.01s | 0.88 | randomized_benchmarking.RandomizedBenchmarkingBenchmark.time_ibmq_backend_transpile_single_thread([0]) |
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED. |
raynelfss
changed the title
compose_transforms()compose_transforms()
There was a problem hiding this comment.
Looks like a good start, thanks for working on this.
Unfortunately, the Python code you're starting with is probably sorely in need of a rewrite. I'm not entirely familiar with it, but it seems riddled with wrong / bad docs and tuple abuse.
I'll need to do a second pass once you've had a look at the comments thus far. I haven't spent enough time reviewing the parameter mapping stuff, specifically.
| @@ -0,0 +1,21 @@ | |||
| // This code is part of Qiskit. | |||
There was a problem hiding this comment.
Is there a clear benefit to having a separate folder here for basis_translator?
I don't know that I mind it, but it's different from the folder structure we had in Python, and none of the other accelerate modules really go more than one module deep (except for synthesis, which seems to copy the organization of the Python module).
There was a problem hiding this comment.
This is just to plan ahead, since the BasisTranslator has a couple of moving parts and since I am separating things at the moment to make them more readable. If you take a look at #12811, that feature also exists in that same folder. but in a different file If you think it shouldn't be this way it can be changed.
There was a problem hiding this comment.
It suppose it doesn't really matter much, so if you think it helps to organize I'm good with it 🙂.
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
|
|
||
| for (gate_name, gate_num_qubits) in source_basis.iter().cloned() { | ||
| // Need to grab a gate instance to find num_qubits and num_params. | ||
| // Can be removed following https://github.com/Qiskit/qiskit-terra/pull/3947 . |
There was a problem hiding this comment.
This issue appears to be closed. I think the comment may be outdated in your implementation, anyhow?
There was a problem hiding this comment.
Yes, this was a question I had. Not sure how to tackle it since it seems that issue has been abandoned.
There was a problem hiding this comment.
I would probably just remove the comment, and add a method comment to get_example_gates that explains that its purpose is just to get the number of parameters for a gate with a specific name and number of qubits, since that info is only found on an instruction.
(...also, we should probably rename get_example_gates to get_num_params and change the return type to HashMap<(String, u32), usize>. As it is, we don't use the PackedInstructions coming out of the function (do we?) and they are being cloned)
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
| dag.apply_operation_back(placeholder_gate, qr, (), check=False) | ||
| mapped_instrs[gate_name, gate_num_qubits] = placeholder_params, dag | ||
|
|
||
| for gate_name, gate_num_qubits, equiv_params, equiv in basis_transforms: |
There was a problem hiding this comment.
It looks like the documentation is wrong then for basis_transforms in the original Python code, no? Since the tuple does not have the number of bits there.
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
- Remove unused `circuit_to_dag` and `OperationRef` imports. - Streamline complex types into simpler aliases. - Rename `CircuitRep` to `CircuitFromPython`. - Reshape `get_example_gates` to work with `CircuitData` instances during its recursion. - Remove unnecessary clone of `gate_name` in `compose_transform`. - Use `mapped_instructions` values when iterating in `compose_transforms`. - Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust. - Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple.
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
Comment on lines
+152
to
+159
| let op_node = dag.get_node(py, node)?; | ||
| dag.py_substitute_node_with_dag( | ||
| py, | ||
| op_node.bind(py), | ||
| &replace_dag, | ||
| None, | ||
| true, | ||
| )?; |
There was a problem hiding this comment.
I imagine this is slower than it ought to be, but it'd be non-trivial to expose a native-Rust substitute_node_with_dag. Maybe we can revisit doing so in a future PR.
- Remove stale comment related to Qiskit#3947 - Rename tuple instances to `GateIdentifier`. - Rename `doomed_nodes` to `nodes_to_replace`. - Add docstring for `get_example_gates`. - Rename `get_example_gates` to `get_gate_num_params`. Co-authored-by: Kevin Hartman <kevin@hart.mn>
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
crates/accelerate/src/basis/basis_translator/compose_transforms.rs
Outdated
Show resolved
Hide resolved
- Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged.
kevinhartman
approved these changes
Sep 27, 2024
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
This was referenced Sep 27, 2024
emilkovacev
pushed a commit
to emilkovacev/qiskit
that referenced
this pull request
Oct 1, 2024
…kit#13137) * Initial: Add `compose_transforms` and `get_example_gates` to Rust. * Add: `compose_transform` logic in rust * Fix: Correct the behavior of `compose_transforms`. - Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation. - Remove panic statement when checking for example gates. - Use qreg when adding an instruction to the mock_dag in `compose_transforms`. - Add missing comparison check in for loop to compare the mapped instructions. - Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract. - Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`. * Fix: Leverage rust-based `circuit_to_dag` converters. - Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances. * Format: Fix indentation of import lines in `compose_transforms.rs` * Formatting: Separate complicated type aliases * Fix: Adapt to new `DAGCircuit` limitations * Format: Remove unused imports in BasisTranslator * Fix: Adapt to Qiskit#13143 * Fix: Code review comments - Remove unused `circuit_to_dag` and `OperationRef` imports. - Streamline complex types into simpler aliases. - Rename `CircuitRep` to `CircuitFromPython`. - Reshape `get_example_gates` to work with `CircuitData` instances during its recursion. - Remove unnecessary clone of `gate_name` in `compose_transform`. - Use `mapped_instructions` values when iterating in `compose_transforms`. - Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust. - Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple. * Fix: More commments from code review - Remove stale comment related to Qiskit#3947 - Rename tuple instances to `GateIdentifier`. - Rename `doomed_nodes` to `nodes_to_replace`. - Add docstring for `get_example_gates`. - Rename `get_example_gates` to `get_gate_num_params`. Co-authored-by: Kevin Hartman <kevin@hart.mn> * Refactor: Rename `example_gates` to `gate_param_counts` - Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged. --------- Co-authored-by: Kevin Hartman <kevin@hart.mn>
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
ElePT
pushed a commit
to ElePT/qiskit
that referenced
this pull request
Oct 9, 2024
…kit#13137) * Initial: Add `compose_transforms` and `get_example_gates` to Rust. * Add: `compose_transform` logic in rust * Fix: Correct the behavior of `compose_transforms`. - Use `PackedOperation.control_flow` instead of `CONTROL_FLOW_OP_NAMES` to check if an instructuion is a control flow operation. - Remove panic statement when checking for example gates. - Use qreg when adding an instruction to the mock_dag in `compose_transforms`. - Add missing comparison check in for loop to compare the mapped instructions. - Use borrowed `DAGCircuit` instances in the recursion of `get_example_gates`, do not use extract. - Fix behavior of `get_example_gates` to return `PackedInstruction` instances instead of `NodeIndex`. * Fix: Leverage rust-based `circuit_to_dag` converters. - Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances. * Format: Fix indentation of import lines in `compose_transforms.rs` * Formatting: Separate complicated type aliases * Fix: Adapt to new `DAGCircuit` limitations * Format: Remove unused imports in BasisTranslator * Fix: Adapt to Qiskit#13143 * Fix: Code review comments - Remove unused `circuit_to_dag` and `OperationRef` imports. - Streamline complex types into simpler aliases. - Rename `CircuitRep` to `CircuitFromPython`. - Reshape `get_example_gates` to work with `CircuitData` instances during its recursion. - Remove unnecessary clone of `gate_name` in `compose_transform`. - Use `mapped_instructions` values when iterating in `compose_transforms`. - Rename `DAGCircuit::substitute_node_with_dag` to `DAGCircuit::py_*` in rust. - Adapted `_basis_search` to return a tuple of tuples instead of a 4-tuple. * Fix: More commments from code review - Remove stale comment related to Qiskit#3947 - Rename tuple instances to `GateIdentifier`. - Rename `doomed_nodes` to `nodes_to_replace`. - Add docstring for `get_example_gates`. - Rename `get_example_gates` to `get_gate_num_params`. Co-authored-by: Kevin Hartman <kevin@hart.mn> * Refactor: Rename `example_gates` to `gate_param_counts` - Remove `CircuitFromPython` and re-use original instance from `EquivalenceLibrary` after Qiskit#12585 merged. --------- Co-authored-by: Kevin Hartman <kevin@hart.mn>
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
The following commits add the helper function
compose_transformsfrom theBasisTranslatortranspiler pass into rust. This is a small progression towards completing #12246. This function will be temporarily exposed to Python until #12246 is fully realized.Details and comments
As we continue moving parts of the
BasisTranslatorinto Rust thanks to #12585 and #12811. The next logical step is to bring in those functions that relied heavily on theDAGCircuitand since it now lives in Rust we can now leverage its usage by moving this helper function into Rust.compose_transformsbasically returns a mapping of(gate_name, num_qubits) : (parameters, gate_definition_as_dag)for each gate present in theDAGCircuitthat is currently being processed. It also breaks downControlFlowOpto include its definitions as well.Using the latest additions by #13036 we can perform most of these conversions from rust using both
circuit_to_dagorDAGCirctut::from_circuit_data. This function will be temporarily exposed to Python until #12246 is fully realized.Blockers
apply_operationmethods #13143These are not strictly blockers but some nice-to-haves.
EquivalenceLibrary#12585basis_searchandBasisSearchVisitorto rust. #12811