Make function record subinterpreter safe #5771
Conversation
|
I am sort of baffled by the graalpy failures ... looks like it is trying to run a closure during shutdown which tries to acquire the GIL, but fails because python is now null (having already shut down, I assume). Not sure how these changes would've altered this sort of behavior...... |
|
Wow I'm trying to get my head around this PR. Starting with a maybe simple question: Is this unrelated to the function record changes? I looked back, the code changed there was first introduced here, in August 2017: It was changed recently (June 18, 2024) here: Could it make sense to break out that commit as a separate PR? (TBH I don't really understand the change. I see you're replacing |
|
I just edited my previous comment, to point here instead of your initial commit. |
Yes, I found looking for other statics which aren't sub-interpreter safe. It is not sub-interpreter safe (and I think the
Yes, I will split it out. |
|
Good call splitting them up @rwgk, turns out the function_record stuff wasn't the graal problem at all! |
|
I quizzed ChatGPT regarding this PR, could you please read? (I think it's a <5 minutes read.) https://chatgpt.com/share/68886181-f5a8-8008-83b5-b30329e9aec5 The internals version bump is somewhat of a big ouch, but if ChatGPT's analysis is correct, trying to avoid that isn't easy. WDYT? |
ChatGPT seems pretty confused here to me. I've pushed a change to avoid the internals version bump.... this type was module local before this change, so keeping it local is fine, and avoids the version bump. It might even be better this way... |
That's awesome, thanks! (I might need a couple days to find the time to carefully review the changes.) |
| constexpr char tp_name_impl[] | ||
| constexpr char tp_qualname_impl[] = PYBIND11_INTERNAL_MODULE_NAME | ||
| "." | ||
| "pybind11_detail_function_record_" PYBIND11_DETAIL_FUNCTION_RECORD_ABI_ID | ||
| "_" PYBIND11_PLATFORM_ABI_ID; | ||
| constexpr char tp_plainname_impl[] |
There was a problem hiding this comment.
Did you consider adding
setattr((PyObject *) type, "__module__", str(PYBIND11_INTERNAL_MODULE_NAME));
in get_function_record_PyTypeObject(), instead of making this change here? That would seem more idiomatic and consistent within pybind11.
There was a problem hiding this comment.
Yes, I tried that but it doesn't work and gives:
DeprecationWarning: builtin type pybind11_detail_function_record_v1_system_libstdcpp_gxx_abi_1xxx_use_cxx11_abi_1 has no __module__ attribute
I don't really understand why.
There was a problem hiding this comment.
Hm ... I think it's worth spending time getting to the bottom of this. I might be able to drill down in a few hours.
Wild guess: Maybe we need to explicitly add Py_TPFLAGS_HEAPTYPE?
There was a problem hiding this comment.
Explicitly adding Py_TPFLAGS_HEAPTYPE does not help, I'm still seeing this:
<frozen importlib._bootstrap>:488: 321 warnings
<frozen importlib._bootstrap>:488: DeprecationWarning: builtin type pybind11_detail_function_record_v1_system_libstdcpp_gxx_abi_1xxx_use_cxx11_abi_1 has no __module__ attribute
I'll try to find out why.
…xplicitly add Py_TPFLAGS_HEAPTYPE → does not resolve DeprecationWarning :-(
…ibute, explicitly add Py_TPFLAGS_HEAPTYPE → does not resolve DeprecationWarning :-("
This reverts commit 9ccd6de.
Everything looks good to me. |
Description
I think I have tracked down the sporadic unit test failure/crash.
The function record type is using a global static type definition, which is not subinterpreter safe. It needs to use a heap type which is registered with each (sub)interpreter.
Suggested changelog entry:
📚 Documentation preview 📚: https://pybind11--5771.org.readthedocs.build/