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

Declare free-threaded support for PyModule #4588

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

Conversation

ngoldbaum
Copy link
Contributor

@ngoldbaum ngoldbaum commented Oct 1, 2024

Adds a supports_free_threaded argument to the pymodule macro and a supports_free_threaded method to PyModule.

Updates the tests and examples to mark that they support free-threading.

Removes UNSAFE_PYO3_BUILD_FREE_THREADED=1 from the build config.

@ngoldbaum
Copy link
Contributor Author

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.

@ngoldbaum
Copy link
Contributor Author

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.

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.

@ngoldbaum ngoldbaum marked this pull request as ready for review October 21, 2024 23:31
@ngoldbaum ngoldbaum changed the title WIP: declare free-threaded support in pymodule macro Declare free-threaded support for PyModule Oct 21, 2024
@@ -1,4 +1,4 @@
error: expected one of: `name`, `crate`, `module`, `submodule`
error: expected one of: `name`, `crate`, `module`, `submodule`, `supports_free_threaded`
Copy link
Contributor Author

@ngoldbaum ngoldbaum Oct 21, 2024

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.

Copy link
Member

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.

Copy link
Member

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)]?

Copy link
Member

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).

Copy link
Member

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)] ?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

@ngoldbaum
Copy link
Contributor Author

This probably needs some discussion in the new guide section on free-threading support.

pyo3-ffi/build.rs Outdated Show resolved Hide resolved
@ngoldbaum
Copy link
Contributor Author

woohoo! looks like tests are passing except for the codecov issue affecting all PRs right now.

@ngoldbaum ngoldbaum force-pushed the module-support branch 2 times, most recently from 599a58e to 8f86b9e Compare October 23, 2024 21:57
src/macros.rs Outdated Show resolved Hide resolved
src/types/module.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a 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/decorator/src/lib.rs Outdated Show resolved Hide resolved
examples/sequential/src/module.rs Outdated Show resolved Hide resolved
Comment on lines 35 to 45
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
Copy link
Member

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?

Copy link
Contributor Author

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.

pyo3-ffi/build.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/moduleobject.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/moduleobject.rs Show resolved Hide resolved
guide/src/free-threading.md Outdated Show resolved Hide resolved
Comment on lines 389 to 422

/// 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<()>;
Copy link
Member

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?

Copy link
Contributor Author

@ngoldbaum ngoldbaum Oct 25, 2024

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`
Copy link
Member

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.

@davidhewitt davidhewitt mentioned this pull request Oct 25, 2024
5 tasks
@ngoldbaum
Copy link
Contributor Author

ngoldbaum commented Oct 31, 2024

It occurred to me while working on something else that this PR should remove the UNSAFE_PYO3_BUILD_FREE_THREADED uses in the CI scripts a well.

EDIT: done

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.

4 participants