Enable PyO3 in cargo unit tests. #13169
Conversation
|
One or more of the following people are relevant to this code:
|
Pull Request Test Coverage Report for Build 11673899398Details
💛 - Coveralls |
|
If you enable the |
kevinhartman
changed the title
There was a problem hiding this comment.
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.
| ```bash | ||
| tox -erust | ||
| ``` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
CONTRIBUTING.md
Outdated
| 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 was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point. I've reworded this section to guide users to first create and activate a virtual environment if they're running without tox.
CONTRIBUTING.md
Outdated
|
|
||
| ```bash | ||
| cargo test --no-default-features | ||
| python setup.py build_rust --release --inplace |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Should be done in 234b6a5 as well.
| [testenv:rust] | ||
| basepython = python3 | ||
| setenv = | ||
| PYTHONUSERBASE={envdir} |
There was a problem hiding this comment.
The default tox configuration will build in release mode, I think for rust testing we should use debug mode by default.
There was a problem hiding this comment.
Done in 6856adb.
|
This should be ready to go, @mtreinish. The Rust tests for |
Summary
At present, most of our core data structures like
DAGCircuitinternally 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:
auto-initialize, which initializes the free-threaded Python interpreter. This enables the use ofPython::with_giland thus access to aPythontoken from within tests.mirifor such tests, since it is not compatible (e.g.#[cfg(not(miri))]).PYTHONUSERBASEenvironment variable to the activevenv's directory. This allows the Python interpreter baked into the test executable to locate the Pythonqiskitmodule.toxenv has been added to orchestrate this. To run Rust tests, simply invoketox -erust.Details and comments
As an example, this PR adds unit tests for
DAGCircuit::push_backandDAGCircuit::push_front. We really ought to add more testing for the other new Rust-facing methods (e.g.DAGCircuit::extendvia 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.