Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-76785: Improved Subinterpreters Compatibility with 3.12#115424
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
Merged
ericsnowcurrently merged 20 commits into python:main from ericsnowcurrently:compat-3.12-fixesFeb 13, 2024
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
20 commits Select commit Hold shift + click to select a range
ba23dd7 Add accessor macros for cross-interpreter data.
ericsnowcurrently db3f7de Hide newer PyCodeObject fields behind macros.
ericsnowcurrently ee80f64 Add _PyThreadState_SetWhence().
ericsnowcurrently ef8dea0 Fix crossinterp.c.
ericsnowcurrently 71a6a9a Use PyInterpreterState_GetID().
ericsnowcurrently 00b80ff Add _PyRuntimeState_GetXIState() and _PyInterpreterState_GetXIState().
ericsnowcurrently e5fcc75 Factor out _interpreters_common.h.
ericsnowcurrently f50017e Factor out crossinterp_data_lookup.h.
ericsnowcurrently 7bd8f7f Use PyInterpreterState_Get().
ericsnowcurrently f68737b Add _Py_EMPTY_STR.
ericsnowcurrently c99fd01 Use PyErr_SetString().
ericsnowcurrently e2f9cf0 Use MODULE_NAME for the module init func name.
ericsnowcurrently dedc0e8 Factor out crossinterp_exceptionsw.h.
ericsnowcurrently 64605ed Fix exception docstrings.
ericsnowcurrently 5c008de Move NotShareableError to crossinterp_exceptions.h.
ericsnowcurrently f17b29b Work around a ref leak.
ericsnowcurrently 18ede77 Fix build dependencies.
ericsnowcurrently 842a990 Fix the Exception names.
ericsnowcurrently 8448f9d Use PyInterpreterState_GetID().
ericsnowcurrently 455460c Update the C analyzer.
ericsnowcurrently File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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 hidden or 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 hidden or 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 hidden or 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 hidden or 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,13 @@ | ||
| #define _RESOLVE_MODINIT_FUNC_NAME(NAME) \ | ||
| PyInit_ ## NAME | ||
| #define RESOLVE_MODINIT_FUNC_NAME(NAME) \ | ||
| _RESOLVE_MODINIT_FUNC_NAME(NAME) | ||
| static int | ||
| ensure_xid_class(PyTypeObject *cls, crossinterpdatafunc getdata) | ||
| { | ||
| //assert(cls->tp_flags & Py_TPFLAGS_HEAPTYPE); | ||
| return _PyCrossInterpreterData_RegisterClass(cls, getdata); | ||
| } |
This file contains hidden or 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 hidden or 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
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
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.
Uh oh!
There was an error while loading. Please reload this 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.
Apologies for not noticing this at the time.
This is incorrect. The
_co_instrumentation_versionbeing greater than zero does not indicate whether a code object is instrumented.It may suggest that it was instrumented at some point, although that may not remain true.
The version number has no meaning at all, expect to the instrumentation system, and shouldn't be used to infer properties of the code object.
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.
FWIW, this change only moved an existing hard-coded expression to a macro. I have no problems with any of it getting refactored to be more correct or better named. My motivation here is strictly to support building the 3.12 backport of my PyPI package. Replacing a macro is a lot easier than directly replacing a line of code in a file.
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.
Maybe so, but it is still incorrect.
It was added originally here: 92ca90b#diff-f29800af0b7052514f5cc3d1a5858d704a8f0dee4c88788b741c00a0ff39f8d0R402
What are you guarding against, and why?
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.
In that spot we are rejecting (for now) any code object that isn't completely basic. That includes ones that might have instrumentation.
Uh oh!
There was an error while loading. Please reload this 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.
Instrumentation is not a feature of the code object any more than specialization. It is how the interpreter does monitoring. If you marshal or copy a code object, the instrumentation is stripped.
In other words, checking for instrumentation is unnecessary here (or anyway else, in theory).
Even if it were necessary,
_co_instrumentation_versiondoes not indicate whether a code object is instrumented or not.We would need a proper API to detect instrumentation (which would probably need to scan the code)
_PyCode_HAS_INSTRUMENTATIONis misnamed and an attractive nuisance. Can we please remove it?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.
The same reasoning applies to executors, and
_co_monitoring.We can ignore
_co_extrasas well, as_PyInterpreterState_SetEvalFrameFuncis per-interpreter now.