[subinterpreters] PEP 554 implementation: add interpreters module #76785
Comments
|
In the interest of getting something landed for 3.7, so we can start using it in tests, I'm putting up a patch for a low-level interpreters module. In some ways this is a precursor for issue bpo-30439, which will add a proper public stdlib module in 3.8. The module I'm adding draws from the ideas in PEP-554 (particularly for channels). Consequently, this will also give us an opportunity to try out some of the semantics from the PEP to give us better ideas for 3.8. I expect to have some follow-on patches to facilitate simpler use in tests. This patch is big enough already. :) |
ericsnowcurrently
added
3.7 (EOL)
|
@ned, it may be a little tight to land this given the time left before beta 1. However, this is meant as a tool for us to use in the test suite (particularly to test the subinterpreter C-API). So I'm arguing that, if necessary, it would still be okay to land this after the feature freeze. (I'm still hoping to get this in before the cutoff.) What do you think? |
|
FYI, there are a few things I need to clean up in the PR. However, I expect that those changes will be minor relative to the the whole patch, so I wanted to get the ball rolling on a review. :) |
|
@eric, given the breadth of change introduced in the PR (including adding a new extension), I think it would be best if at all possible to get it in for beta 1 if we can resolve the review comments in time. If necessary and if there are no objections from other core developers, I would be willing to consider making an exception and allowing it into beta 2 as long as it remains a private interface. If it looks like it won't be in releasable shape by then, I think you should hold off for 3.8; doing otherwise would be unfair to others and to our downstream beta users / testers, for example, even if it is private, adding a new extension and setup.py changes potentially affect downstream packagers. |
|
Sounds good, Ned. Thanks for taking a look. I should have everything finished up by Friday, so I'm hopeful for landing the change before the deadline. I may have a few minor tweaks to make after that, but I'll discuss that with you before making any changes if that happens. |
|
I've merged the patch without Windows support, which shouldn't be a problem given the purpose of the extension module. I've also added a PR for get the module building under Windows. I'd like to get that resolved ASAP. |
|
Eric, looks like some buildbots are unhappy, for instance: |
|
Yeah, I'm looking into it. Also, I noticed some refleaks that I'll be sorting out. |
|
On 4 of the buildbots: ====================================================================== Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test__xxsubinterpreters.py", line 890, in test_drop_multiple_times
interpreters.channel_drop_interpreter(cid, send=True, recv=True)
SystemError: More keyword list entries (7) than format specifiers (3)====================================================================== Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test__xxsubinterpreters.py", line 848, in test_drop_single_user
interpreters.channel_drop_interpreter(cid, send=True, recv=True)
SystemError: More keyword list entries (7) than format specifiers (3)====================================================================== Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test__xxsubinterpreters.py", line 957, in test_drop_used_multiple_times_by_single_user
interpreters.channel_drop_interpreter(cid, send=True, recv=True)
SystemError: More keyword list entries (7) than format specifiers (3)====================================================================== Traceback (most recent call last):
File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test__xxsubinterpreters.py", line 899, in test_drop_with_unused_items
interpreters.channel_drop_interpreter(cid, send=True, recv=True)
SystemError: More keyword list entries (7) than format specifiers (3) |
|
On the PPC64 AIX 3.x buildbot: ====================================================================== Traceback (most recent call last):
File "/home/shager/cpython-buildarea/3.x.edelsohn-aix-ppc64/build/Lib/test/test__xxsubinterpreters.py", line 784, in test_repr
self.assertEqual(repr(cid), 'ChannelID(10)')
AssertionError: 'ChannelID(0)' != 'ChannelID(10)'
- ChannelID(0)
+ ChannelID(10)
? + |
|
I just put up a PR that should fix the 4 buildbots. |
|
The buildbots should be happier now. I'll keep an eye on them. |
|
A couple defects reported by coverity: ** CID 1428758: Integer handling issues (CONSTANT_EXPRESSION_RESULT) /Modules/_xxsubinterpretersmodule.c: 45 in _coerce_id() 1217 } |
|
I've landed a PR that fixes all the memory leaks in the module. It also fixes the 2 defects reported by coverity. The only thing left here is to get the module building under Windows. |
|
FYI, out of 2389 source lines in the C extension, 1563 are the channel-related code. That means the non-channel code is 826 lines (about a third). That non-channel code does not depend on the channel code at all and I considered splitting the source out, but figured there wasn't enough benefit. However, I might revisit the matter later when I circle back to PEP-554. :) |
|
Eric, it looks like your recent commit introduced a refleak. We need to fix it before beta2. ~/d/p/cpython (master $) » ./python.exe -m test -R3:3 test_multiprocessing_fork 1 test failed: And just before it: ~/d/p/cpython ((bd09335…) $) » ./python.exe -m test -R3:3 test_multiprocessing_fork Total duration: 9 min 12 sec |
I had meant to switch everything to InterpreterError when I added it a while back. At the time I missed a few key spots. As part of this, I've added print-the-exception to _PyXI_InitTypes() and fixed an error case in `_PyStaticType_InitBuiltin().
In addition to the increase test coverage, this is a precursor to sorting out how we handle interpreters created directly via the C-API.
This is similar to the situation with threading._DummyThread. The methods (incl. __del__()) of interpreters.Interpreter objects must be careful with interpreters not created by interpreters.create(). The simplest thing to start with is to disable any method that modifies or runs in the interpreter. As part of this, the runtime keeps track of where an interpreter was created. We also handle interpreter "refcounts" properly.
|
Since #117662, the Windows refleak buildbot fails with a rather big leak: Are you able to debug/reproduce this? |
|
I'll take a look. |
|
FYI, I've narrowed it down to the situation where we're running a "capturing" wrapper (see Lib/test/test_interpreters/utils.py) in a legacy subinterpreter (or at least one with the |
|
Looks like the culprit is one of the modules imported via test.support.os_helper. I'm guessing _ctypes. I'll have a PR up shortly. |
gh-117662 introduced some refleaks, or, rather, exposed some existing refleaks. The leaks are coming when test.support.os_helper is imported in a "legacy" interpreter. I've updated test.test_interpreters.utils to avoid importing os_helper, which fixes the leaks. I'll address the root cause separately.
|
FYI, I've opened gh-117936 to deal with the root cause of the refleak. |
pythongh-115566) This brings the code under test.support.interpreters, and the corresponding extension modules, in line with recent updates to PEP 734. (Note: PEP 734 has not been accepted at this time. However, we are using an internal copy of the implementation in the test suite to exercise the existing subinterpreters feature.)
I missed this change in pythongh-115566.
I had added an extra cleanup abstraction a while back that has turned out to be unnecessary.
…-116328) This includes adding pickle support to various classes, and small changes to improve the maintainability of the low-level _xxinterpqueues module.
…ongh-116369) I accidentally introduced the warning in pythongh-116328.
Mostly we unify the two different implementations of the conversion code (from PyObject * to int64_t. We also drop the PyArg_ParseTuple()-style converter function, as well as rename and move PyInterpreterID_LookUp().
I added it quite a while ago as a strategy for managing interpreter lifetimes relative to the PEP 554 (now 734) implementation. Relatively recently I refactored that implementation to no longer rely on InterpreterID objects. Thus now I'm removing it.
These helpers make it easier to customize and inspect the config used to initialize interpreters. This is especially valuable in our tests. I found inspiration from the PyConfig API for the PyInterpreterConfig dict conversion stuff. As part of this PR I've also added a bunch of tests.
…ythongh-117485) This eliminates the duplication of functionally identical helpers in the _testinternalcapi and _xxsubinterpreters modules.
…-117491) This is a follow-up to pythongh-117170 and pythongh-117485.
…7489) I had meant to switch everything to InterpreterError when I added it a while back. At the time I missed a few key spots. As part of this, I've added print-the-exception to _PyXI_InitTypes() and fixed an error case in `_PyStaticType_InitBuiltin().
…h-117662) In addition to the increase test coverage, this is a precursor to sorting out how we handle interpreters created directly via the C-API.
This is similar to the situation with threading._DummyThread. The methods (incl. __del__()) of interpreters.Interpreter objects must be careful with interpreters not created by interpreters.create(). The simplest thing to start with is to disable any method that modifies or runs in the interpreter. As part of this, the runtime keeps track of where an interpreter was created. We also handle interpreter "refcounts" properly.
…7913) pythongh-117662 introduced some refleaks, or, rather, exposed some existing refleaks. The leaks are coming when test.support.os_helper is imported in a "legacy" interpreter. I've updated test.test_interpreters.utils to avoid importing os_helper, which fixes the leaks. I'll address the root cause separately.
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: