From ba48d556eae95e873a78ef720f7c20b10a86eb23 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Tue, 17 Sep 2024 13:07:29 -0400 Subject: [PATCH 01/17] POC for using PyO3 in cargo unit tests. --- crates/circuit/src/dag_circuit.rs | 46 +++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/crates/circuit/src/dag_circuit.rs b/crates/circuit/src/dag_circuit.rs index 5b218b2de818..1017215bdc0b 100644 --- a/crates/circuit/src/dag_circuit.rs +++ b/crates/circuit/src/dag_circuit.rs @@ -6800,3 +6800,49 @@ fn add_global_phase(py: Python, phase: &Param, other: &Param) -> PyResult } type SortKeyType<'a> = (&'a [Qubit], &'a [Clbit]); + +#[cfg(test)] +mod test { + use crate::dag_circuit::DAGCircuit; + use crate::imports::{CLASSICAL_REGISTER, QUANTUM_REGISTER}; + use crate::operations::StandardGate; + use crate::packed_instruction::{PackedInstruction, PackedOperation}; + use crate::Qubit; + use pyo3::prelude::*; + use pyo3::Python; + + fn new_dag(py: Python, qubits: u32, clbits: u32) -> DAGCircuit { + let qreg = QUANTUM_REGISTER.get_bound(py).call1((qubits,)).unwrap(); + let creg = CLASSICAL_REGISTER.get_bound(py).call1((clbits,)).unwrap(); + let mut dag = DAGCircuit::new(py).unwrap(); + dag.add_qreg(py, &qreg).unwrap(); + dag.add_creg(py, &creg).unwrap(); + dag + } + + #[test] + fn test_push_back() { + pyo3::prepare_freethreaded_python(); + Python::with_gil(|py| { + let mut dag = new_dag(py, 4, 4); + let cx = PackedInstruction { + op: PackedOperation::from_standard(StandardGate::CXGate), + qubits: dag.qargs_interner.insert_owned(vec![Qubit(0), Qubit(1)]), + clbits: dag.cargs_interner.get_default(), + params: None, + extra_attrs: Default::default(), + #[cfg(feature = "cache_pygates")] + py_op: Default::default(), + }; + let cx_node = dag.push_back(py, cx).unwrap(); + + let [q0_in_node, q0_out_node] = dag.qubit_io_map[0]; + let [q1_in_node, q1_out_node] = dag.qubit_io_map[1]; + + assert!(dag.dag.find_edge(q0_in_node, cx_node).is_some()); + assert!(dag.dag.find_edge(q1_in_node, cx_node).is_some()); + assert!(dag.dag.find_edge(cx_node, q0_out_node).is_some()); + assert!(dag.dag.find_edge(cx_node, q1_out_node).is_some()); + }); + } +} From 4425ac16311133eb70f67368741ed89934fb87a0 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Tue, 17 Sep 2024 16:14:20 -0400 Subject: [PATCH 02/17] Reorder jobs in test-linux.yml. --- .azure/test-linux.yml | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/.azure/test-linux.yml b/.azure/test-linux.yml index a641ae215efd..fa65d8ad93a2 100644 --- a/.azure/test-linux.yml +++ b/.azure/test-linux.yml @@ -45,17 +45,6 @@ jobs: path: .stestr displayName: "Cache stestr" - - ${{ if eq(parameters.testRust, true) }}: - # We need to avoid linking our crates into full Python extension libraries during Rust-only - # testing because Rust/PyO3 can't handle finding a static CPython interpreter. - - bash: cargo test --no-default-features - env: - # On Linux we link against `libpython` dynamically, but it isn't written into the rpath - # of the test executable (I'm not 100% sure why ---Jake). It's easiest just to forcibly - # include the correct place in the `dlopen` search path. - LD_LIBRARY_PATH: '$(usePython.pythonLocation)/lib:$LD_LIBRARY_PATH' - displayName: "Run Rust tests" - - bash: | set -e python -m pip install --upgrade pip setuptools wheel virtualenv @@ -107,6 +96,19 @@ jobs: sudo apt-get install -y graphviz displayName: 'Install optional non-Python dependencies' + # Note that we do this explicitly *after* installing Qiskit since some Rust tests + # still depend on Qiskit's Python API via PyO3. + - ${{ if eq(parameters.testRust, true) }}: + # We need to avoid linking our crates into full Python extension libraries during Rust-only + # testing because Rust/PyO3 can't handle finding a static CPython interpreter. + - bash: cargo test --no-default-features + env: + # On Linux we link against `libpython` dynamically, but it isn't written into the rpath + # of the test executable (I'm not 100% sure why ---Jake). It's easiest just to forcibly + # include the correct place in the `dlopen` search path. + LD_LIBRARY_PATH: '$(usePython.pythonLocation)/lib:$LD_LIBRARY_PATH' + displayName: "Run Rust tests" + - bash: | set -e source test-job/bin/activate From 63501bc644b632d8453ddc2cf9bcb3ffa0bd6ec0 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Tue, 17 Sep 2024 16:27:49 -0400 Subject: [PATCH 03/17] Actually use the venv... --- .azure/test-linux.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.azure/test-linux.yml b/.azure/test-linux.yml index fa65d8ad93a2..0d145553e9c0 100644 --- a/.azure/test-linux.yml +++ b/.azure/test-linux.yml @@ -101,7 +101,10 @@ jobs: - ${{ if eq(parameters.testRust, true) }}: # We need to avoid linking our crates into full Python extension libraries during Rust-only # testing because Rust/PyO3 can't handle finding a static CPython interpreter. - - bash: cargo test --no-default-features + - bash: | + source test-job/bin/activate + python tools/report_numpy_state.py + cargo test --no-default-features env: # On Linux we link against `libpython` dynamically, but it isn't written into the rpath # of the test executable (I'm not 100% sure why ---Jake). It's easiest just to forcibly From 2fd982f950ebce97553f17c5cd4774fec0c9a459 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Tue, 17 Sep 2024 16:35:44 -0400 Subject: [PATCH 04/17] Disable Miri for PyO3-based cargo tests. --- crates/circuit/src/dag_circuit.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/circuit/src/dag_circuit.rs b/crates/circuit/src/dag_circuit.rs index 1017215bdc0b..e1b7806ff5f4 100644 --- a/crates/circuit/src/dag_circuit.rs +++ b/crates/circuit/src/dag_circuit.rs @@ -6802,6 +6802,7 @@ fn add_global_phase(py: Python, phase: &Param, other: &Param) -> PyResult type SortKeyType<'a> = (&'a [Qubit], &'a [Clbit]); #[cfg(test)] +#[cfg_attr(miri, ignore)] mod test { use crate::dag_circuit::DAGCircuit; use crate::imports::{CLASSICAL_REGISTER, QUANTUM_REGISTER}; From 1b594c17241d43745efaab6ccc0b9b47d63ca1b0 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Tue, 17 Sep 2024 16:42:08 -0400 Subject: [PATCH 05/17] Fix disable for module-level. --- crates/circuit/src/dag_circuit.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/circuit/src/dag_circuit.rs b/crates/circuit/src/dag_circuit.rs index e1b7806ff5f4..4fc2eaeaebab 100644 --- a/crates/circuit/src/dag_circuit.rs +++ b/crates/circuit/src/dag_circuit.rs @@ -6801,8 +6801,7 @@ fn add_global_phase(py: Python, phase: &Param, other: &Param) -> PyResult type SortKeyType<'a> = (&'a [Qubit], &'a [Clbit]); -#[cfg(test)] -#[cfg_attr(miri, ignore)] +#[cfg(all(test, not(miri)))] mod test { use crate::dag_circuit::DAGCircuit; use crate::imports::{CLASSICAL_REGISTER, QUANTUM_REGISTER}; From d5d4714ef8f6a8b62e3f20661fdd9639f3b1db69 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Tue, 17 Sep 2024 16:54:43 -0400 Subject: [PATCH 06/17] Improve comment in Rust test runner CI job. --- .azure/test-linux.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.azure/test-linux.yml b/.azure/test-linux.yml index 0d145553e9c0..9cba9a7d96c4 100644 --- a/.azure/test-linux.yml +++ b/.azure/test-linux.yml @@ -96,8 +96,8 @@ jobs: sudo apt-get install -y graphviz displayName: 'Install optional non-Python dependencies' - # Note that we do this explicitly *after* installing Qiskit since some Rust tests - # still depend on Qiskit's Python API via PyO3. + # Note that we explicitly use the virtual env with Qiskit installed to run the Rust + # tests since some of them still depend on Qiskit's Python API via PyO3. - ${{ if eq(parameters.testRust, true) }}: # We need to avoid linking our crates into full Python extension libraries during Rust-only # testing because Rust/PyO3 can't handle finding a static CPython interpreter. From 63d6334b07d001d3a4be1c4c7a1ffd6a9c6a951d Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Tue, 17 Sep 2024 17:44:23 -0400 Subject: [PATCH 07/17] Use pyo3/auto-initialize feature in test. --- crates/accelerate/Cargo.toml | 3 +++ crates/circuit/Cargo.toml | 3 +++ crates/circuit/src/dag_circuit.rs | 1 - 3 files changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/accelerate/Cargo.toml b/crates/accelerate/Cargo.toml index 4c570f4a5285..4b21d014ec42 100644 --- a/crates/accelerate/Cargo.toml +++ b/crates/accelerate/Cargo.toml @@ -58,3 +58,6 @@ features = ["ndarray"] [dependencies.pulp] version = "0.18.22" features = ["macro"] + +[dev-dependencies] +pyo3 = { workspace = true, features = ["auto-initialize"] } diff --git a/crates/circuit/Cargo.toml b/crates/circuit/Cargo.toml index ed1f849bbf62..2876f4ef7e0e 100644 --- a/crates/circuit/Cargo.toml +++ b/crates/circuit/Cargo.toml @@ -39,3 +39,6 @@ features = ["union"] [features] cache_pygates = [] + +[dev-dependencies] +pyo3 = { workspace = true, features = ["auto-initialize"] } diff --git a/crates/circuit/src/dag_circuit.rs b/crates/circuit/src/dag_circuit.rs index 4fc2eaeaebab..7545d45223ce 100644 --- a/crates/circuit/src/dag_circuit.rs +++ b/crates/circuit/src/dag_circuit.rs @@ -6822,7 +6822,6 @@ mod test { #[test] fn test_push_back() { - pyo3::prepare_freethreaded_python(); Python::with_gil(|py| { let mut dag = new_dag(py, 4, 4); let cx = PackedInstruction { From f0925e71a7836c6c4343ee2f9c55785428a45599 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Fri, 20 Sep 2024 17:28:39 -0400 Subject: [PATCH 08/17] Update contibuting guide. --- CONTRIBUTING.md | 63 ++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 55 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 53043ffaa289..de60e392e1c3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -572,10 +572,15 @@ Note: If you have run `test/ipynb/mpl_tester.ipynb` locally it is possible some ### Testing Rust components -Rust-accelerated functions are generally tested from Python space, but in cases -where there is Rust-specific internal details to be tested, `#[test]` functions -can be included inline. Typically it's most convenient to place these in a -separate inline module that is only conditionally compiled in, such as +Many of Qiskit's core data structures and algorithms are implemented in Rust. +The bulk of this code is exercised heavily by our Python-based unit testing, +but this coverage really only provides integration-level testing from the +perspective of Rust. + +To provide proper Rust unit testing, we use `cargo test`. Rust tests are +integrated directly into the Rust file being tested within a `tests` module. +Functions decorated with `#[test]` within these modules are built and run +as tests. ```rust #[cfg(test)] @@ -587,15 +592,57 @@ mod tests { } ``` -To run the Rust-space tests, do +Rust tests are run separately from the Python tests. To run them, do ```bash +python setup.py build_rust --release --inplace cargo test --no-default-features ``` -Our Rust-space components are configured such that setting the -``-no-default-features`` flag will compile the test runner, but not attempt to -build a linked CPython extension module, which would cause linker failures. +The first command builds Qiskit from source (in release mode, but --debug is fine too), +which ensures that Rust tests that interact with Qiskit's Python code actually +use the latest Python code from your working directory. + +The second command actually invokes the tests via Cargo. The ``-no-default-features`` +flag is used to compile an isolated test runner without building a linked CPython +extension module (which would otherwise cause linker failures). + +#### Calling Python from Rust tests +By default, our Cargo project configuration allows Rust tests to interact with the +Python interpreter by calling `Python::with_gil` to obtain a `Python` (`py`) token. +This is particularly helpful when testing Rust code that (still) requires interaction +with Python. + +To execute code that needs the GIL in your tests, define the `tests` module as +follows: + +```rust +#[cfg(all(test, not(miri)))] // disable for Miri! +mod tests { + use pyo3::prelude::*; + + #[test] + fn my_first_test() { + Python::with_gil(|py| { + todo!() // do something that needs a `py` token. + }) + } +} +``` + +To ensure that Rust tests are properly importing changes in Qiskit's Python code +from the working directory, be sure to build and install Qiskit from source prior +to running `cargo test --no-default-features`. + +> [!IMPORTANT] +> Note that we explicitly disable compilation of such tests when running with Miri, i.e. +`#[cfg(not(miri))]`. This is necessary because Miri doesn't support the FFI +> code used internally by PyO3. +> +> If not all of your tests will use the `Python` token, you can disable Miri on a per-test +basis within the same module by decorating *the specific test* with `#[cfg_attr(miri, ignore)]` +instead of disabling Miri for the entire module. + ### Unsafe code and Miri From 86ec3384fdb437915b5c2e9de70b496c2aa79d26 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Thu, 26 Sep 2024 14:00:28 -0400 Subject: [PATCH 09/17] Testing for push_back and push_front. --- crates/circuit/src/dag_circuit.rs | 190 +++++++++++++++++++++++++++--- crates/circuit/src/imports.rs | 1 + 2 files changed, 173 insertions(+), 18 deletions(-) diff --git a/crates/circuit/src/dag_circuit.rs b/crates/circuit/src/dag_circuit.rs index 7545d45223ce..1f3774082436 100644 --- a/crates/circuit/src/dag_circuit.rs +++ b/crates/circuit/src/dag_circuit.rs @@ -6803,13 +6803,17 @@ type SortKeyType<'a> = (&'a [Qubit], &'a [Clbit]); #[cfg(all(test, not(miri)))] mod test { - use crate::dag_circuit::DAGCircuit; - use crate::imports::{CLASSICAL_REGISTER, QUANTUM_REGISTER}; + use crate::circuit_instruction::OperationFromPython; + use crate::dag_circuit::{DAGCircuit, Wire}; + use crate::imports::{CLASSICAL_REGISTER, MEASURE, QUANTUM_REGISTER}; use crate::operations::StandardGate; use crate::packed_instruction::{PackedInstruction, PackedOperation}; - use crate::Qubit; + use crate::{Clbit, Qubit}; + use ahash::HashSet; use pyo3::prelude::*; - use pyo3::Python; + use rustworkx_core::petgraph::prelude::*; + use rustworkx_core::petgraph::visit::IntoEdgeReferences; + // use pyo3::Python; fn new_dag(py: Python, qubits: u32, clbits: u32) -> DAGCircuit { let qreg = QUANTUM_REGISTER.get_bound(py).call1((qubits,)).unwrap(); @@ -6820,28 +6824,178 @@ mod test { dag } - #[test] - fn test_push_back() { - Python::with_gil(|py| { - let mut dag = new_dag(py, 4, 4); - let cx = PackedInstruction { + macro_rules! cx_gate { + ($dag:expr, $q0:expr, $q1:expr) => { + PackedInstruction { op: PackedOperation::from_standard(StandardGate::CXGate), - qubits: dag.qargs_interner.insert_owned(vec![Qubit(0), Qubit(1)]), - clbits: dag.cargs_interner.get_default(), + qubits: $dag + .qargs_interner + .insert_owned(vec![Qubit($q0), Qubit($q1)]), + clbits: $dag.cargs_interner.get_default(), params: None, extra_attrs: Default::default(), #[cfg(feature = "cache_pygates")] py_op: Default::default(), - }; - let cx_node = dag.push_back(py, cx).unwrap(); + } + }; + } + + macro_rules! measure { + ($py:expr, $dag:expr, $qarg:expr, $carg:expr) => {{ + // Python::with_gil(|py| { + let py_op = MEASURE.get_bound($py).call0().unwrap(); + let op_from_py: OperationFromPython = py_op.extract().unwrap(); + let qubits = $dag.qargs_interner.insert_owned(vec![Qubit($qarg)]); + let clbits = $dag.cargs_interner.insert_owned(vec![Clbit($qarg)]); + PackedInstruction { + op: op_from_py.operation, + qubits, + clbits, + params: Some(Box::new(op_from_py.params)), + extra_attrs: op_from_py.extra_attrs, + #[cfg(feature = "cache_pygates")] + py_op: Default::default(), + } + // }) + }}; + } + + #[test] + fn test_push_back() -> PyResult<()> { + Python::with_gil(|py| { + let mut dag = new_dag(py, 2, 2); + + // IO nodes. + let [q0_in_node, q0_out_node] = dag.qubit_io_map[0]; + let [q1_in_node, q1_out_node] = dag.qubit_io_map[1]; + let [c0_in_node, c0_out_node] = dag.clbit_io_map[0]; + let [c1_in_node, c1_out_node] = dag.clbit_io_map[1]; + + // Add a CX to the otherwise empty circuit. + let cx = cx_gate!(dag, 0, 1); + let cx_node = dag.push_back(py, cx)?; + assert!(matches!(dag.op_names.get("cx"), Some(1))); + + let expected_wires = HashSet::from_iter([ + (q0_in_node, cx_node, Wire::Qubit(Qubit(0))), + (cx_node, q0_out_node, Wire::Qubit(Qubit(0))), + (q1_in_node, cx_node, Wire::Qubit(Qubit(1))), + (cx_node, q1_out_node, Wire::Qubit(Qubit(1))), + // No clbits used, so in goes straight to out. + (c0_in_node, c0_out_node, Wire::Clbit(Clbit(0))), + (c1_in_node, c1_out_node, Wire::Clbit(Clbit(1))), + ]); + + let actual_wires: HashSet<_> = dag + .dag + .edge_references() + .map(|e| (e.source(), e.target(), e.weight().clone())) + .collect(); + + assert_eq!(actual_wires, expected_wires, "unexpected DAG structure"); + + // Add measures after CX. + let measure_q0 = measure!(py, dag, 0, 0); + let measure_q0_node = dag.push_back(py, measure_q0)?; + + let measure_q1 = measure!(py, dag, 1, 1); + let measure_q1_node = dag.push_back(py, measure_q1)?; + + let expected_wires = HashSet::from_iter([ + // q0In -> CX -> M -> q0Out + (q0_in_node, cx_node, Wire::Qubit(Qubit(0))), + (cx_node, measure_q0_node, Wire::Qubit(Qubit(0))), + (measure_q0_node, q0_out_node, Wire::Qubit(Qubit(0))), + // q1In -> CX -> M -> q1Out + (q1_in_node, cx_node, Wire::Qubit(Qubit(1))), + (cx_node, measure_q1_node, Wire::Qubit(Qubit(1))), + (measure_q1_node, q1_out_node, Wire::Qubit(Qubit(1))), + // c0In -> M -> c0Out + (c0_in_node, measure_q0_node, Wire::Clbit(Clbit(0))), + (measure_q0_node, c0_out_node, Wire::Clbit(Clbit(0))), + // c1In -> M -> c1Out + (c1_in_node, measure_q1_node, Wire::Clbit(Clbit(1))), + (measure_q1_node, c1_out_node, Wire::Clbit(Clbit(1))), + ]); + + let actual_wires: HashSet<_> = dag + .dag + .edge_references() + .map(|e| (e.source(), e.target(), e.weight().clone())) + .collect(); + assert_eq!(actual_wires, expected_wires, "unexpected DAG structure"); + Ok(()) + }) + } + + #[test] + fn test_push_front() -> PyResult<()> { + Python::with_gil(|py| { + let mut dag = new_dag(py, 2, 2); + + // IO nodes. let [q0_in_node, q0_out_node] = dag.qubit_io_map[0]; let [q1_in_node, q1_out_node] = dag.qubit_io_map[1]; + let [c0_in_node, c0_out_node] = dag.clbit_io_map[0]; + let [c1_in_node, c1_out_node] = dag.clbit_io_map[1]; + + // Add measures first (we'll add something before them afterwards). + let measure_q0 = measure!(py, dag, 0, 0); + let measure_q0_node = dag.push_back(py, measure_q0)?; + + let measure_q1 = measure!(py, dag, 1, 1); + let measure_q1_node = dag.push_back(py, measure_q1)?; + + let expected_wires = HashSet::from_iter([ + (q0_in_node, measure_q0_node, Wire::Qubit(Qubit(0))), + (measure_q0_node, q0_out_node, Wire::Qubit(Qubit(0))), + (q1_in_node, measure_q1_node, Wire::Qubit(Qubit(1))), + (measure_q1_node, q1_out_node, Wire::Qubit(Qubit(1))), + (c0_in_node, measure_q0_node, Wire::Clbit(Clbit(0))), + (measure_q0_node, c0_out_node, Wire::Clbit(Clbit(0))), + (c1_in_node, measure_q1_node, Wire::Clbit(Clbit(1))), + (measure_q1_node, c1_out_node, Wire::Clbit(Clbit(1))), + ]); + + let actual_wires: HashSet<_> = dag + .dag + .edge_references() + .map(|e| (e.source(), e.target(), e.weight().clone())) + .collect(); - assert!(dag.dag.find_edge(q0_in_node, cx_node).is_some()); - assert!(dag.dag.find_edge(q1_in_node, cx_node).is_some()); - assert!(dag.dag.find_edge(cx_node, q0_out_node).is_some()); - assert!(dag.dag.find_edge(cx_node, q1_out_node).is_some()); - }); + assert_eq!(actual_wires, expected_wires); + + // Add a CX before the measures. + let cx = cx_gate!(dag, 0, 1); + let cx_node = dag.push_front(py, cx)?; + assert!(matches!(dag.op_names.get("cx"), Some(1))); + + let expected_wires = HashSet::from_iter([ + // q0In -> CX -> M -> q0Out + (q0_in_node, cx_node, Wire::Qubit(Qubit(0))), + (cx_node, measure_q0_node, Wire::Qubit(Qubit(0))), + (measure_q0_node, q0_out_node, Wire::Qubit(Qubit(0))), + // q1In -> CX -> M -> q1Out + (q1_in_node, cx_node, Wire::Qubit(Qubit(1))), + (cx_node, measure_q1_node, Wire::Qubit(Qubit(1))), + (measure_q1_node, q1_out_node, Wire::Qubit(Qubit(1))), + // c0In -> M -> c0Out + (c0_in_node, measure_q0_node, Wire::Clbit(Clbit(0))), + (measure_q0_node, c0_out_node, Wire::Clbit(Clbit(0))), + // c1In -> M -> c1Out + (c1_in_node, measure_q1_node, Wire::Clbit(Clbit(1))), + (measure_q1_node, c1_out_node, Wire::Clbit(Clbit(1))), + ]); + + let actual_wires: HashSet<_> = dag + .dag + .edge_references() + .map(|e| (e.source(), e.target(), e.weight().clone())) + .collect(); + + assert_eq!(actual_wires, expected_wires, "unexpected DAG structure"); + Ok(()) + }) } } diff --git a/crates/circuit/src/imports.rs b/crates/circuit/src/imports.rs index 5c77f8ef7d17..0a0ca9624898 100644 --- a/crates/circuit/src/imports.rs +++ b/crates/circuit/src/imports.rs @@ -110,6 +110,7 @@ pub static FOR_LOOP_OP_CHECK: ImportOnceCell = ImportOnceCell::new("qiskit.dagcircuit.dagnode", "_for_loop_eq"); pub static UUID: ImportOnceCell = ImportOnceCell::new("uuid", "UUID"); pub static BARRIER: ImportOnceCell = ImportOnceCell::new("qiskit.circuit", "Barrier"); +pub static MEASURE: ImportOnceCell = ImportOnceCell::new("qiskit.circuit", "Measure"); pub static UNITARY_GATE: ImportOnceCell = ImportOnceCell::new( "qiskit.circuit.library.generalized_gates.unitary", "UnitaryGate", From 578f01fdd1d6a0c50f18fa5521227796804a6b08 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Wed, 2 Oct 2024 18:43:54 -0400 Subject: [PATCH 10/17] Add tox rust env for Cargo tests. --- CONTRIBUTING.md | 23 +++++++++++++++-------- tox.ini | 8 ++++++++ 2 files changed, 23 insertions(+), 8 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index de60e392e1c3..1fc241bb59e2 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -592,18 +592,29 @@ mod tests { } ``` -Rust tests are run separately from the Python tests. To run them, do +Rust tests are run separately from the Python tests. The easiest way to run +them is via ``tox``, which creates an isolated venv and pre-installs ``qiskit`` +prior to running ``cargo test``: + +```bash +tox -erust +``` + +You can also execute them directly in your own virtual environment with these +commands (which is what the ``tox`` env is doing under the hood): ```bash python setup.py build_rust --release --inplace -cargo test --no-default-features +PYTHONUSERBASE="$VIRTUAL_ENV" cargo test --no-default-features ``` -The first command builds Qiskit from source (in release mode, but --debug is fine too), +The first command builds Qiskit (in release, but --debug is fine too) in editable mode, which ensures that Rust tests that interact with Qiskit's Python code actually use the latest Python code from your working directory. -The second command actually invokes the tests via Cargo. The ``-no-default-features`` +The second command actually invokes the tests via Cargo. The ``PYTHONUSERBASE`` +environment variable tells the embedded Python interpreter to look for packages +in your active virtual environment. The ``-no-default-features`` flag is used to compile an isolated test runner without building a linked CPython extension module (which would otherwise cause linker failures). @@ -630,10 +641,6 @@ mod tests { } ``` -To ensure that Rust tests are properly importing changes in Qiskit's Python code -from the working directory, be sure to build and install Qiskit from source prior -to running `cargo test --no-default-features`. - > [!IMPORTANT] > Note that we explicitly disable compilation of such tests when running with Miri, i.e. `#[cfg(not(miri))]`. This is necessary because Miri doesn't support the FFI diff --git a/tox.ini b/tox.ini index a0c5665695d2..1a122602032a 100644 --- a/tox.ini +++ b/tox.ini @@ -31,6 +31,14 @@ deps = commands = stestr run {posargs} +[testenv:rust] +basepython = python3 +setenv = + PYTHONUSERBASE={envdir} +allowlist_externals = cargo +commands = + cargo test --no-default-features + [testenv:lint] basepython = python3 # `pylint` will examine the source code, not the version that would otherwise be From 65a90d34391c5a08f759af299bc55680f153ef83 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Wed, 2 Oct 2024 18:48:50 -0400 Subject: [PATCH 11/17] Set Python user base in CI. --- .azure/test-linux.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.azure/test-linux.yml b/.azure/test-linux.yml index 9cba9a7d96c4..2be97d1d30ad 100644 --- a/.azure/test-linux.yml +++ b/.azure/test-linux.yml @@ -104,7 +104,7 @@ jobs: - bash: | source test-job/bin/activate python tools/report_numpy_state.py - cargo test --no-default-features + PYTHONUSERBASE="$VIRTUAL_ENV" cargo test --no-default-features env: # On Linux we link against `libpython` dynamically, but it isn't written into the rpath # of the test executable (I'm not 100% sure why ---Jake). It's easiest just to forcibly From d33060d33231b847348ea6066dcd523864fa8773 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Wed, 2 Oct 2024 19:24:26 -0400 Subject: [PATCH 12/17] Clean up. --- crates/circuit/src/dag_circuit.rs | 47 +++++++++++++++++-------------- 1 file changed, 26 insertions(+), 21 deletions(-) diff --git a/crates/circuit/src/dag_circuit.rs b/crates/circuit/src/dag_circuit.rs index 6eae1a16d6a9..010728b96897 100644 --- a/crates/circuit/src/dag_circuit.rs +++ b/crates/circuit/src/dag_circuit.rs @@ -6929,7 +6929,6 @@ mod test { use pyo3::prelude::*; use rustworkx_core::petgraph::prelude::*; use rustworkx_core::petgraph::visit::IntoEdgeReferences; - // use pyo3::Python; fn new_dag(py: Python, qubits: u32, clbits: u32) -> DAGCircuit { let qreg = QUANTUM_REGISTER.get_bound(py).call1((qubits,)).unwrap(); @@ -6957,22 +6956,22 @@ mod test { } macro_rules! measure { - ($py:expr, $dag:expr, $qarg:expr, $carg:expr) => {{ - // Python::with_gil(|py| { - let py_op = MEASURE.get_bound($py).call0().unwrap(); - let op_from_py: OperationFromPython = py_op.extract().unwrap(); - let qubits = $dag.qargs_interner.insert_owned(vec![Qubit($qarg)]); - let clbits = $dag.cargs_interner.insert_owned(vec![Clbit($qarg)]); - PackedInstruction { - op: op_from_py.operation, - qubits, - clbits, - params: Some(Box::new(op_from_py.params)), - extra_attrs: op_from_py.extra_attrs, - #[cfg(feature = "cache_pygates")] - py_op: Default::default(), - } - // }) + ($dag:expr, $qarg:expr, $carg:expr) => {{ + Python::with_gil(|py| { + let py_op = MEASURE.get_bound(py).call0().unwrap(); + let op_from_py: OperationFromPython = py_op.extract().unwrap(); + let qubits = $dag.qargs_interner.insert_owned(vec![Qubit($qarg)]); + let clbits = $dag.cargs_interner.insert_owned(vec![Clbit($qarg)]); + PackedInstruction { + op: op_from_py.operation, + qubits, + clbits, + params: Some(Box::new(op_from_py.params)), + extra_attrs: op_from_py.extra_attrs, + #[cfg(feature = "cache_pygates")] + py_op: Default::default(), + } + }) }}; } @@ -6993,8 +6992,10 @@ mod test { assert!(matches!(dag.op_names.get("cx"), Some(1))); let expected_wires = HashSet::from_iter([ + // q0In => CX => q0Out (q0_in_node, cx_node, Wire::Qubit(Qubit(0))), (cx_node, q0_out_node, Wire::Qubit(Qubit(0))), + // q1In => CX => q1Out (q1_in_node, cx_node, Wire::Qubit(Qubit(1))), (cx_node, q1_out_node, Wire::Qubit(Qubit(1))), // No clbits used, so in goes straight to out. @@ -7011,10 +7012,10 @@ mod test { assert_eq!(actual_wires, expected_wires, "unexpected DAG structure"); // Add measures after CX. - let measure_q0 = measure!(py, dag, 0, 0); + let measure_q0 = measure!(dag, 0, 0); let measure_q0_node = dag.push_back(py, measure_q0)?; - let measure_q1 = measure!(py, dag, 1, 1); + let measure_q1 = measure!(dag, 1, 1); let measure_q1_node = dag.push_back(py, measure_q1)?; let expected_wires = HashSet::from_iter([ @@ -7057,19 +7058,23 @@ mod test { let [c1_in_node, c1_out_node] = dag.clbit_io_map[1]; // Add measures first (we'll add something before them afterwards). - let measure_q0 = measure!(py, dag, 0, 0); + let measure_q0 = measure!(dag, 0, 0); let measure_q0_node = dag.push_back(py, measure_q0)?; - let measure_q1 = measure!(py, dag, 1, 1); + let measure_q1 = measure!(dag, 1, 1); let measure_q1_node = dag.push_back(py, measure_q1)?; let expected_wires = HashSet::from_iter([ + // q0In => M => q0Out (q0_in_node, measure_q0_node, Wire::Qubit(Qubit(0))), (measure_q0_node, q0_out_node, Wire::Qubit(Qubit(0))), + // q1In => M => q1Out (q1_in_node, measure_q1_node, Wire::Qubit(Qubit(1))), (measure_q1_node, q1_out_node, Wire::Qubit(Qubit(1))), + // c0In -> M -> c0Out (c0_in_node, measure_q0_node, Wire::Clbit(Clbit(0))), (measure_q0_node, c0_out_node, Wire::Clbit(Clbit(0))), + // c1In -> M -> c1Out (c1_in_node, measure_q1_node, Wire::Clbit(Clbit(1))), (measure_q1_node, c1_out_node, Wire::Clbit(Clbit(1))), ]); From 234b6a56e847914cfb75c6462489bb1900bb4ea1 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Thu, 17 Oct 2024 16:14:59 -0400 Subject: [PATCH 13/17] Address review comments. --- CONTRIBUTING.md | 21 ++++++++++++--------- tox.ini | 1 + 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 14fb6c74bdf4..1b79c62bf9c6 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -570,7 +570,7 @@ The bulk of this code is exercised heavily by our Python-based unit testing, but this coverage really only provides integration-level testing from the perspective of Rust. -To provide proper Rust unit testing, we use `cargo test`. Rust tests are +To provide Rust unit testing, we use `cargo test`. Rust tests are integrated directly into the Rust file being tested within a `tests` module. Functions decorated with `#[test]` within these modules are built and run as tests. @@ -589,28 +589,31 @@ For more detailed guidance on how to write Rust tests, you can refer to the Rust documentation's [guide on writing tests](https://doc.rust-lang.org/book/ch11-01-writing-tests.html). Rust tests are run separately from the Python tests. The easiest way to run -them is via ``tox``, which creates an isolated venv and pre-installs ``qiskit`` -prior to running ``cargo test``: +them is via `tox`, which creates an isolated venv and pre-installs `qiskit` +prior to running `cargo test`: ```bash tox -erust ``` -You can also execute them directly in your own virtual environment with these -commands (which is what the ``tox`` env is doing under the hood): +You can also execute them directly in your own virtual environment. If you haven't +done so already, [create a Python virtual environment](#set-up-a-python-venv) and +**_activate it_**. + +Then, run the following commands: ```bash -python setup.py build_rust --release --inplace +python setup.py build_rust --inplace PYTHONUSERBASE="$VIRTUAL_ENV" cargo test --no-default-features ``` -The first command builds Qiskit (in release, but --debug is fine too) in editable mode, +The first command builds Qiskit in editable mode, which ensures that Rust tests that interact with Qiskit's Python code actually use the latest Python code from your working directory. -The second command actually invokes the tests via Cargo. The ``PYTHONUSERBASE`` +The second command actually invokes the tests via Cargo. The `PYTHONUSERBASE` environment variable tells the embedded Python interpreter to look for packages -in your active virtual environment. The ``-no-default-features`` +in your active virtual environment. The `-no-default-features` flag is used to compile an isolated test runner without building a linked CPython extension module (which would otherwise cause linker failures). diff --git a/tox.ini b/tox.ini index f48851e847fa..f6d2596a559c 100644 --- a/tox.ini +++ b/tox.ini @@ -34,6 +34,7 @@ commands = [testenv:rust] basepython = python3 setenv = + RUST_DEBUG=1 PYTHONUSERBASE={envdir} allowlist_externals = cargo commands = From 6856adbb9dc006ef0ed85f7869fd15778b470b0a Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Mon, 21 Oct 2024 15:08:24 -0400 Subject: [PATCH 14/17] Set RUST_DEBUG in pkg env for Tox rust env. --- tox.ini | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/tox.ini b/tox.ini index f6d2596a559c..2ea5876b2b98 100644 --- a/tox.ini +++ b/tox.ini @@ -33,13 +33,21 @@ commands = [testenv:rust] basepython = python3 +package_env = .pkg-rust setenv = - RUST_DEBUG=1 PYTHONUSERBASE={envdir} allowlist_externals = cargo commands = cargo test --no-default-features +# This is a special package_env used by the 'rust' env above +# to force Qiskit's Rust code to build in debug mode. We do this +# to speed up linking and save dev time. Our Cargo tests should +# still run fast enough. +[testenv:.pkg-rust] +setenv = + RUST_DEBUG=1 + [testenv:lint] basepython = python3 # `pylint` will examine the source code, not the version that would otherwise be From 4813a7c057efe7cec1f49efe30cdbb71e4d96774 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Mon, 21 Oct 2024 16:08:08 -0400 Subject: [PATCH 15/17] Explain --skip-pkg-install option for faster run. --- CONTRIBUTING.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b79c62bf9c6..a458ce10c889 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -596,8 +596,16 @@ prior to running `cargo test`: tox -erust ``` -You can also execute them directly in your own virtual environment. If you haven't -done so already, [create a Python virtual environment](#set-up-a-python-venv) and +> [!TIP] +> If you've already built your changes (e.g. `python setup.py build_rust --release --inplace`), +> you can pass `--skip-pkg-install` when invoking `tox` to avoid a rebuild. This works because +> Python will instead find and use Qiskit from the current working directory (since we skipped +> its installation). + +#### Using a custom venv instead of `tox` + +If you're not using `tox`, you can also execute Cargo tests directly in your own virtual environment. +If you haven't done so already, [create a Python virtual environment](#set-up-a-python-venv) and **_activate it_**. Then, run the following commands: From f72fb1498dc2e83d5495fc556be129f5a0bd6fa9 Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Mon, 21 Oct 2024 16:23:11 -0400 Subject: [PATCH 16/17] Fix typo. --- CONTRIBUTING.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index a458ce10c889..ef1ef56bcaae 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -621,7 +621,7 @@ use the latest Python code from your working directory. The second command actually invokes the tests via Cargo. The `PYTHONUSERBASE` environment variable tells the embedded Python interpreter to look for packages -in your active virtual environment. The `-no-default-features` +in your active virtual environment. The `--no-default-features` flag is used to compile an isolated test runner without building a linked CPython extension module (which would otherwise cause linker failures). From 2bdee8dfab82e836ca1aa79d0e7fb3f5aa1265ec Mon Sep 17 00:00:00 2001 From: Kevin Hartman Date: Mon, 21 Oct 2024 16:39:20 -0400 Subject: [PATCH 17/17] Update TOC. --- CONTRIBUTING.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ef1ef56bcaae..64d6f38ed4fb 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -15,6 +15,11 @@ community in this goal. * [Changelog generation](#changelog-generation) * [Release notes](#release-notes) * [Testing](#testing) + * [Qiskit's Python test suite](#qiskits-python-test-suite) + * [Snapshot testing for visualizations](#snapshot-testing-for-visualizations) + * [Testing Rust components](#testing-rust-components) + * [Using a custom venv instead of tox](#using-a-custom-venv-instead-of-tox) + * [Calling Python from Rust tests](#calling-python-from-rust-tests) * [Style and Lint](#style-and-lint) * [Building API docs locally](#building-api-docs-locally) * [Troubleshooting docs builds](#troubleshooting-docs-builds) @@ -403,13 +408,15 @@ build all the documentation into `docs/_build/html` and the release notes in particular will be located at `docs/_build/html/release_notes.html` ## Testing - Once you've made a code change, it is important to verify that your change does not break any existing tests and that any new tests that you've added also run successfully. Before you open a new pull request for your change, -you'll want to run the test suite locally. +you'll want to run Qiskit's Python test suite (as well as its Rust-based +unit tests if you've modified native code). + +### Qiskit's Python test suite -The easiest way to run the test suite is to use +The easiest way to run Qiskit's Python test suite is to use [**tox**](https://tox.readthedocs.io/en/latest/#). You can install tox with pip: `pip install -U tox`. Tox provides several advantages, but the biggest one is that it builds an isolated virtualenv for running tests. This