Oxidize GateDirection pass #13069

eliarbel · 2024-09-02T11:25:33Z

Summary

Move GateDirection._run_coupling_map and GateDirection._run_target to Rust. Also moving the code which creates replacement DAGs to Rust. Both GateDirection and CheckGateDirection will reside after this in gate_direction.rs.

Details and comments

Depends on #13042
Close #12252

Move GateDirection._run_coupling_map and GateDirection._run_target to Rust. Also moving the code to create replacement DAGs to Rust. Both GateDirection and CheckGateDirection will reside in gate_direction.rs.

qiskit-bot · 2024-10-25T09:19:45Z

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

@Qiskit/terra-core

eliarbel · 2024-10-25T09:21:03Z

Will add performance benchmarking numbers soon.

coveralls · 2024-10-25T09:42:34Z

Pull Request Test Coverage Report for Build 11665219277

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

347 of 355 (97.75%) changed or added relevant lines in 3 files are covered.
6 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.04%) to 88.755%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
crates/accelerate/src/gate_direction.rs	343	351	97.72%

Files with Coverage Reduction	New Missed Lines	%
crates/accelerate/src/unitary_synthesis.rs	1	92.2%
crates/accelerate/src/two_qubit_decompose.rs	1	92.09%
crates/qasm2/src/lex.rs	4	92.23%

Totals
Change from base Build 11636461507:	0.04%
Covered Lines:	76674
Relevant Lines:	86388

💛 - Coveralls

raynelfss

This looks pretty good so far, I have a couple of comments mainly suggesting the usage of rust native methods for the DAGCircuit. But I feel this should be ready after those corrections.

crates/accelerate/src/gate_direction.rs

test/python/transpiler/test_gate_direction.py

raynelfss · 2024-10-25T15:55:44Z

crates/accelerate/src/gate_direction.rs

+    m.add_wrapped(wrap_pyfunction!(py_fix_with_coupling_map))?;
+    m.add_wrapped(wrap_pyfunction!(py_fix_with_target))?;


Is there a reason why we're callingn these fix_... and not run_...?

As stated in #13069 (comment) - the main reason being that both GateDirection and CheckGateDirection live in this file and there are the check_... couple and the fix_... couple, each pair corresponds to their respective passes. I could BTW just have one run method for each pass which could take both Target and a coupling map but I actually liked better the modularity of having 2 entry points (one for target and one for coupling map), where each defines a specialized closure but both call the same function for each pass. These are being called by GateDirection.run() anyway. That said, should we follow a convention that all pass entry functions should be named run..,? If so I'll prefix the four python functions in the file with it. Let me know.

It is good to have conventions to keep everything consistent. That said, we haven't had any of them apply to transpiler passes in rust yet. We do have the main run method calling upon other helper methods, so having the rust methods that represent the main run method of the pass have run somewhere in the name would be nice. I did it as a suffix for #13237, but having it as a prefix sounds more elegant. Maybe we should open an issue about these conventions here (sort of what #12936 proposes).

On a second thought and after considering some alternatives I actually liked most this one: py_check_direction_coupling_map (and similarly for the fix and target variants). I find it more informational and succinct than other options like: py_run_check_with_coupling_map or py_run_check_coupling_map or py_run_check_direction_coupling_map etc... Also the py_ prefix in it indicates that it's the Python entry point, hence also the pass entry point so the run prefix is not really required for this purpose. So unless you feel strongly against it, I would prefer to stay with this more informational naming scheme here until we decide on a convention for the pass entry point names.

raynelfss · 2024-10-25T15:57:30Z

crates/accelerate/src/gate_direction.rs

    m.add_wrapped(wrap_pyfunction!(py_check_with_coupling_map))?;
    m.add_wrapped(wrap_pyfunction!(py_check_with_target))?;


Are we using these two methods outside of Rust? I don't see them being used in the python gate_direction.py fie.

Yes, these are used in check_gate_direction.py. Both CheckGateDirection and GateDirection are implemented in gate_direction.rs. The logic is very similar for both, so I figured it makes sense to put those together and they are rather small each.

Mainly replace going through Python with Rust-native calls in various places, most prominently when building replacement DAGs.

raynelfss

This looks about ready to merge, just a couple of suggestions that warrant discussion.

raynelfss · 2024-10-29T17:13:24Z

crates/accelerate/src/gate_direction.rs

+
+    for i in 0..num_qubits {
+        let qubit = qreg.call_method1(intern!(py, "__getitem__"), (i,))?;
+        qargs.push(dag.qubits().find(&qubit).expect("Qubit should stored"));


Suggested change

qargs.push(dag.qubits().find(&qubit).expect("Qubit should stored"));

qargs.push(dag.qubits().find(&qubit).expect("Qubit should have been stored in the DAGCircuit"));

Ah... good catch. Thx. Done in 79a6217

raynelfss · 2024-10-29T17:13:56Z

crates/accelerate/src/gate_direction.rs

+        qargs,
+        &[],
+        param,
+        ExtraInstructionAttributes::new(None, None, None, None),


Use Default here:

Suggested change

ExtraInstructionAttributes::new(None, None, None, None),

ExtraInstructionAttributes::default(),

Done in 79a6217

raynelfss · 2024-10-29T17:39:27Z

crates/accelerate/src/gate_direction.rs

+fn cx_replacement_dag(py: Python) -> PyResult<DAGCircuit> {
+    let new_dag = &mut DAGCircuit::new(py)?;
+    let qargs = add_qreg(py, new_dag, 2)?;
+    let qargs = qargs.as_slice();
+
+    apply_operation_back(py, new_dag, StandardGate::HGate, &[qargs[0]], None)?;
+    apply_operation_back(py, new_dag, StandardGate::HGate, &[qargs[1]], None)?;
+    apply_operation_back(
+        py,
+        new_dag,
+        StandardGate::CXGate,
+        &[qargs[1], qargs[0]],
+        None,
+    )?;
+    apply_operation_back(py, new_dag, StandardGate::HGate, &[qargs[0]], None)?;
+    apply_operation_back(py, new_dag, StandardGate::HGate, &[qargs[1]], None)?;
+
+    Ok(new_dag.clone())
+}


Could we maybe optimize these circuit building functions by having them work more like OnceCell? Where you could create a static instance that is initialized just once, you can then just call clone each time you need a replacement DAGCircuit. For this you could use GILOnceCell, and have these function calls just call on .get_or_init(py, |_| )

Here's an example of how it could work:

static FOO_DAG: GILOnceCell<PyResult<DAGCircuit>> = GILOnceCell::new(); fn foo_replacement_dag(py: Python) -> PyResult<DAGCircuit> { FOO_DAG.get_or_init(py, || -> PyResult<DAGCircuit> { let mut dag: DAGCircuit = DAGCircuit::new(py); // Not sure if you can call `add_qreg` from within the closure. let qargs = add_qreg(py, new_dag, 2)?; let qargs = qargs.as_slice(); // not sure if you can call `apply_operation_back` from inside the closure. apply_operation_back(py, &mut dag, StandardGate::XGate, &[qargs[0]], None)?; Ok(dag) }).cloned() }

This ensures that the circuit is built one and that whenever we call on this function, we just clone the circuit.

This change is merely a suggestion since #13335 makes improvements on chained additions to the DAGCircuit and these changes could be made in a follow up 🙃.

As said in #13069 (comment) this is definitely an optimization opportunity I would like to explore. I am actually exploring already some caching implementation, similar to what you suggested, namely generating a replacement DAG (on-demand) once and cloning when needed. This applies to non-parameterized gates only since replacements for parameterized gates (e.g. RZX) depend on the actual parameter. It also includes caching a template DAG for RZX and then when an RZX replacement is needed replacing the RZX node with a concrete parametrically-bound RZX node. Then I also want to look at avoiding calling .clone() , which I believe could give even more improvement, but this will require more refactoring. But bottom-line I'm afraid I will not have time to properly explore and benchmark more optimization for the 1.3 release, so I think we should have this in a new PR. And obviously there is a risk of jut trying to micro optimize without too much benefit, but again, I'll need more time to benchmark in detail.

eliarbel · 2024-10-29T21:21:24Z

Runtime comparison results:

Before [`f2e07bc`] main	After [`4b363a1`] fix-gate-direction	Ratio	Benchmark (num_qubits, depth)
103±5ms	5.79±0.3ms	0.06	gate_direction.GateDirectionBenchmarks.time_gate_direction_target(14, 1024)
28.8±3ms	1.79±0.1ms	0.06	gate_direction.GateDirectionBenchmarks.time_gate_direction_target(5, 1024)
56.8±0.9ms	6.52±0.1ms	0.11	mapping_passes.RoutedPassBenchmarks.time_gate_direction(20, 1024)
6.48±0.3ms	687±20μs	0.11	mapping_passes.RoutedPassBenchmarks.time_gate_direction(5, 1024)

raynelfss

LGTM! Just one inline comment that I don't think we'll address here. Great job!

raynelfss · 2024-11-04T16:21:54Z

crates/accelerate/src/gate_direction.rs

+    let dag_op_node = Py::new(
+        py,
+        (
+            DAGOpNode {
+                instruction: CircuitInstruction {
+                    operation: packed_inst.op.clone(),
+                    qubits: py_args.unbind(),
+                    clbits: PyTuple::empty_bound(py).unbind(),
+                    params: packed_inst.params_view().iter().cloned().collect(),
+                    extra_attrs: packed_inst.extra_attrs.clone(),
+                    #[cfg(feature = "cache_pygates")]
+                    py_op: packed_inst.py_op.clone(),
+                },
+                sort_key: "".into_py(py),
+            },
+            DAGNode { node: None },
+        ),
+    )?;


This is something I discussed with @kevinhartman, but we will probably move to using .into_py(py) to convert an PyClass to a PyObject in Rust, but we want to make sure to do it in a follow up to make sure it is used everywhere, so I'm okay with leaving it as it is here but perform the change to .into_py(py) in a follow up.

eliarbel added this to the 1.3.0 milestone Sep 11, 2024

eliarbel added 4 commits September 15, 2024 11:38

First working implementation

e7188a0

Rebase on main with CheckGateDirection ported

257c0d3

Port GateDirection to Rust

8dc1337

Move GateDirection._run_coupling_map and GateDirection._run_target to Rust. Also moving the code to create replacement DAGs to Rust. Both GateDirection and CheckGateDirection will reside in gate_direction.rs.

First working implementation

1a3c38d

eliarbel closed this Sep 15, 2024

eliarbel force-pushed the fix-gate-direction branch from 30bf21b to 653b5b6 Compare September 15, 2024 09:21

eliarbel reopened this Sep 15, 2024

eliarbel added 3 commits September 15, 2024 12:32

Fix minor build issues

e5ad421

Merge branch 'main' into fix-gate-direction

c83de97

Merge branch 'main' into fix-gate-direction

99f9f24

raynelfss self-assigned this Oct 22, 2024

eliarbel added 2 commits October 25, 2024 11:43

Complete implementation

0adc48f

Merge branch 'main' into fix-gate-direction

253c554

eliarbel marked this pull request as ready for review October 25, 2024 09:19

eliarbel requested a review from a team as a code owner October 25, 2024 09:19

mtreinish added performance Rust This PR or issue is related to Rust code in the repository labels Oct 25, 2024

raynelfss reviewed Oct 25, 2024

View reviewed changes

Address review comments

4b363a1

Mainly replace going through Python with Rust-native calls in various places, most prominently when building replacement DAGs.

raynelfss reviewed Oct 29, 2024

View reviewed changes

eliarbel added 2 commits November 4, 2024 14:48

Rename functions and other minor changes

79a6217

Merge branch 'main' into fix-gate-direction

03136e9

raynelfss approved these changes Nov 4, 2024

View reviewed changes

raynelfss added this pull request to the merge queue Nov 4, 2024

Merged via the queue into Qiskit:main with commit 2a1fd08 Nov 4, 2024
17 checks passed

eliarbel added the Changelog: None Do not include in changelog label Nov 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Oxidize GateDirection pass #13069

Oxidize GateDirection pass #13069

eliarbel commented Sep 2, 2024

qiskit-bot commented Oct 25, 2024

eliarbel commented Oct 25, 2024

coveralls commented Oct 25, 2024 •

edited

Loading

raynelfss left a comment

raynelfss Oct 25, 2024

eliarbel Oct 29, 2024

raynelfss Oct 29, 2024

eliarbel Nov 4, 2024

raynelfss Oct 25, 2024

eliarbel Oct 29, 2024

raynelfss left a comment

raynelfss Oct 29, 2024

eliarbel Nov 4, 2024

raynelfss Oct 29, 2024

eliarbel Nov 4, 2024

raynelfss Oct 29, 2024

eliarbel Nov 4, 2024

eliarbel commented Oct 29, 2024

raynelfss left a comment

raynelfss Nov 4, 2024

		m.add_wrapped(wrap_pyfunction!(py_fix_with_coupling_map))?;
		m.add_wrapped(wrap_pyfunction!(py_fix_with_target))?;

		m.add_wrapped(wrap_pyfunction!(py_check_with_coupling_map))?;
		m.add_wrapped(wrap_pyfunction!(py_check_with_target))?;

	qargs.push(dag.qubits().find(&qubit).expect("Qubit should stored"));
	qargs.push(dag.qubits().find(&qubit).expect("Qubit should have been stored in the DAGCircuit"));

	ExtraInstructionAttributes::new(None, None, None, None),
	ExtraInstructionAttributes::default(),

Oxidize GateDirection pass #13069

Oxidize GateDirection pass #13069

Conversation

eliarbel commented Sep 2, 2024

Summary

Details and comments

qiskit-bot commented Oct 25, 2024

eliarbel commented Oct 25, 2024

coveralls commented Oct 25, 2024 • edited Loading

Pull Request Test Coverage Report for Build 11665219277

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

raynelfss left a 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

raynelfss left a 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

eliarbel commented Oct 29, 2024

raynelfss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Oct 25, 2024 •

edited

Loading