-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Embeded sub-interpreters #5666
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
base: master
Are you sure you want to change the base?
Embeded sub-interpreters #5666
Conversation
I think #5646 is about to go in, then I'll rebase this and add 3.13t and 3.14t in. (Edit: I think I meant to put this on the previous PR, didn't realize there were two) |
It looks to me like the 3.14 crashes are actually a CPython bug, python/cpython#134144. |
386231f
to
8031359
Compare
Do you know why mingw is failing? |
Not sure. My guess is the failure is destroying the sub-interpreter on a different OS thread than the one that created it. You can do that in 3.13, but in 3.12 it seems even though the feature is "stable" there are some issue with PyThreadState objects on sub-interpreters in that version. |
@henryiii Maybe it's a good time to drop AppVeyor? I haven't seen anything resulting uniquely from the AppVeyor job for years. |
And find a better way to make that work.
I am surprised other compilers allowed this code with a deleted move ctor.
It just has to be ifdef'd because it is slightly broken on 3.12, working well on 3.13, and kind of crashy on 3.14beta. These two verion ifdefs solve all the issues.
They contain Python object references and acquire the GIL, that means they are a danger with subinterpreters!
Try special casing the destruction on the same thread.
…see if the problems go away.
24af141
to
ae0da30
Compare
I don't think the failure is due to this PR, pasting it here and rerunning:
|
Yes, it's a flake, and unrelated. We'll need to see how often it shows up, and in which jobs (3.14t only, or both). |
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
This PR introduces support for Python sub-interpreters in pybind11 by adding a dedicated API, updating documentation, and refactoring tests to exercise the new functionality.
- Added a new
pybind11/subinterpreter.h
header with RAII wrappers and utilities for sub-interpreters. - Updated embedding documentation (
docs/advanced/embedding.rst
) to describe sub-interpreter API and best practices. - Refactored existing interpreter tests: extracted sub-interpreter tests into
test_subinterpreter.cpp
and updated build configuration.
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
tests/test_embed/test_interpreter.cpp | Removed embedded sub-interpreter tests and helper functions. |
tests/test_embed/CMakeLists.txt | Added test_subinterpreter.cpp to the test executable. |
tests/extra_python_package/test_files.py | Included include/pybind11/subinterpreter.h in test package. |
include/pybind11/subinterpreter.h | New API for creating, using, and destroying Python sub-interpreters. |
include/pybind11/gil.h | Clarified shutdown comment in gil_scoped_acquire . |
docs/advanced/misc.rst | Added an anchor for concurrency section. |
docs/advanced/embedding.rst | Rewrote embedding guide to cover sub-interpreter usage and safety. |
CMakeLists.txt | Installed subinterpreter.h alongside other headers. |
Comments suppressed due to low confidence (1)
docs/advanced/embedding.rst:484
- Use “its” instead of “it's” in documentation.
:class:`subinterpreter_scoped_activate` past the lifetime of it's :class:`subinterpreter`)
Co-authored-by: Copilot <[email protected]>
@ericsnowcurrently You might be interested in this PR. ;) |
I'm dropping the flaky 3.14t free-threading logs into #5674. I've seen two of these now. They have not been in the subintepreters part (at least yet). |
I started looking at the PR a few days ago and got distracted by other stuff. I do plan on getting back to it, but don't wait for me. @henryiii (or any other pybind11 folks at PyCon), if you have time at the sprints I wouldn't mind walking through this with you. |
I left yesterday afternoon. @virtuald, are you still at the sprints? The good news is the API is mostly separate from pybind11. What you do once you have a subinterpreter, of course, then you use pybind11 to interact with it, but the launching of subintepreters is isolated to this PR. (The embedding of Python is also from pybind11.) I'm tempted to drop this into the 3.0 RC, since I'm still finishing preparations, and then we'll have ~1 week to change if anything pops up; this is new so last RC changes are fine, IMO. |
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.
Some comments
old.creation_tstate_ = nullptr; | ||
} | ||
|
||
subinterpreter &operator=(subinterpreter &&old) { |
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.
subinterpreter &operator=(subinterpreter &&old) { | |
subinterpreter &operator=(subinterpreter &&old) noexcept { |
I'm surprised clang-tidy didn't catch 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.
Hmm, is this not being included in the cmake for some reason?
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 don't think our embedded tests are. The header is included, but it doesn't get checked unless it's also compiled somewhere, I think.
subinterpreter(subinterpreter const ©) = delete; | ||
subinterpreter &operator=(subinterpreter const ©) = delete; | ||
|
||
subinterpreter(subinterpreter &&old) |
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.
Also should be noexcept?
static inline subinterpreter create() { | ||
// same as the default config in the python docs | ||
PyInterpreterConfig cfg; | ||
memset(&cfg, 0, sizeof(cfg)); |
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.
memset(&cfg, 0, sizeof(cfg)); | |
std::memset(&cfg, 0, sizeof(cfg)); |
void disarm() { creation_tstate_ = nullptr; } | ||
|
||
/// An empty wrapper cannot be activated | ||
bool empty() const { return istate_ == nullptr; } |
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 forget, do we have a macro for nodiscard?
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.
git grep nodiscard
doesn't reveal anything. That should be in the macro def, so no.
Signed-off-by: Henry Schreiner <[email protected]>
46d0884
to
30c520b
Compare
Description
This PR adds a header file,
subinterpreter.h
which provides utility classes for embedded Python to create, use, and destroy sub-interpreters. The PR includes documentation rewrite of the "Embedded Sub-interpreter support" section. The PR also splits out the multiple interpreter tests fromtest_embed/test_interpreter.cpp
into their own file in that directory, updates them to use the new API, and adds new tests to cover the API and some interesting aspects of subinterpreters.API
Everything is in
subinterpreter.h
because this feature requiresPYBIND11_SUBINTERPRETER_SUPPORT
, which is 3.12+ (and some other caveats). Including this file in non-supported versions produces a#error
. There are no notable API outside of this new file, except for a change to embedded modules which was split out into its own PR: #5665 . This PR currently includes theembed.h
changes from that PR because this PR would not compile without them, and so this PR depends on that one being merged first (and when it is, I will fix this one with a rebase).Class
subinterpreter
manages the lifetime of a sub-interpreter. Default constructing asubinterpreter
does not create asubinterpreter
it creates an empty wrapper, this is done to make the objects easier to move around and to prevent accidentally creating them (as creating them has to acquire and release the GIL).To create a sub-interpreter, one does:
py::subinterpreter si = subinterpreter::create()
, similar topy::initialize_interpreter()
for the main interpreter. I didn't provide apy::subinterpreter::destroy(...)
since that doesn't seem to make a lot of sense (you instead destruct thesubinterpreter
object returned bycreate
) but if we want one to mirrorpy::finalize_interpreter()
then I could add one easily.subinterpreter
also includes some useful utilities:current()
to get a non-owningsubinterpreter
for the currently active interpreter (even the main one),main()
to get a non-owningsubinterpreter
for the main interpreter (even if it is not the current one),id()
to get the ID number, andstate_dict()
to get the interpreter state dict.Class
subinterpreter_scoped_activate
is the RAII wrapper, similar togil_scoped_acquire
which acquires the sub-interpreter's GIL and makes it the active interpreter. The similarity to what the gil RAII classes do is also why it is named that.Class
scoped_subinterpreter
is also provided in order to mirror the main interpreter'sscoped_interpreter
.Suggested changelog entry:
* Added API in ``pybind11/subinterpreter.h`` for embedding sub-intepreters (requires Python 3.12+).
📚 Documentation preview 📚: https://pybind11--5666.org.readthedocs.build/