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

docs: Expand docs on when and why allow_threads is necessary #4767

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

Conversation

ngoldbaum
Copy link
Contributor

This is a response to the confusion encountered in #4738

I verified that the example I added in parallelism.md runs sucessfully despite the fact that it's decorated with rust,no_run.

@ngoldbaum ngoldbaum force-pushed the expand-allow-threads-docs branch from 8b47a5d to c2bb87d Compare December 5, 2024 18:11
Copy link
Contributor Author

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

noticed a grammar issue on my phone

guide/src/parallelism.md Outdated Show resolved Hide resolved
guide/src/parallelism.md Outdated Show resolved Hide resolved
@ngoldbaum ngoldbaum force-pushed the expand-allow-threads-docs branch from 615d8f2 to b71acef Compare December 11, 2024 18:47
Copy link
Member

@mejrs mejrs left a comment

Choose a reason for hiding this comment

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

When I read this next to https://docs.python.org/3.13/c-api/init.html I notice there's a mismatch in the terminology used. For example this documentation uses (and used) "attach" and "detach" a lot, but the C api docs don't - they talk about setting, resetting and associating threadstate. I think it would be clearer if we used the same terminology.

Otherwise it looks great, thank you!

guide/src/free-threading.md Outdated Show resolved Hide resolved
Comment on lines 160 to 166
objects or call into the CPython C API. If you are not yet attached to the
Python runtime, you can register a thread using the [`Python::with_gil`]
function. Threads created via the Python [`threading`] module do not not need to
do this, but all other OS threads that interact with the Python runtime must
explicitly attach using `with_gil` and obtain a `'py` liftime.

Since there is no GIL in the free-threaded build, releasing the GIL for
long-running tasks is no longer necessary to ensure other threads run, but you
should still detach from the interpreter runtime using [`Python::allow_threads`]
when doing long-running tasks that do not require the CPython runtime. The
garbage collector can only run if all threads are detached from the runtime (in
a stop-the-world state), so detaching from the runtime allows freeing unused
memory.
do this, and pyo3 will handle setting up the [`Python<'py>`] token when CPython
calls into your extension, but all other OS threads that interact with the
Python runtime must explicitly attach using `with_gil` and obtain a `'py`
liftime.
Copy link
Member

Choose a reason for hiding this comment

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

This is a lot of words to say "you still need to call with_gil and use Python, to ensure that the current thread has a Python threadstate associated with it". Can we pare it down?

Co-authored-by: Bruno Kolenbrander <[email protected]>
@ngoldbaum
Copy link
Contributor Author

We intentionally went with the attach/detach terminology in #4577 (see e.g. #4577 (review)). Let me see if I can make it make sense using "save/restore a thread state"...

@ngoldbaum
Copy link
Contributor Author

I just asked about this issue on the CPython discussion forum: https://discuss.python.org/t/c-api-docs-for-free-threading/74183. IMO the CPython docs need free-threading specific copy here, and the terminology we're using about attaching and detaching from the runtime (IMO) makes more sense when thinking about the free-threaded build.

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.

3 participants