-
Notifications
You must be signed in to change notification settings - Fork 757
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
Declare free-threaded support for PyModule #4588
base: main
Are you sure you want to change the base?
Conversation
Just a note that this should probably only be merged once all the "RuntimeWarning: The global interpreter lock (GIL) has been enabled to load module foo" warnings in the python tests are fixed in the free-threaded build. |
6834f3f
to
1bd1338
Compare
This is done now and I think all the tests are passing so I'm going to move this out of draft phase and update the PR description. |
@@ -1,4 +1,4 @@ | |||
error: expected one of: `name`, `crate`, `module`, `submodule` | |||
error: expected one of: `name`, `crate`, `module`, `submodule`, `supports_free_threaded` |
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.
bikeshed opportunity: should I call this supports_free_threaded
or something else? supports_free_threading
is one more character but also feels less weird grammatically maybe?
I could also use GIL in the name since the relevant C API uses names like GIL_NOT_USED
and GIL_USED
.
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.
supports_free_threading
seems slightly better to me; I think in the long run it will be supports_free_threading = false
to opt-out, and the default won't need the argument, so longer but clearer seems the obvious choice to me.
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.
What about just #[pymodule(free_threaded)]
?
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 guess to me that implies that it's always free-threaded (which wouldn't be true on a GIL-enabled Python).
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.
Maybe #[pymodule(requires_gil = false)]
?
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.
How about pymodule(gil_used)
, since the names of the Py_mod_gil
slots that indicates an extension supports free-threading are Py_MOD_GIL_NOT_USED
and Py_MOD_GIL_USED
. With gil_used
users are maybe somewhat more likely to link this with upstream documentation that is probably also useful for determining what they're supposed to do.
I think using gil
in the name is fine since this is a low-level concern directly related to whether or not code is making strong assumptions about the GIL.
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.
In the latest version of this PR I renamed it to gil_used = false
. In the long run that means users will do pymodule(gil_used)
and otherwise not need to think about it.
This probably needs some discussion in the new guide section on free-threading support. |
11f2a12
to
3021c46
Compare
woohoo! looks like tests are passing except for the codecov issue affecting all PRs right now. |
599a58e
to
8f86b9e
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, broadly looks great to me, a bunch of various thoughts :)
examples/string-sum/src/lib.rs
Outdated
let module = PyModule_Create(ptr::addr_of_mut!(MODULE_DEF)); | ||
if module.is_null() { | ||
return module; | ||
} | ||
#[cfg(Py_GIL_DISABLED)] | ||
{ | ||
if PyUnstable_Module_SetGIL(module, Py_MOD_GIL_NOT_USED) < 0 { | ||
return std::ptr::null_mut(); | ||
} | ||
} | ||
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.
Would it be better to add the declaration to m_slots
above, in this "raw ffi" case?
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.
using PyModule_Create
implies that we're using single-phase initialization, which means we can't set the slot directly:
PyModuleDef_Slot *m_slots
An array of slot definitions for multi-phase initialization, terminated by a {0, NULL} entry. When using single-phase initialization, m_slots must be NULL.
src/types/module.rs
Outdated
|
||
/// Declare whether or not this module supports running with the GIL disabled | ||
/// | ||
/// If the module does not rely on the GIL for thread safety, you can pass True | ||
/// to this function to indicate the module does not rely on the GIL for | ||
/// thread-safety. | ||
/// | ||
/// This function sets the [`Py_MOD_GIL` | ||
/// slot](https://docs.python.org/3/c-api/module.html#c.Py_mod_gil) on the | ||
/// module object. The default is `Py_MOD_GIL_USED`, so passing `false` to | ||
/// this function is a no-op unless you have already set `Py_MOD_GIL` to | ||
/// `Py_MOD_GIL_NOT_USED` elsewhere. | ||
/// | ||
/// # Examples | ||
/// | ||
/// ```rust | ||
/// use pyo3::prelude::*; | ||
/// | ||
/// #[pymodule(supports_free_threaded = true)] | ||
/// fn my_module(py: Python<'_>, module: &Bound<'_, PyModule>) -> PyResult<()> { | ||
/// let submodule = PyModule::new(py, "submodule")?; | ||
/// submodule.supports_free_threaded(true)?; | ||
/// module.add_submodule(&submodule)?; | ||
/// Ok(()) | ||
/// } | ||
/// ``` | ||
/// | ||
/// The resulting module will not print a `RuntimeWarning` and re-enable the | ||
/// GIL when Python imports it on the free-threaded build, since all module | ||
/// objects defined in the extension have `Py_MOD_GIL` set to | ||
/// `Py_MOD_GIL_NOT_USED`. | ||
/// | ||
/// This is a no-op on the GIL-enabled build. | ||
fn supports_free_threaded(&self, supports_free_threaded: bool) -> PyResult<()>; |
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'm unsure about this one; I think long term I want to split functions what are to do with "module building" from the PyModuleMethods
which I'd think should be for "normal" runtime operations on modules.
Was there a particular use case which needed this?
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 need this to set the Py_mod_gil
slot on submodules that get created without the proc macro.
@@ -1,4 +1,4 @@ | |||
error: expected one of: `name`, `crate`, `module`, `submodule` | |||
error: expected one of: `name`, `crate`, `module`, `submodule`, `supports_free_threaded` |
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.
supports_free_threading
seems slightly better to me; I think in the long run it will be supports_free_threading = false
to opt-out, and the default won't need the argument, so longer but clearer seems the obvious choice to me.
28d07c0
to
39ffe29
Compare
It occurred to me while working on something else that this PR should remove the EDIT: done |
356b2ac
to
a6f59f3
Compare
Adds a
supports_free_threaded
argument to thepymodule
macro and asupports_free_threaded
method toPyModule
.Updates the tests and examples to mark that they support free-threading.
Removes
UNSAFE_PYO3_BUILD_FREE_THREADED=1
from the build config.