-
Notifications
You must be signed in to change notification settings - Fork 802
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
Conversation
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()) }; |
There was a problem hiding this comment.
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:
sys
is imported andsys.modules
assigned to a variable- the module is initialised
- 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 thepy
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.
CodSpeed Performance ReportMerging #4099 will not alter performanceComparing Summary
|
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 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`
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:
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 gatetesting
. This is enabled by default and included infull
.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 byPython::with_gil(|py| {...})
.Additionally the user can request for one or more modules and/or functions to be imported and exposed as
PyModule
andPyFunction
s. This is done by adding#[pyo3import(...)]
attributes to the testcase.Initialising and exposing the
PyModules
- unsafe codeblock + Python>=3.9When 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
:auto-initialize
then the interpreter is guaranteed to be initialised andappend_to_inittab!()
will fail - in this case all tests are guaranteed to panic.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
py_raises!
andcall!
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.#[pyo3test]
coverage to include classes.ToDos
bugfix problems pre-initiatized interpreteradd 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 resultsadd a "testing your exported code" section to "2. Using Rust from Python" with How-To guidanceadd 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 tocontributing.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.