-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Added exception translator specific mutex used with try_translate_exceptions #5362
Merged
Merged
Changes from all commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
9eded36
Added exception translator specific mutex used with try_translate_exc…
vfdev-5 a546f53
- Replaced with_internals_for_exception_translator by with_exception_…
vfdev-5 4182510
style: pre-commit fixes
pre-commit-ci[bot] 6e3f68e
Fixed formatting and added explicit to ctors
vfdev-5 8275a6a
Addressed PR review comments
vfdev-5 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
from __future__ import annotations | ||
|
||
|
||
class PythonMyException7(Exception): | ||
def __init__(self, message): | ||
self.message = message | ||
super().__init__(message) | ||
|
||
def __str__(self): | ||
return "[PythonMyException7]: " + self.message.a |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Unless #5296 is merged first, this will lead to incompatibility between extension modules compiled with or without
Py_GIL_DISABLED
defined. — EDIT next day: THIS IS NOT AN ISSUE. See comments posted after this one.I just only now realize ... Huston we have a problem?We already added
pymutex mutex
tostruct internals
but didn't changePYBIND11_INTERNALS_VERSION
.Ouch? Sorry I missed that before.What do we want here? I'm very uncertain, @colesbury could you please advise?
Could that work? The idea here is that
struct internals
is the same with and withoutPy_GIL_DISABLED
defined.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.
Free-threading is experimental so changing the ABI before was fine. Now I think packages might be beginning to ship, so probably not such a great idea to change without a bump. But not fond of these confusing bumps. The ABI is currently a mess, by the way - the Windows only bump broke conda-forge and the only solution was to pin <2.12, which now is breaking packages (see the feedstock for more info).
Will the internals version matter after #5296? Could we just basically do away with the concept, and only do compiler/stdlib, etc?
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.
Not entirely, I think. The internals versions still need to match for the inheritance functionality to work correctly. (I'd probably need a day or two to figure out conclusively if that's fixable or just fundamentally impossible.)
But the more I think about it, the more I'm getting worried about having a different
struct internals
with and withoutPy_GIL_DISABLED
defined:pybind11/include/pybind11/detail/internals.h
Lines 177 to 180 in 5efc743
pybind11/include/pybind11/detail/internals.h
Lines 185 to 190 in 5efc743
What happens if there is one extension built with the GIL enabled, another built with free-threading, and both are imported into the same process?
Currently we only have this:
pybind11/include/pybind11/detail/common.h
Lines 390 to 405 in 5efc743
Do we need to also check for
Py_GIL_DISABLED
compatibility there?Or do we have to do something here?
pybind11/include/pybind11/detail/internals.h
Lines 324 to 332 in 5efc743
@colesbury
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.
You can't have extensions compiled with and without
Py_GIL_DISABLED
in the same process. An extension might require the GIL to be dynamically enabled, but that's different: that's a matter of calling (or not calling)PyUnstable_Module_SetGIL()
. The extension will still need to be compiled withPy_GIL_DISABLED
to be loaded in 3.13t.In other words:
Py_GIL_DISABLED
is part of the extension ABI, similar toPy_VERSION_HEX
cp313-cp313t
), just like the version numberThere 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.
Or as @henryiii wrote in #5148 (comment): "A free-threaded build can't talk to a non-free-threaded build".
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 agree, I should have built using same python version, e.g. 3.13t and 3.13.
Doing so, above example is giving either a segfault:
or
If this is again unrelated sorry for the noise.
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 a lot! That really helps me understand the situation better. So I think what we need to do is work on this code:
pybind11/include/pybind11/detail/common.h
Lines 390 to 405 in 5efc743
We want to catch if there is a mismatch between the Python interpreter kind (gil, free-threading) and the way the extension was built.
In a separate PR.
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.
Yeah ... "version bump chaos"
Trying to summarize how I understand the situation:
The internals version wasn't changed for Python 3.13t — but that's ok because there were no released packages anyway that could have been built with a non-matching
struct internals
.Assuming there are released packages for 3.13t now, yes, it would indeed be safest to bump the internals version for 3.13t. — But pegging that on
Py_GIL_DISABLED
will make it very difficult for the general user community to reason about the internals version.If this PR gets merged as is, we'll have internals version
4
for Python < 3.12 if_MSC_VER
is not defined.5
for Python 3.12, or 3.13, or if_MSC_VER
is defined.6
for Python 3.13t only.Who can (or wants to) understand that? :-)
And then we also still have #5339 (smart_holder) and #5280 (native enum) that I'd really like to see merged.
I believe this is a situation where the maintainers need to step up to balance "making progress" with "minimizing confusion".
@henryiii wrote:
That makes me think, let's not mess with the internals version in this PR:
WDYT?
Looking beyond that question, I feel it'd be best to approach this strategically:
6
unconditionally.That's a discussion far beyond this PR, but I'm mentioning it here because it would be another reason to not mess with the internals version here.
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'm concerned that landing this PR without changing the version number will cause problems not just for users but also package maintainers (e.g., scipy, PyTorch) that use pybind11 and support free-threading. The failure modes for different internals can be complicated -- in this case, I think you run the risk of treating uninitialized memory like a mutex.
I think the approach in this PR is a good long term fix, but if the version bump is the issue we can first pursue a different approach that avoids changing the internals struct (and consider landing this change later when #5280 is ready).
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.
First priority, I want to be helpful. Second minimize potential for confusion.
Unfortunately I'm technically somewhat handicapped at the moment (no Linux workstation), but let me try to get #5296 merged asap, with @henryiii's formal approval, to be sure we're on the same.
After that version bumps will have a lot less impact, and it won't matter as much that people fully understand the numbering.