-
Notifications
You must be signed in to change notification settings - Fork 891
feat: implement multi-phase initialization (PEP-489) #5525
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
Conversation
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.
Thanks very much for picking this up, it's been a constant want to move forward with this but only so many things I can proceed on at once!
Have placed a full build label on the PR, so if you push any changes it'll trigger a full run. Last I recall, GraalPy testing was not working properly so that'll be what we need to understand before we can merge.
src/impl_/pymodule.rs
Outdated
| // TODO: remove this once we default to `gil_used = false` (i.e. assume free-threading | ||
| // support in extensions). This is purely a convenience so that users only need to | ||
| // annotate the top-level module. |
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.
I would be keen to switch the default to gil_used = false in PyO3 0.28, which would allow us to simplify here.
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.
I'll leave as is until I understand the graalpy failure, but it should be easy to drop the gil_used stuff out before final review
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.
c84e6e5 to
f056e32
Compare
f056e32 to
edb4193
Compare
|
Interesting errors, I think this is because of
&
And I doubt there are prebuilts for the esoteric triples happening here. I'll look into this locally |
|
I think those warnings are red herrings, we should be skipping tests for any packages which don't support graalpy. |
edb4193 to
d4e90fc
Compare
74b35bf to
b4e88e9
Compare
|
Found the issue, eventually (thanks strace): Not sure how we want to handle this @davidhewitt? |
|
Looks like graalpy supports these from v25.0.0 onwards: oracle/graalpython@9faded9 |
|
Aha great find! In that case I think we should drop support for GraalPy 3.11, it seems justified if it allows us to get a big fix in. That can be done as a separate PR and then we can rebase here. |
|
Okay, can you give me a quick rundown of what that entails? crate feature/cfgs, workflow bump? |
|
#5116 is probably a good example to look at |
|
I get a clean test suite locally with |
b7bb908 to
fc9dcee
Compare
|
Not sure how to deal with the |
84a512c to
cf095d8
Compare
src/impl_/pymodule.rs
Outdated
| // Because the array is c-style, the empty elements should be zeroed. | ||
| // `std::mem::zeroed()` requires msrv 1.75 for const | ||
| values: [ZEROED_SLOT; N], |
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.
we recently bumped msrv to 1.83
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.
Switch to mem::zeroed in 01b44ed, though I think the explicitness of ZEROED_SLOT is better, personally
This code was for user convenience when gil_used defaulted to true, but since we're changing that to false, it is no longer needed.
cf095d8 to
01b44ed
Compare
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.
Thanks very much for helping push this over the line! I am happy with this code as-is, and I'll follow up by writing migration notes for users both related to gil_used and for the change to multi-phase init.
PyO3 0.28 will probably be shipping around December, so perhaps with some more iteration we can figure out what's needed for users to (unsafely) opt-in to subinterpreter support in time.
(We have a lot of globals which are not subinterpreter safe, so it might be that we need to iterate gradually and accept it'll be sometime next year when the support lands.)
|
Thanks David! I'll push it for as long as I have time. I think next up is looking into how to support a public interface to module state, creating an inventory of the aforementioned globals, and then a plan for how we'll handle them. I expect module state will play a part in that plan. |
Description
This is a rebased version of #5142.
Within, we switch to using PEP-489 multi-phase initialization for pyo3 modules, over the previous single-phase initialization method.
This work lays the foundation for several major upsides for pyo3 (and consumers therein):
pymoduleinitialization via rusty wrappers around thePy_mod_execcallbackstatics when managingpymodulestateTo be clear, this PR does not implement these, but it is a necessary base step towards them.
Of note, we also switch to
gil_used = falseby default in this PR, which may require consumers to explicitly opt in, if theirpymoduleis not thread safe. I expect this to be a relatively rare occurrence given rust's stance on thread safety.