Enable PyO3 in cargo unit tests. #13169

kevinhartman · 2024-09-17T17:12:58Z

Summary

At present, most of our core data structures like DAGCircuit internally rely on the Python interpreter in some way, which has so far prevented us from writing Rust-based unit tests. Up to this point, we've gotten away with depending on our Python-based test coverage to exercise Rust code internally, but this is quickly becoming insufficient as we add more Rust-only API surface.

This PR does a few things to enable the use of the Python interpreter and load Qiskit's Python code into it:

The test executable is built with PyO3 feature auto-initialize, which initializes the free-threaded Python interpreter. This enables the use of Python::with_gil and thus access to a Python token from within tests.
We disable miri for such tests, since it is not compatible (e.g. #[cfg(not(miri))]).
We set the PYTHONUSERBASE environment variable to the active venv's directory. This allows the Python interpreter baked into the test executable to locate the Python qiskit module.
A new tox env has been added to orchestrate this. To run Rust tests, simply invoke tox -erust.

Details and comments

As an example, this PR adds unit tests for DAGCircuit::push_back and DAGCircuit::push_front. We really ought to add more testing for the other new Rust-facing methods (e.g. DAGCircuit::extend via additional PRs).

As more of Qiskit's core is ported to Rust, our reliance on the Python interpreter from Rust code should eventually be overcome, and at that point we shouldn't need these changes anymore.

qiskit-bot · 2024-09-17T17:13:02Z

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

@Qiskit/terra-core
@kevinhartman
@mtreinish

coveralls · 2024-09-17T17:41:31Z

Pull Request Test Coverage Report for Build 12795511628

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

0 of 0 changed or added relevant lines in 0 files are covered.
8 unchanged lines in 3 files lost coverage.
Overall coverage increased (+0.002%) to 88.912%

Files with Coverage Reduction	New Missed Lines	%
crates/qasm2/src/lex.rs	1	92.98%
crates/qasm2/src/expr.rs	1	94.02%
crates/qasm2/src/parse.rs	6	96.69%

Totals
Change from base Build 12771449472:	0.002%
Covered Lines:	79432
Relevant Lines:	89338

💛 - Coveralls

jakelishman · 2024-09-17T20:50:32Z

If you enable the pyo3/auto-initialize feature during the test-suite build, then the pyo3::prepare_freethreaded_python calls won't be necessary. Don't know if you had other work around the test harness planned, though.

mtreinish

Overall this looks great to me, I really am looking forward to having an established pattern for doing more involved rust testing. Especially as we're moving more into rust. I left a few inline comments, mostly nits in the docs and a question in the tests. The only concern I have right now is that we're defaulting to building the python extension in release mode for rust testing and I feel like we should be defaulting to debug mode for the extension when in a rust testing context. Not because I think we need the debug symbols or optimizations disabled, but just to reduce the compile time since the extension is only there to make python work and we shouldn't really be concerned with the runtime performance of it in a rust testing context.

CONTRIBUTING.md

mtreinish · 2024-10-09T13:45:09Z

CONTRIBUTING.md

+```bash
+tox -erust
+```


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.

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

mtreinish · 2024-10-09T13:46:39Z

CONTRIBUTING.md

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


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.

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

CONTRIBUTING.md

mtreinish · 2024-10-09T13:51:11Z

CONTRIBUTING.md


 ```bash
-cargo test --no-default-features
+python setup.py build_rust --release --inplace


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.

Should be done in 234b6a5 as well.

mtreinish · 2024-10-09T13:54:41Z

tox.ini

+[testenv:rust]
+basepython = python3
+setenv =
+  PYTHONUSERBASE={envdir}


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

Done in 6856adb.

crates/circuit/src/dag_circuit.rs

kevinhartman · 2024-11-05T14:46:59Z

This should be ready to go, @mtreinish.

The Rust tests for DAGCircuit added in the PR can be seen passing in CI here: https://dev.azure.com/qiskit-ci/qiskit/_build/results?buildId=62018&view=logs&j=bf26c3d0-9b88-5f67-96f4-43488114ea6d&t=5c74c3c4-ab20-5140-0e04-11aec0505a88&l=148

eliarbel · 2024-11-27T15:55:42Z

CONTRIBUTING.md


 ```bash
-cargo test --no-default-features
+python setup.py build_rust --inplace
+PYTHONUSERBASE="$VIRTUAL_ENV" cargo test --no-default-features


Overall this PR is great, long needed for running Rust tests easily with PyO3 in Qiskit!

My only comment is about virtual environments other than venv. For Conda I had to set LD_LIBRARY_PATH to point to the lib directory under the env dir so the embedded interpreter could find the required shared libraries. I don't think we should provide examples for other custom virtual envs, but rather just put a [!note] comment that for other virtual envs a different setup might be required (and maybe mention LD_LIBRARY_PATH for Conda since it's common).

Thanks for trying this on Conda. Any chance you could try the following instead of LD_LIBRARY_PATH?

PYTHONUSERBASE="$CONDA_PREFIX" cargo test --no-default-features

This was actually my first attempt to make it work in Conda, following the nice documentation you put together. But it doesn't help. I didn't dig into the actual root cause but it surely something about the embedded interpreter not being aware to the virtual environment if it's Conda. I wouldn't put time solving this but rather just include a warning in this section for users to note.

I'd forgotten that on Linux, the rpath of the test executable doesn't include the Python library. To work around this, the rust tox env now exports LD_LIBRARY_PATH to include the path to the current Python environment's shared Python lib before running cargo test. This is similar to what we do in CI.

Can you try running tox -erust on Linux again with these changes? I've also updated the docs for manual invocations without tox.

I can confirm it's working now. Thanks @kevinhartman!

mtreinish

LGTM, thanks for doing this

* POC for using PyO3 in cargo unit tests. * Reorder jobs in test-linux.yml. * Actually use the venv... * Disable Miri for PyO3-based cargo tests. * Fix disable for module-level. * Improve comment in Rust test runner CI job. * Use pyo3/auto-initialize feature in test. * Update contibuting guide. * Testing for push_back and push_front. * Add tox rust env for Cargo tests. * Set Python user base in CI. * Clean up. * Address review comments. * Set RUST_DEBUG in pkg env for Tox rust env. * Explain --skip-pkg-install option for faster run. * Fix typo. * Update TOC. * Add Python lib to LD_LIBRARY_PATH in tox.

POC for using PyO3 in cargo unit tests.

ba48d55

kevinhartman requested a review from a team as a code owner September 17, 2024 17:12

kevinhartman added 4 commits September 17, 2024 16:14

Reorder jobs in test-linux.yml.

4425ac1

Actually use the venv...

63501bc

Disable Miri for PyO3-based cargo tests.

2fd982f

Fix disable for module-level.

1b594c1

kevinhartman added 2 commits September 17, 2024 16:54

Improve comment in Rust test runner CI job.

d5d4714

Use pyo3/auto-initialize feature in test.

63d6334

mtreinish mentioned this pull request Sep 19, 2024

Add inverse function to StandardGate in rust #13168

Merged

kevinhartman added 3 commits September 20, 2024 17:28

Update contibuting guide.

f0925e7

Testing for push_back and push_front.

86ec338

Add tox rust env for Cargo tests.

578f01f

kevinhartman changed the title ~~[WIP] Proof of concept for using PyO3 in cargo unit tests.~~ Enable PyO3 in cargo unit tests. Oct 2, 2024

kevinhartman added 3 commits October 2, 2024 18:48

Set Python user base in CI.

65a90d3

Merge branch 'main' of github.com:Qiskit/qiskit into cargo-test-dag

00c9158

Clean up.

d33060d

mtreinish reviewed Oct 9, 2024

View reviewed changes

kevinhartman added 3 commits October 17, 2024 16:14

Address review comments.

234b6a5

Set RUST_DEBUG in pkg env for Tox rust env.

6856adb

Explain --skip-pkg-install option for faster run.

4813a7c

kevinhartman requested a review from mtreinish October 21, 2024 20:20

kevinhartman added 3 commits October 21, 2024 16:23

Fix typo.

f72fb14

Update TOC.

2bdee8d

Merge branch 'main' of github.com:Qiskit/qiskit into cargo-test-dag

d648af6

eliarbel reviewed Nov 27, 2024

View reviewed changes

Merge branch 'main' of github.com:Qiskit/qiskit into cargo-test-dag

739195f

Add Python lib to LD_LIBRARY_PATH in tox.

3d32aca

jakelishman mentioned this pull request Jan 22, 2025

Split PySparseObservable off SparseObservable #13595

Merged

Cryoris mentioned this pull request Jan 23, 2025

Additional PySparseObservable tests #13726

Open

mtreinish approved these changes Jan 27, 2025

View reviewed changes

mtreinish added this pull request to the merge queue Jan 27, 2025

Merged via the queue into Qiskit:main with commit cc7ac53 Jan 27, 2025
17 checks passed

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

Enable PyO3 in cargo unit tests. #13169

Enable PyO3 in cargo unit tests. #13169

Uh oh!

Conversation

kevinhartman commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Details and comments

Uh oh!

qiskit-bot commented Sep 17, 2024

Uh oh!

coveralls commented Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 12795511628

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

Uh oh!

jakelishman commented Sep 17, 2024

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kevinhartman commented Nov 5, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mtreinish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kevinhartman commented Sep 17, 2024 •

edited

Loading

coveralls commented Sep 17, 2024 •

edited

Loading