-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Support for sub-interpreters #5564
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
59a2076
to
02e9609
Compare
236cc25
to
75d55f3
Compare
df2fcc6
to
e64d19f
Compare
Very excited to see this! I have a couple of comments/questions:
We'll need some docs, too. Maybe we should do a full test run with the define on, too. |
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 is a complex PR. I need to find more time later for a full review.
High-level questions:
- Could it be useful to split this PR: 1. multi-phase init only. 2. multi-interpreter support? — That would make it easier to do the reviews now, and understand the development steps in the future. It might also help us dealing with bugs after this change is released.
- Are you still working on a new Python tests, to exercise the new multi-interpreter support?
- Is there a potential for bug or feature interference between free-threading and multi-interpreter functionality? — I think we'll need tests for all combinations of (free-threading on/off) x (multi-interpreter support on/off); not for all platforms, but maybe one each: Linux, macOS, Windows.
Sure. I have created #5574 for multi-phase init only. I will keep updating this PR for sub-interpreter support, and will remove the multiphase init from this branch shortly.
I'm not currently, but I can add a few more after/along with some additional changes from comments.
No one would ever say less testing :) I think the potential problems are small. While they have similar goals, free threading and own-gil-sub-interpreters are fairly different and could be used together. Sub-interpreters were originally created (I think) to offer sandboxing features. So even with free threading, the idea that a module is used in two different sandboxes is still valid, and it still needs separation for each instance. Also I considered whether free-threading is a superset of sub-interpreter support. I think it is not, for the same reason that the two have slightly different implications for a module. With free threading (only) it is perfectly reasonable to have, for example, a global/static atomic variable. With sub-interpreters that is probably incorrect, because each sub-interpreter should have its own separate state. However, if a module is free-threading safe (so, thread safe) and it is multi-interpreter aware (as no global state) then it should also be own-gil-sub-interpreter safe... that is, it doesn't need GIL synchronization across the many multiple subinterpreter states, which must be true or it would not be free-threading safe. |
My 2cts: TLS in shared libraries is real disaster (especially C++ So adding more TLS to the internals data structure in general sounds like a pretty significant performance sink. I would encourage you to thoroughly benchmark function/method calling on Windows/macOS/Linux to see how bad this is, and to what extent these costs can be mitigated. |
FYI, this is a perfect example of where I'd personally always rebase and force push. ;) |
Luckily, these are all zero-initialized pointers, so no constructors. And I think multiple interpreters in multiple threads a the same time might qualify as a complex case. Still, point taken, on this expert advice I have made a bunch of changes to get the Unfortunately, in the multi-interpreter case there really isn't any choice but to use |
Looks like, with the rewrite, the cost is an extra 0.22ns per call to get_details (an increase of about 15%). IMO that is a very small cost, unlikely any real-world usage will notice it. The cost is much more significant when a subinterpreter is actually created, the cost to access the internals triples. In my opinion, that's just the cost of using subinterpreters. ... I've added this information to the PR description. |
Hi, I wonder if you have read these:
To support sub-interpreters, I think we also need to implement PEP 573 and PEP 630. To clarify:
I think it would be really hard to make |
The goal of these is to get rid of global state, and replace it with state that is tied to the module instance. While implementing this according to the python guides will definitely accomplish that goal -- and that might be the best way to do it --, that is not the only way to accomplish it. I think strict adherence to these would require major rewrites of several parts of pybind11, but I don't think that is necessary to support sub-interpreters/multi-interpreters. Definitely following PEP573 would make a module work with sub-interpreters. It is a sufficient condition but not a necessary condition. Pybind11's global state is entirely contained within
I don't think this is required, since Pybind11's types are managed by its internals structures, they already are not globally static in the strict sense. Converting them to use Type_Spec and Slots is IMO unrelated to this PR. (Edit: or, maybe pybind11 is already doing heap types? At least, some of them are...)
I agree that is probably impossible. My goal here isn't full module isolation, pybind11 already doesn't have module isolation and it can't be added. That doesn't mean it can't support sub-interpreters. Maybe another way to think about it is that this PR adds interpreter isolation without adding module isolation. The examples you linked explain the kinds of problems that non-isolated modules have, which existing pybind11 modules all have, and they would continue to have after this PR. |
I've run @wjakob's benchmarks for nanobind on this PR. The binary size might be a bit off, since I can't run strip due to needing undefined symbols, but I don't think that has any affect here anyway. I couldn't get nanobind to load, so I had to take it off the runtime plot. This seems to have a noticeable impact on debug (unoptimized) performance, but not really noticeable on runtime, probably within the uncertainty margins. I'd love for the runtime cost to go down (there's an old PR that did that, but not usable anymore) instead of up, but this looks acceptable to avoid complications building. |
I'd make |
Sound good, but a question for @b-pass: are all combinations of options valid? I think we should make sure we can clearly diagnosing invalid combinations. |
I just reproduced the error:
Then using the same commands as reported before, I'm still getting the same error. This should be very easy to reproduce, I have a standard Ubuntu 24.04 x86_64 workstation. |
On Ubuntu 24.04 (x86-64), I did the below, could not reproduce the problem.
Output:
I also tried with a |
Ok, I got it to crash for me after running |
…the test returns.
Thanks for confirming! — I was beginning to develop doubts about the health of my Ubuntu workstation. (So sad we don't have any sanitizers at the moment...) |
I merged your commit 22968e9 locally: unfortunately I'm still seeing the error. |
@henryiii Do you know how easy or difficult it is to build the embedding test with |
If it happens concurrently in multiple threads, badness ensues....
@rwgk OK, I think it's fixed in 89e9599 . The problem was concurrent threads importing the same module at the same time overwrote the (global, static) module_def and slots, which occasionally would stomp on each other before their init was finished. Fixed (in detail/common.h I might try to do
gil_not_used can be mixed with the multiple interpreter options. Only one of the multiple_interpreters options can be specified ... per_interpreter_gil is more constrained than shared_gil, so in the presence of both it is safe to report per_interpreter_gil. So is the suggestion to change the current |
Personally, I'm fine with the current naming, which makes it clearer that you can pick one multiple_interperters option. Otherwise, it would be |
Ok, let's stick with the current implementation then. I added a PR for docs, #5659. |
@rwgk if the error has gone away and you're OK with the naming, then I think we're good to go. |
I still see the error :-( I tried a couple times from scratch. I'll click the Update branch button here, which will switch us over to the 24.04 runners. Let's see if the error shows up in the CI, too. |
I ran this:
With this outcome:
I.e. it fails every single time. IIUC, it only fails sometimes for @b-pass? It's a mystery to me what's different on my machine. |
Again with
Not a single success, again. |
This crossed my mind (
But still:
(I'm using a |
After resetting my I rebuilt from scratch after resetting
I have to figure out what on my regular |
Did that change which version of Python was being picked up? It only affects newer Pythons. |
Yes and no. Let's call this
This is
For completeness: In what follows, it does NOT matter if I had But at runtime:
I can switch back and forth, the behavior is 100% deterministic. Interestingly (but as I expected):
Open question: How can that lead to the test failure? |
I'm getting the same behavior when using the Note: Usually I run my tests with a scons-based build. With that, tests pass for This is a bit twisty: I had to tweak my scons files to make it work for this PR. To avoid that before, I chose to simply use the cmake build instead. I totally didn't expect that to fail on But back to the cmake build: This fails as reported before:
These work, everything else exactly equal:
I don't know what to do about this, but it's pretty much certain now that the weird error has nothing to do with this PR. |
@b-pass wrote:
OK, then my suggestion with the chaining doesn't make sense, and the current naming is fine. @henryiii wrote:
Awesome, I'll merge this now. Ideally we should do something about the weird test_embed behavior, but certainly not in this PR. |
There's something wrong with the embed tests in CMake. I'll have to try to work it out, but I can't get them to pass locally, haven't for a while. But they work on many of our CI jobs (a few don't have the requirements present to run them). |
I don't think it's cmake specific (maybe you didn't mean that, but JIC, to clarify). My scons-based test just happens to work interactively because I'm doing this (read that code from the bottom up): test_embed_dirpath = os.path.join(tests_dirpath, "test_embed")
test_embed, list_of_test_py = build_list_of_tests(tests_dirpath, substrings)
env = {"PYTHONPATH": normabspath("lib")}
if test_embed:
bin_test_embed = normabspath("bin/test_embed")
print('(cd "%s" && PYTHONPATH="%s" "%s")' % (
test_embed_dirpath, env["PYTHONPATH"], bin_test_embed))
sys.stdout.flush()
subprocess.call([bin_test_embed], cwd=test_embed_dirpath, env=env) What I didn't realize in the past: passing in I believe if you change the cmake files to do this
local testing will pass for you, too. |
Description
This PR add the ability for pybind11 modules to support subinterpreters. This support requires 2 things:
internals
andlocal_internals
have to have an instance per-interpreter (can no longer be static singletons), which is the primary subject of this PR.Multiphase init
The PR adds tags which can be passed as the 3rd argument to the
PYBIND11_MODULE
definition macro (in addition to the existingmod_gil_not_used
). If the tags are not specified, the module is defined without multiple interpreter support (as they are today).If multiple interpreters is example, then when a module is imported a second time in a sub-interpreter, the module's
exec
slot is run again. For pybind11 this means the user's the module init function is re-run in the sub-interpreter. That's good, because the sub-interpreter needs it's own type_info for all of the bindings.internals
This presents the problem that the place that pybind11 stores these is currently a singleton. But sub-interpreters need this state to be per-interpreter. That means the correct instance (for the current active interpreter) needs to be retrieved from the interpreter state dict. Fortunately, the internals pointer-to-pointer is already stored in the state dict.
In order to minimize performance costs, we can detect whether or not multi-subinterpreters are present by counting how many times the module has been imported. If it has only been imported once then it can only possibly have one
internals
(even if there are other sub-interpreters where it was not imported). When it has been imported more times, then we need to do additional work to make sure the right internals object is used (the one associated with the current interpreter in the current thread). We can switch between these two cases with a single simple branch, thus causing minimal performance overhead for existing code.In the multi-interpreter case we would still like to minimized the cost of accessing internals, we don't want to have to reach into the interpreter state dict every time. So we cache the value in a
thread_local
along with the pointer to thePyInterpreterState
to which it belongs. This means that the slow path (acquiring the GIL, doing a dict lookup, etc) is only done when the activePyInterpreterState
changes (or the first timeget_internals
is called in an OS thread). So the fast path merely checks that thePyInterpreterState
hasn't changed, and then returns the previously looked up value.local_internals
also has to change in a similar way tointernals
.Both of these now share some templated code to manage their thread-local cache and the state-dict cache. Some code around these has also been cleaned up (like moving most of the
internals
initialization into its constructor).Memory management / Future work
This PR does not add support for creating / deleting / switching between sub-interpreters.
In embed, pybind11 only cleans up the internals and local_interals associated with the main interpreter (when it is finalized). Since it doesn't currently manage any subinterpreters it can't clean up after them.
Suggested changelog entry:
The guide needs to add a short mention of
py::multiple_interpreters::per_interpreter_gil()
andpy::multiple_interpreters::shared_gil()
tags.