-
Notifications
You must be signed in to change notification settings - Fork 2.2k
docs: add documentation for mod_gil_not_used and multiple_interpreters #5659
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
@colesbury, could you do a quick review of the |
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 for following up so quickly!
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 looks great to me. A few minor comments below
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.
Pull Request Overview
Add detailed documentation for the new py::mod_gil_not_used()
tag (free-threading support) and the multiple_interpreters
tags (sub-interpreter support), with usage examples and best-practice guidance.
- Introduce “Free-threading support” section describing how to use
py::mod_gil_not_used()
- Introduce “Sub-interpreter support” section covering
py::multiple_interpreters::per_interpreter_gil()
,shared_gil()
, and disabling withnot_supported()
- Provide end-to-end examples showing how to combine free-threading and sub-interpreter safety
Comments suppressed due to low confidence (1)
docs/advanced/misc.rst:221
- Typo in function reference: should be
multiple_interpreters::not_supported()
to match the namespace used elsewhere.
:func:`multiple_interpreter::not_supported()`
Copilot suggestion Co-authored-by: Copilot <[email protected]>
Copilot suggestion Co-authored-by: Copilot <[email protected]>
docs/advanced/misc.rst
Outdated
static py::dict mydict; | ||
static std::mutex mydict_lock; | ||
m.def("get_last", [](py::object key, py::object next) { | ||
std::lock_guard<std::mutex> guard(mydict_lock); |
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.
Hm ... isn't this prone to dead-locking?
IIUC, py::mod_gil_not_used()
means this module is compatible with free-threading, but it doesn't mean free-threading is actually active. — Is that correct?
Assume this runs without free-threading, in other words, it runs with the GIL, then
- thread A might release the GIL in the code below,
- another thread B might reach
mydict_lock
here while holding the GIL, - then thread A will wait for the GIL, but thread B will never release it because it's waiting for thread A to release
mydict_lock
.
The situation seems to be very similar to what's described under docs/advanced/deadlock.md
I don't know what the situation is with free-threading, therefore I'm not sure what remedy to suggest here.
To be sure what we're recommending here is correct and robust, it'd be ideal to have stress tests. ChatGPT has some suggestions:
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.
IIUC, py::mod_gil_not_used() means this module is compatible with free-threading, but it doesn't mean free-threading is actually active. — Is that correct?
Correct. It must be built with free-threading, and there must be no modules loaded which don't support free-threading. (Loading a module without support will enable the GIL for the whole program.)
Hm ... isn't this prone to dead-locking?
As written, the code does not have a deadlock. It doesn't ever release the GIL, and it can't be called without the GIL (all of the code is in the module's init, or within a function called from python).
thread A might release the GIL in the code below
Yes, if you added one line somewhere in this function, such as py::gil_scoped_release{};
, then you could easily cause a deadlock. But I think that's true of almost any code that uses locks. I tried to think of the simplest example that would show both free-threading and sub-interpreters... it was kind of difficult to come up with one actually.
Since the code, as written, is correct I think the best remedy is probably just a warning to be careful about deadlocks which also references the deadlocks page.
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.
It doesn't ever release the GIL
The best way for mere mortals like us to reason about this (although strictly speaking it's not "any", but "most" or
"many"):
Any call into the Python C API may release the GIL. (It will also re-acquire it before returning.)
Please see the updated ChatGPT link for a more nuanced take. — I think ChatGPT's answer is partially besides the point I want to make here, but the conclusion is: "Careful examination of the specific API functions used is necessary to understand their behavior concerning the GIL." — That is of course completely impractical for any but the most narrowly defined conditions — which would have to include the Python version, platform, etc. — therefore the simplified rule above is usually most practical.
For this PR, I believe it'll be best to omit most of this section until we have all examples backed up with comprehensive tests. Examples tend to get copy-pasted. If people copy this pattern, I'm sure many will run into deadlocks, which are often extremely difficult to debug, especially in large, complex applications.
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.
Calls that release the GIL are not that common, it's things like IO. Can't we just provide a warning and leave the example? Unless you think the entire example is likely wrong, it still seems like it's useful to have examples. If someone modifies it to invalid it by adding a GIL call, that's not the example's fault (especially if there was a warning about the GIL).
Traveling for PyCon, so didn't really get a chance to look over the example in detail, though.
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.
Calls that release the GIL are not that common
Hm ... I think it happens more than one might expect. E.g. if an object is garbage collected as a sideeffect, in the code after the mutex lock, all bets are off what that triggers.
I really think it's best to omit this section for now, to not have people copy-paste an unsafe pattern.
We could move it to a separate PR immediately, and work on it as we get a chance.
If we don't know how to make this safe ... lot's of doubts popping up in my mind, TBH. Is it actually difficult?
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've not researched free-threaded python extensively, so I don't know what they actually recommend that modules do for locking. But if it is as bad as you describe, then almost no lock in a C/C++ module would be safe. (And I wonder if there are any CPython calls within pybind11 crossing lock boundaries.)
I can try to contrive some code that needs a lock but doesn't make any CPython calls while holding it, it shouldn't be that difficult and all the verbiage is already written....
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.
@colesbury might have input?
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.
Very nice! This reads very well, and the logic looks good to me, although I don't think of myself as an expert knowing about all potential threading pitfalls.
It'd be great to add something like
mod_gil_not_used.cpp
mod_per_interpreter_gil.cpp
mod_per_interpreter_gil_gil_not_used.cpp
with copies of these examples, plus Python code with the ChatGPT-generated stress tests.
I guess it's not entirely trivial to set those up, but I think this would be very valuable long term.
Description
Adding documentation for #5564 the multiple_interpreters tags (sub-interpreter support), and also
py::mod_gil_not_used()
with usage examples and best-practice guidance.Suggested changelog entry: