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 all 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
99 changes: 85 additions & 14 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -565,11 +572,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 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)]
Expand All @@ -581,18 +592,78 @@ 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


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

```bash
cargo test --no-default-features
python setup.py build_rust --inplace
PYTHONUSERBASE="$VIRTUAL_ENV" 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 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`
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.
})
}
}
```

> [!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 @@ -60,5 +60,8 @@ features = ["ndarray"]
version = "0.18.22"
features = ["macro"]

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

[features]
cache_pygates = ["qiskit-circuit/cache_pygates"]
3 changes: 3 additions & 0 deletions crates/circuit/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,6 @@ features = ["union"]

[features]
cache_pygates = []

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