GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-76785: Improved Subinterpreters Compatibility with 3.12 by ericsnowcurrently · Pull Request #115424 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented Feb 13, 2024 •
edited by bedevere-app Bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

For the most part, these changes make is substantially easier to backport subinterpreter-related code to 3.12, especially the related modules (e.g. _xxsubinterpreters). The main motivation is to support releasing a PyPI package with the 3.13 capabilities compiled for 3.12.

A lot of the changes here involve either hiding details behind macros/functions or splitting up some files.

  • Issue: [subinterpreters] PEP 554 implementation: add interpreters module #76785

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ericsnowcurrently ericsnowcurrently added the skip news label Feb 13, 2024
@bedevere-app bedevere-app Bot added the awaiting core review label Feb 13, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Feb 13, 2024
[subinterpreters] PEP 554 implementation: add interpreters module #76785
Closed
@ericsnowcurrently
ericsnowcurrently force-pushed the compat-3.12-fixes branch from 912e5d3 to c906ee3 Compare February 13, 2024 18:29
ericsnowcurrently added 20 commits February 13, 2024 14:23
@ericsnowcurrently
Add accessor macros for cross-interpreter data.
ba23dd7
@ericsnowcurrently
Hide newer PyCodeObject fields behind macros.
db3f7de
@ericsnowcurrently
Add _PyThreadState_SetWhence().
ee80f64
@ericsnowcurrently
Fix crossinterp.c.
ef8dea0
@ericsnowcurrently
Use PyInterpreterState_GetID().
71a6a9a
@ericsnowcurrently
Add _PyRuntimeState_GetXIState() and _PyInterpreterState_GetXIState().
00b80ff
@ericsnowcurrently
Factor out _interpreters_common.h.
e5fcc75
@ericsnowcurrently
Factor out crossinterp_data_lookup.h.
f50017e
@ericsnowcurrently
Use PyInterpreterState_Get().
7bd8f7f
@ericsnowcurrently
Add _Py_EMPTY_STR.
f68737b
@ericsnowcurrently
Use PyErr_SetString().
c99fd01
@ericsnowcurrently
Use MODULE_NAME for the module init func name.
e2f9cf0
@ericsnowcurrently
Factor out crossinterp_exceptionsw.h.
dedc0e8
@ericsnowcurrently
Fix exception docstrings.
64605ed
@ericsnowcurrently
Move NotShareableError to crossinterp_exceptions.h.
5c008de
@ericsnowcurrently
Work around a ref leak.
f17b29b
@ericsnowcurrently
Fix build dependencies.
18ede77
@ericsnowcurrently
Fix the Exception names.
842a990
@ericsnowcurrently
Use PyInterpreterState_GetID().
8448f9d
@ericsnowcurrently
Update the C analyzer.
455460c
@ericsnowcurrently
ericsnowcurrently force-pushed the compat-3.12-fixes branch from c906ee3 to 455460c Compare February 13, 2024 21:23
@ericsnowcurrently
ericsnowcurrently merged commit 514b1c9 into python:main Feb 13, 2024
@bedevere-app bedevere-app Bot removed the awaiting core review label Feb 13, 2024
@ericsnowcurrently
ericsnowcurrently deleted the compat-3.12-fixes branch February 13, 2024 21:56
fsc-eriker pushed a commit to fsc-eriker/cpython that referenced this pull request Feb 14, 2024
@ericsnowcurrently @fsc-eriker
pythongh-76785: Improved Subinterpreters Compatibility with 3.12 (pyt…
abcca8a 
…hongh-115424)

For the most part, these changes make is substantially easier to backport subinterpreter-related code to 3.12, especially the related modules (e.g. _xxsubinterpreters). The main motivation is to support releasing a PyPI package with the 3.13 capabilities compiled for 3.12.

A lot of the changes here involve either hiding details behind macros/functions or splitting up some files.
markshannon
markshannon reviewed Jul 29, 2024
View reviewed changes
Comment thread Include/internal/pycore_code.h
// This helps with backporting certain changes to 3.12.
#define _PyCode_HAS_EXECUTORS(CODE) \
(CODE->co_executors != NULL)
#define _PyCode_HAS_INSTRUMENTATION(CODE) \

@markshannon markshannon Jul 29, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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_version being 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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Jul 29, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Jul 29, 2024

Copy link
Copy Markdown
Member

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?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@ericsnowcurrently ericsnowcurrently Jul 29, 2024

Copy link
Copy Markdown
Member Author

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Jul 30, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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_version does 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_INSTRUMENTATION is misnamed and an attractive nuisance. Can we please remove it?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@markshannon markshannon Jul 30, 2024

Copy link
Copy Markdown
Member

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_extras as well, as _PyInterpreterState_SetEvalFrameFunc is per-interpreter now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

2 participants

@ericsnowcurrently @markshannon
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.