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

pyo3-testing: initial proc_macro to make testing pyo3functions easier #4099

Open
wants to merge 112 commits into
base: main
Choose a base branch
from

Conversation

MusicalNinjaDad
Copy link

@MusicalNinjaDad MusicalNinjaDad commented Apr 19, 2024

Overview

Add #[pyo3test] to support easier testing of code wrapped by #[pyfunction]. See section 2.4 of the Guide: "Testing your code" for more details.

Concept

The idea is to make it easy for people to run final tests of their exported functions in rust (step 2 below). The test approach supported and documented in the Guide (new section 2.4) is:

  1. test your functionality in rust, by implementing it as pure rust functions and directly unit testing them.
  2. wrap those rust functions with #[pyo3...] and run simple unit tests in rust to validate the wrapping, that you got the type conversions and error handling correct etc.
  3. build your python project and run python tests with pytest or unittest on the final codebase.
    See also the original discussion in Proc Macros to support and simplify testing #3984

Implementation Details

This PR introduces a new crate pyo3-testing which is placed behind a feature gate testing. This is enabled by default and included in full.

The crate provides a single proc macro #[pyo3test] and also contains the implementation functionality and unit tests. As these are not made public, they can coexist in the same crate for simplicity (no additional "-backend" or "-procmacros" crate is needed).

At its simplest the macro takes a function (the "testcase") adds a #[test] attribute, initialises the interpreter and executes the function body wrapped by Python::with_gil(|py| {...}).

Additionally the user can request for one or more modules and/or functions to be imported and exposed as PyModule and PyFunctions. This is done by adding #[pyo3import(...)] attributes to the testcase.

Initialising and exposing the PyModules - unsafe codeblock + Python>=3.9

When running multiple tests on wrapped modules, interpreter initialisation becomes a problem. This issue will be experienced by anyone who is trying to test their wrapped functions in rust, whether they create the entire testcase themselves or use pyo3-testing:

  • If the tests are run with auto-initialize then the interpreter is guaranteed to be initialised and append_to_inittab!() will fail - in this case all tests are guaranteed to panic.
  • If the tests are run without auto-initialize then there is the risk that the interpreter will already be initialized by a different test - in this case the tests are simply flaky and will panic at random.

I experienced this issue continually with different fail points but no easy way to guarantee failure.

The solution to this involves a small unsafe block and is only possible on cPython >=3.9 - I will add a specific comment to that code with an explanation and to allow for a more focussed discussion / review.

Ideas for extending in future PRs

  • add py_raises! and call! macros to make it easier to write testcases. If these are macro-rules! macros then this will likely mean creating a pyo3_testing_procmacros crate, moving the current code there and re-exporting it from pyo3_testing in order to maintain the current API. For now working on the assumption of YAGNI and avoiding the additional complexity of a new crate, given that this should not require any breaking changes going forwards.
  • extend #[pyo3test] coverage to include classes.
  • add a cfg option and document a github action example to allow easy skipping of pyo3tests in CI for unsupported python versions.

ToDos

  • bugfix problems pre-initiatized interpreter
  • add dedicated comment to the module initialisation, resolve the current comment on that topic.
  • raise a separate PR for the additional github workflow, so it no longer shows up here as a change (where it doesn't really belong)
  • create UI tests for error handling proc_macros & potentially improve error handling based on the results
  • add a "testing your exported code" section to "2. Using Rust from Python" with How-To guidance
  • add an entry for this PR in newsfragments - see [https://pyo3.rs/main/contributing.html#documenting-changes]

Note

What would you like me to do about .github/workflows/ci-quick.yml? I've raised #4115 for that and a few updates to contributing.md. If you would like me to remove it from here I will do so when the review is complete as a final step (otherwise I run into issues with testing my code for any additional changes). If the other PR ik OK and merged before this then leaving it here is a no-op.

let sys = PyModule::import_bound(py, "sys").unwrap();
let py_modules: Bound<'_, PyDict> =
sys.getattr("modules").unwrap().downcast_into().unwrap();
let py_adders_pymodule = unsafe { Bound::from_owned_ptr(py, py_adders::__pyo3_init()) };
Copy link
Author

Choose a reason for hiding this comment

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

This is an expanded example of how modules are initialised and exposed in a pre-initialised interpreter. It is based on the example in the Guide section 3.4:

  1. sys is imported and sys.modules assigned to a variable
  2. the module is initialised
  3. the module is inserted into sys.modules

I believe that the lifetime of py_adders_pymodule is safe as:

  • py_adders::__pyo3_init() is called with the GIL lock in place.
  • Therefore the GIL handler will hand out the same 'py lifetime as the rest of the code is using.
  • Bound::from_owned_ptr() is passed the py token from this GIL lock, binding the initialised module to the correct lock
  • and the whole process is performed as a single action.

Have I correctly understood how the GIL lock is handled w.r.t. the lifetime of the PyModule created by __pyo3_init?

append_to_inittab! is not possible here, because there is no guarantee that the interpreter is not already initialised.

This sadly also means that compatibility is limited to cPython>=3.9 if I understand make_module correctly.

Copy link

codspeed-hq bot commented Apr 24, 2024

CodSpeed Performance Report

Merging #4099 will not alter performance

Comparing MusicalNinjaDad:pyo3-testing (48a29ad) with main (7263fa9)

Summary

✅ 69 untouched benchmarks

@MusicalNinjaDad MusicalNinjaDad marked this pull request as ready for review April 24, 2024 14:52
@MusicalNinjaDad
Copy link
Author

MusicalNinjaDad commented Apr 24, 2024

Hi @davidhewitt

This is now ready for review in my opinion, there are a few additional ideas which I have documented at the end of the PR. I would be happy to continue working on this topic and support the pyo3-testing functionality if you are interested in including it.

I also understand that this is quite a large PR and you are busy. I hope it is worth your time when you do have a chance to look at it. If there is anything I can do to help make reviewing easier, please just let me know. Please also don't feel like you have to provide comments on everything at once, I'm happy to take any feedback in pieces if that suits you better.

`ImportError: PyO3 modules compiled for CPython 3.8 or older may only be initialized once per interpreter process`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants