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 — bpo-32604: Expose the subinterpreters C-API in a "private" stdlib module. by ericsnowcurrently · Pull Request #1748 · python/cpython

Conversation

@ericsnowcurrently

@ericsnowcurrently ericsnowcurrently commented May 23, 2017 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

https://bugs.python.org/issue32604

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label May 23, 2017
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from e0611a8 to bee1baa Compare May 23, 2017 04:38
@ericsnowcurrently ericsnowcurrently changed the title Expose the subinterpreters C-API in a "private" stdlib module. bpo-30439: Expose the subinterpreters C-API in a "private" stdlib module. May 23, 2017
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from bee1baa to 060a720 Compare May 23, 2017 23:42
@ericsnowcurrently ericsnowcurrently mentioned this pull request May 24, 2017
bpo-30439: Add some helpful low-level functions for subinterpreters. #1802
Closed
pitrou
pitrou reviewed May 25, 2017
View reviewed changes
Comment thread Doc/library/_interpreters.rst Outdated
@@ -0,0 +1,69 @@
:mod:`_interpreters` --- Low-level interpreters API

@pitrou pitrou May 25, 2017

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.

There should be a warning about the module being private, and/or the API being subject to change without notice, IMHO.

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Aug 22, 2017

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.

Good point. For the most part I was following the precedent of the _thread module. However, in this case the module is relatively unstable still and there is no "interpreters" module yet. So a warning makes sense.

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan Jan 20, 2018

Copy link
Copy Markdown
Contributor

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.

Given the not-yet-approved PEP, I think it would be a good idea to use a name that makes it clear that this really is intended as an API for the CPython test suite, rather than for general use.

That would mean going with something like _xxsubinterpreters for now.

Sorry, something went wrong.

Uh oh!

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

@ncoghlan ncoghlan Jan 20, 2018

Copy link
Copy Markdown
Contributor

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.

Note: one consequence of that is that redistributors that remove the test suite would likely remove this module as well, and I think that's a desirable outcome: we want this as a learning tool for the further development of PEP 554 as a proposal for 3.8 (and to find and fix bugs in the subinterpreter support in 3.7), not as an inadvertent commitment to a particular API.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pitrou
pitrou reviewed May 25, 2017
View reviewed changes
Comment thread Lib/test/test__interpreters.py Outdated
import threading
import _interpreters
def f():
_interpreters.run_string(id, dedent('''

@pitrou pitrou May 25, 2017

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.

I don't understand where the id variable is coming from... is it the built-in id function?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Aug 22, 2017

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.

Good catch. That's a typo that I didn't notice because the resulting exception in the thread does not cause the process to fail.

FYI, adding the missing "id = _interpreters.create()" line causes the test to fail (correctly); the subinterpreter didn't get cleaned up before we reached Py_Finalize(), resulting in Py_FatalError. I'll sort that out separately.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from 060a720 to 2fd0e64 Compare August 22, 2017 20:01
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch 2 times, most recently from 114bda4 to 52ca360 Compare September 7, 2017 17:58
@Mariatta Mariatta added needs rebase and removed needs rebase labels Oct 9, 2017
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from 52ca360 to 9991d75 Compare November 27, 2017 23:24
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from 9991d75 to e9d9b04 Compare December 5, 2017 01:29
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from 81de3c8 to bfc025f Compare December 12, 2017 05:12
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch 2 times, most recently from e05d02e to 396e294 Compare December 23, 2017 00:30
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch 3 times, most recently from 5266080 to c86647a Compare January 13, 2018 00:49
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch 2 times, most recently from ff4ae72 to 0981dbf Compare January 20, 2018 01:44
@ericsnowcurrently ericsnowcurrently changed the title bpo-30439: Expose the subinterpreters C-API in a "private" stdlib module. bpo-32604: Expose the subinterpreters C-API in a "private" stdlib module. Jan 20, 2018
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from 0981dbf to 0be6314 Compare January 20, 2018 02:13
1st1
1st1 reviewed Jan 20, 2018
View reviewed changes
Comment thread Doc/library/_interpreters.rst Outdated

@1st1 1st1 Jan 20, 2018

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.

Can we call the function run as per the PEP?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jan 26, 2018

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.

Note that the PEP is all about the high-level API we want to expose to users. In contrast, run_string() is the low-level function focused strictly as an analog to PyRun_String*(). As we support other runnable things (e.g. code objects, functions), we will add corresponding low-level functions. The eventual high-level method (i.e. Interpreter.run() will call the appropriate low-level function.

Also, I'm removing the docs for now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
1st1
1st1 reviewed Jan 20, 2018
View reviewed changes
Comment thread Include/internal/pystate.h Outdated

@1st1 1st1 Jan 20, 2018

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 actual layout of the struct should go into an "internal" header file:

"Include/pystate.h":

typedef struct _xid _PyCrossInterpreterData;

"Include/internal/pystate.h":

struct _xid {
    void *data;
    // etc
}

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jan 26, 2018

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.

I haven't touched Include/pystate.h. If you are suggesting that I move the typedef (but not the layout) there, I don't see any benefit to that for 3.7. This is not a public API for now.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
1st1
1st1 reviewed Jan 20, 2018
View reviewed changes
Comment thread Modules/_interpretersmodule.c Outdated

@1st1 1st1 Jan 20, 2018

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.

You don't need to check for NULL here, PyThreadState_Get aborts if it can't lookup the state.

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jan 26, 2018

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.

fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
1st1
1st1 reviewed Jan 20, 2018
View reviewed changes
Comment thread Modules/_interpretersmodule.c Outdated

@1st1 1st1 Jan 20, 2018

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.

per PEP 7 please use curly braces always :)

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jan 26, 2018

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.

fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
1st1
1st1 reviewed Jan 20, 2018
View reviewed changes
Comment thread Modules/_interpretersmodule.c Outdated

@1st1 1st1 Jan 20, 2018

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.

Check if shared is NULL

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jan 26, 2018

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.

fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
1st1
1st1 reviewed Jan 20, 2018
View reviewed changes
Comment thread Modules/_interpretersmodule.c Outdated

@1st1 1st1 Jan 20, 2018

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.

Redundant? And need to free err?

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jan 26, 2018

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.

I'd meant to return err on the next line rather than NULL. :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
ericsnowcurrently added 10 commits January 27, 2018 19:12
@ericsnowcurrently
Add typedefs.
2df5aff
@ericsnowcurrently
_shared_exception -> _sharedexception
9bb05cf
@ericsnowcurrently
Free memory in the error case.
633a3d3
@ericsnowcurrently
Whitelist instead of blacklist for richcompare ops.
74e7241
@ericsnowcurrently
Decref in the error case.
bb8619a
@ericsnowcurrently
Add a Misc/NEWS entry.
d785573
@ericsnowcurrently
Do not try to print a bytes object.
f856035
@ericsnowcurrently
Drop some outdated comments.
ba94571
@ericsnowcurrently
Wrap long lines.
9065001
@ericsnowcurrently
Fix some compiler warnings.
2ebc68c
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from 8ae8507 to 2ebc68c Compare January 27, 2018 19:15
@ericsnowcurrently

ericsnowcurrently commented Jan 27, 2018

Copy link
Copy Markdown
Member Author

FYI, I've temporarily removed the Windows support to see if anything else breaks. I'll add it back in soon. I do not plan on merging this without it.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
Always use PyMem_NEW().
48291af
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch 2 times, most recently from ebbf591 to 9718b91 Compare January 27, 2018 21:07
@ericsnowcurrently
Do not try to allocate too much space.
39df95e
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch 2 times, most recently from edec009 to 092fccc Compare January 27, 2018 22:50
@ericsnowcurrently
Add Windows build support.
b07e928
@ericsnowcurrently
ericsnowcurrently force-pushed the low-level-interpreters-module branch from 092fccc to b07e928 Compare January 27, 2018 23:19
@ericsnowcurrently

ericsnowcurrently commented Jan 27, 2018

Copy link
Copy Markdown
Member Author

@zooba I suspect my problem is with Py_BUILD_CORE. When I don't set it I don't get the symbols out of Include/internal. When I do set it I end up missing stuff from python37.lib at link time. I tried adding a "Link" directive right below where I set Py_BUILD_CORE, but that did not help. :/ Any ideas on what I need to do?
All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently commented Jan 29, 2018

Copy link
Copy Markdown
Member Author

@python/windows-team Any of you have ideas on how I can get this PR to link on Windows?
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
Drop Windows build support (for now).
1bfed0f
@ericsnowcurrently

ericsnowcurrently commented Jan 30, 2018

Copy link
Copy Markdown
Member Author

In the interest of the feature freeze I'm going to land the PR without Windows support. :/ The risk for beta1 is low since the PR does not have any OS-specific code and the new module is only for use in the test suite. I'll work on getting Windows support done ASAP.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@ericsnowcurrently
ericsnowcurrently merged commit 7f8bfc9 into python:master Jan 30, 2018
@ericsnowcurrently ericsnowcurrently mentioned this pull request Jan 30, 2018
bpo-32604: Add the _xxsubinterpreters extension module under Windows. #5436
Closed
ncoghlan
ncoghlan reviewed Jan 30, 2018
View reviewed changes
Comment thread Include/internal/pystate.h
//
// We use the ID rather than the PyInterpreterState to avoid issues
// with deleted interpreters.
int64_t interp;

@ncoghlan ncoghlan Jan 30, 2018

Copy link
Copy Markdown
Contributor

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.

Something we'll need to keep an eye on here is not re-using interpreter IDs within the life of a process (otherwise we'll still run into issues with deleted interpreters).

Not that I've ever worked on systems where we introduced bugs by adding a freelist for objects where we assumed IDs would never be re-used or anything ;)

Sorry, something went wrong.

Uh oh!

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

@ericsnowcurrently ericsnowcurrently Jan 30, 2018

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.

We picked int64_t so that it was both really large and would overflow to negative. When the next ID is negative PyInterpreterState_New() raises RuntimError.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
ncoghlan reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@ericsnowcurrently
ericsnowcurrently deleted the low-level-interpreters-module branch February 3, 2018 04:54
@ericsnowcurrently ericsnowcurrently mentioned this pull request Aug 24, 2018
add low-level module (_xxsubinterpreters) ericsnowcurrently/multi-core-python#1
Closed
@ericsnowcurrently ericsnowcurrently mentioned this pull request Apr 10, 2022
[subinterpreters] PEP 554 implementation: add interpreters module #76785
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 1st1 left review comments
@ncoghlan ncoghlan ncoghlan left review comments
@pitrou pitrou pitrou left review comments

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

7 participants

@ericsnowcurrently @1st1 @zooba @ncoghlan @pitrou @Mariatta @the-knights-who-say-ni
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.