Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable PyO3 in cargo unit tests. #13169

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 16 additions & 11 deletions .azure/test-linux.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -107,6 +96,22 @@ jobs:
sudo apt-get install -y graphviz
displayName: 'Install optional non-Python dependencies'

# 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.
- bash: |
source test-job/bin/activate
python tools/report_numpy_state.py
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
# 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
Expand Down
75 changes: 64 additions & 11 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -565,11 +565,15 @@ Note: If you have run `test/ipynb/mpl_tester.ipynb` locally it is possible some

### Testing Rust components

Many Rust-accelerated functions are generally tested from Python space, but in cases
where new Rust-native APIs are being added, or there are Rust-specific internal details
to be tested, `#[test]` functions can be included inline. It's typically 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
kevinhartman marked this conversation as resolved.
Show resolved Hide resolved
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)]
Expand All @@ -581,18 +585,67 @@ mod tests {
}
```

For more detailed guidance on how to add Rust testing you can refer to the Rust
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).

To run the Rust-space tests, 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
```
Comment on lines +602 to +604
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only concern here is that this results in building Qiskit from source twice on every execution. I'm not sure there is a way around this since we need the python extension built to be able to call it via python in the rust tests it just is pretty slow. We should probably build in debug mode by default in tox and for rust testing by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've enabled debug mode by default for the rust env's package environment in 6856adb.

I also documented --skip-pkg-install as an option for running without rebuilding Qiskit when invoking tox -erust, and I've explained there that this is only an option if you've already built the current working tree: 4813a7c


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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a path to running without a virtual environment, but that's probably not good to encourage because it means people will be installing a dev version of qiskit in their system site packages. It might be good to add a sentence here explaining you should make a venv for testing in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I've reworded this section to guide users to first create and activate a virtual environment if they're running without tox.

234b6a5


```bash
cargo test --no-default-features
python setup.py build_rust --release --inplace
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd actually change this to not include --release by default. That just increases the build time and we shouldn't be bottlenecked on the python extension's execution time in rust tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be done in 234b6a5 as well.

PYTHONUSERBASE="$VIRTUAL_ENV" cargo test --no-default-features
```

The first command builds Qiskit (in release, but --debug is fine too) in editable mode,
kevinhartman marked this conversation as resolved.
Show resolved Hide resolved
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``
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).

#### 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.
})
}
}
```

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

Expand Down
3 changes: 3 additions & 0 deletions crates/accelerate/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -59,5 +59,8 @@ features = ["ndarray"]
version = "0.18.22"
features = ["macro"]

[dev-dependencies]
pyo3 = { workspace = true, features = ["auto-initialize"] }

[features]
cache_pygates = ["qiskit-circuit/cache_pygates"]
5 changes: 4 additions & 1 deletion crates/circuit/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -38,4 +38,7 @@ workspace = true
features = ["union"]

[features]
cache_pygates = []
cache_pygates = []

[dev-dependencies]
pyo3 = { workspace = true, features = ["auto-initialize"] }
204 changes: 204 additions & 0 deletions crates/circuit/src/dag_circuit.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6916,3 +6916,207 @@ fn add_global_phase(py: Python, phase: &Param, other: &Param) -> PyResult<Param>
}

type SortKeyType<'a> = (&'a [Qubit], &'a [Clbit]);

#[cfg(all(test, not(miri)))]
mod test {
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::{Clbit, Qubit};
use ahash::HashSet;
use pyo3::prelude::*;
use rustworkx_core::petgraph::prelude::*;
use rustworkx_core::petgraph::visit::IntoEdgeReferences;

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
}

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($q0), Qubit($q1)]),
clbits: $dag.cargs_interner.get_default(),
params: None,
extra_attrs: Default::default(),
#[cfg(feature = "cache_pygates")]
py_op: Default::default(),
}
};
}

macro_rules! measure {
($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([
// 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.
(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!(dag, 0, 0);
let measure_q0_node = dag.push_back(py, measure_q0)?;
mtreinish marked this conversation as resolved.
Show resolved Hide resolved

let measure_q1 = measure!(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!(dag, 0, 0);
let measure_q0_node = dag.push_back(py, measure_q0)?;

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))),
]);

let actual_wires: HashSet<_> = dag
.dag
.edge_references()
.map(|e| (e.source(), e.target(), e.weight().clone()))
.collect();

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(())
})
}
}
1 change: 1 addition & 0 deletions crates/circuit/src/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,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",
Expand Down
8 changes: 8 additions & 0 deletions tox.ini
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,14 @@ deps =
commands =
stestr run {posargs}

[testenv:rust]
basepython = python3
setenv =
PYTHONUSERBASE={envdir}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The default tox configuration will build in release mode, I think for rust testing we should use debug mode by default.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6856adb.

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
Expand Down
Loading