[Oxidize BasisTranslator] Add rust-native `compose_transforms()` #13137

raynelfss · 2024-09-11T22:10:31Z

Summary

The following commits add the helper function compose_transforms from the BasisTranslator transpiler 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 BasisTranslator into Rust thanks to #12585 and #12811. The next logical step is to bring in those functions that relied heavily on the DAGCircuit and since it now lives in Rust we can now leverage its usage by moving this helper function into Rust.

compose_transforms basically returns a mapping of (gate_name, num_qubits) : (parameters, gate_definition_as_dag) for each gate present in the DAGCircuit that is currently being processed. It also breaks down ControlFlowOp to include its definitions as well.

Using the latest additions by #13036 we can perform most of these conversions from rust using both circuit_to_dag or DAGCirctut::from_circuit_data. This function will be temporarily exposed to Python until #12246 is fully realized.

Blockers

Add rust-native apply_operation methods #13143

These are not strictly blockers but some nice-to-haves.

- 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.

qiskit-bot · 2024-09-11T22:10:36Z

One or more of the following people are relevant to this code:

@Qiskit/terra-core
@kevinhartman
@mtreinish

coveralls · 2024-09-11T22:34:48Z

Pull Request Test Coverage Report for Build 10925762356

Warning: 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.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

132 of 150 (88.0%) changed or added relevant lines in 7 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage increased (+0.6%) to 88.935%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/basis/mod.rs	4	5	80.0%
crates/accelerate/src/basis/basis_translator/compose_transforms.rs	115	132	87.12%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	1	92.73%
crates/accelerate/src/two_qubit_decompose.rs	1	91.45%

Totals
Change from base Build 10919208944:	0.6%
Covered Lines:	73667
Relevant Lines:	82832

💛 - Coveralls

raynelfss · 2024-09-11T22:42:02Z

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.

kevinhartman

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.

kevinhartman · 2024-09-19T17:54:27Z

crates/accelerate/src/basis/basis_translator/mod.rs

@@ -0,0 +1,21 @@
+// This code is part of Qiskit.


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).

kevinhartman · 2024-09-19T18:13:54Z

crates/accelerate/src/basis/basis_translator/compose_transforms.rs

+pub type BasisTransforms = Vec<(String, u32, SmallVec<[Param; 3]>, CircuitRep)>;
+pub type MappedTransforms = HashMap<(String, u32), (SmallVec<[Param; 3]>, DAGCircuit)>;


IMO, type aliases are generally more helpful when used to break down complex types into logical ideas, rather than as a means of typing less.

For example, this:

pub type GateIdentifier = (String, u32); pub type BasisTransformIn = (SmallVec<[Param; 3]>, CircuitRep); pub type BasisTransformOut = (SmallVec<[Param; 3]>, DAGCircuit); pub(super) fn py_compose_transforms( py: Python, basis_transforms: Vec<(GateIdenfitier, BasisTransformIn)>, source_basis: HashSet<GateIdentifier>, source_dag: &DAGCircuit, ) -> PyResult<HashMap<GateIdentifier, BasisTransformOut>>

gives the reader a lot more information about the call semantics of the function compared to this:

pub type BasisTransforms = Vec<(String, u32, SmallVec<[Param; 3]>, CircuitRep)>; pub type MappedTransforms = HashMap<(String, u32), (SmallVec<[Param; 3]>, DAGCircuit)>; pub(super) fn py_compose_transforms( py: Python, basis_transforms: BasisTransforms, source_basis: HashSet<(String, u32)>, source_dag: &DAGCircuit, ) -> PyResult<MappedTransforms>

And, as a general rule of thumb, it's usually best to avoid aliasing data structures like Vec and HashMap since it hides their otherwise very familiar semantics from the reader (...though, we're guilty of this in a few places, I expect.)

kevinhartman · 2024-09-19T18:19:10Z

crates/accelerate/src/basis/basis_translator/compose_transforms.rs

+            let data_downcast: Bound<CircuitData> = data.downcast_into()?;
+            let data_extract: CircuitData = data_downcast.extract()?;


I believe you can extract from the Bound<PyAny> without first downcasting to save an isinstance check.

kevinhartman · 2024-09-19T19:03:59Z

crates/circuit/src/dag_circuit.rs

@@ -2861,7 +2861,7 @@ def _format(operand):
    /// Raises:
    ///     DAGCircuitError: if met with unexpected predecessor/successors
    #[pyo3(signature = (node, input_dag, wires=None, propagate_condition=true))]
-    fn substitute_node_with_dag(
+    pub fn substitute_node_with_dag(


Perhaps a strange request, but I think it makes sense for us to rename any PyO3 methods to py_ when opening them up outside of DAGCircuit. The reason being that it'll make it easier for us to notice calls to Python-intended methods from within Rust, which are likely to be ill-performing in contrast to anything proper Rust native.

kevinhartman · 2024-09-19T19:09:51Z

crates/accelerate/src/basis/basis_translator/compose_transforms.rs

+    example_gates: Option<Box<HashMap<(String, u32), PackedInstruction>>>,
+) -> PyResult<Box<HashMap<(String, u32), PackedInstruction>>> {


The original Python function is strange. A more appropriate way to do this in Rust would be to require that the caller allocates the map and provides a mutable reference to it so all recursive calls to the function can keep appending to it.

Suggested change

example_gates: Option<Box<HashMap<(String, u32), PackedInstruction>>>,

) -> PyResult<Box<HashMap<(String, u32), PackedInstruction>>> {

example_gates: &mut HashMap<(String, u32), PackedInstruction>,

) -> PyResult<()> {

kevinhartman · 2024-09-19T19:31:57Z

crates/accelerate/src/basis/basis_translator/compose_transforms.rs

+
+    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 .


This issue appears to be closed. I think the comment may be outdated in your implementation, anyhow?

kevinhartman · 2024-09-19T20:01:31Z

crates/accelerate/src/basis/basis_translator/compose_transforms.rs

+/// Representation of QuantumCircuit which the original circuit object + an
+/// instance of `CircuitData`.
+#[derive(Debug, Clone)]
+pub struct CircuitRep(pub CircuitData);


Maybe CircuitFromPython could be a nice name, similar to how we have OperationFromPython.

kevinhartman · 2024-09-19T20:04:48Z

qiskit/transpiler/passes/basis/basis_translator.py

-        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:


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.

kevinhartman · 2024-09-19T20:11:11Z

crates/accelerate/src/basis/basis_translator/compose_transforms.rs

+            Some(gate.into()),
+        )?;
+        mapped_instructions.insert(
+            (gate_name.clone(), gate_num_qubits),


Do we need to clone gate_name here, or can we just consume it? I don't see anymore uses after this :)

kevinhartman · 2024-09-19T20:12:48Z

crates/accelerate/src/basis/basis_translator/compose_transforms.rs

+impl IntoPy<PyObject> for CircuitRep {
+    fn into_py(self, py: Python<'_>) -> PyObject {
+        QUANTUM_CIRCUIT
+            .get_bound(py)
+            .call_method1("_from_circuit_data", (self.0,))
+            .unwrap()
+            .unbind()
+    }
+}
+
+impl ToPyObject for CircuitRep {
+    fn to_object(&self, py: Python<'_>) -> PyObject {
+        self.clone().into_py(py)
+    }
+}


Are these used?

raynelfss and others added 5 commits September 10, 2024 14:39

Initial: Add compose_transforms and get_example_gates to Rust.

f9e97ea

Add: compose_transform logic in rust

3a3e734

Merge main

1a49bfd

Fix: Leverage rust-based circuit_to_dag converters.

fbb57b8

- Use `circuit_to_dag` and `DAGCircuit::from_circuit_data` to convert `QuantumCircuit` instances.

raynelfss requested a review from a team as a code owner September 11, 2024 22:10

raynelfss added this to the 1.3.0 milestone Sep 11, 2024

Merge branch 'main' into rust-compose-trans

e8c957d

raynelfss added performance Changelog: None Do not include in changelog Rust This PR or issue is related to Rust code in the repository labels Sep 11, 2024

Format: Fix indentation of import lines in compose_transforms.rs

a7d2f17

raynelfss added 6 commits September 12, 2024 08:59

Formatting: Separate complicated type aliases

8e96eb7

Merge branch 'main' into rust-compose-trans

18591ce

Fix: Adapt to new DAGCircuit limitations

676766d

Merge branch 'main' into rust-compose-trans

1b16df6

Format: Remove unused imports in BasisTranslator

a2cb862

Merge branch 'main' into rust-compose-trans

c9082c7

raynelfss added the on hold Can not fix yet label Sep 13, 2024

raynelfss and others added 2 commits September 18, 2024 11:33

Merge branch 'Qiskit:main' into rust-compose-trans

3d1a6d4

Fix: Adapt to Qiskit#13143

956c425

raynelfss removed the on hold Can not fix yet label Sep 18, 2024

raynelfss mentioned this pull request Sep 18, 2024

Add cache_pygates feature to accelerate crate. #13183

Merged

raynelfss changed the title ~~[Oxidize BaisTranslator] Add rust-native compose_transforms()~~ [Oxidize BasisTranslator] Add rust-native compose_transforms() Sep 18, 2024

kevinhartman reviewed Sep 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Oxidize BasisTranslator] Add rust-native `compose_transforms()` #13137

[Oxidize BasisTranslator] Add rust-native `compose_transforms()` #13137

raynelfss commented Sep 11, 2024 •

edited

Loading

qiskit-bot commented Sep 11, 2024

coveralls commented Sep 11, 2024 •

edited

Loading

raynelfss commented Sep 11, 2024

kevinhartman left a comment •

edited

Loading

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

kevinhartman Sep 19, 2024

		pub type BasisTransforms = Vec<(String, u32, SmallVec<[Param; 3]>, CircuitRep)>;
		pub type MappedTransforms = HashMap<(String, u32), (SmallVec<[Param; 3]>, DAGCircuit)>;

		let data_downcast: Bound<CircuitData> = data.downcast_into()?;
		let data_extract: CircuitData = data_downcast.extract()?;

		example_gates: Option<Box<HashMap<(String, u32), PackedInstruction>>>,
		) -> PyResult<Box<HashMap<(String, u32), PackedInstruction>>> {

[Oxidize BasisTranslator] Add rust-native compose_transforms() #13137

Are you sure you want to change the base?

[Oxidize BasisTranslator] Add rust-native compose_transforms() #13137

Conversation

raynelfss commented Sep 11, 2024 • edited Loading

Summary

Details and comments

Blockers

qiskit-bot commented Sep 11, 2024

coveralls commented Sep 11, 2024 • edited Loading

Pull Request Test Coverage Report for Build 10925762356

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

raynelfss commented Sep 11, 2024

kevinhartman left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[Oxidize BasisTranslator] Add rust-native `compose_transforms()` #13137

[Oxidize BasisTranslator] Add rust-native `compose_transforms()` #13137

raynelfss commented Sep 11, 2024 •

edited

Loading

coveralls commented Sep 11, 2024 •

edited

Loading

kevinhartman left a comment •

edited

Loading