-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix: handle implementations without subinterpreter support #5732
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
Conversation
Would it be better to gate this as cpython only? It's not a public API until 3.14 lands |
tests/test_multiple_interpreters.py
Outdated
if sys.version_info >= (3, 15): | ||
import interpreters |
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.
When accepting the PEP the steering council made this concurrent.interpreters
, and it's been given an exception to land in 3.14.0b3. Not sure there's any action worth taking yet, once b3 is out we can just drop support for b2, I assume. Unless we want to go all in on importorskip. Normally not a fan of importorskip for version dependent things, but maybe we should go that way instead.
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.
b3 just got tagged, so I'm going to see if I can test this locally on my build.
There's also |
@ericsnowcurrently, would it make sense for |
(@b-pass, perhaps?) I'm trying to use the new module. Do you know how to replace Edit: I think I got it. I'm guessing the "legacy" interpreter creation option isn't available in the new API. |
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
bfde4d7
to
31d5fa4
Compare
2.13? Did you mean 3.12 or 3.13, perhaps? |
tests/test_multiple_interpreters.py
Outdated
assert res1 != res2, "internals should differ between interpreters" | ||
|
||
# do this after the two interpreters are destroyed and only one remains | ||
import mod_per_interpreter_gil as m3 |
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.
FYI, importing again just pulls this from sys.modules
again, so not sure why this is done this way, would be clearer to just keep reusing m
, but not changing this 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.
Ah, yeah, this import and the next assert don't really do anything.
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 think I'll go ahead and remove these, since m2
and m3
are identical to m
, which means they are really just testing m.internals_at() == m.internals_at()
which isn't interesting.
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 glanced through, looks good to me, but my subinterpreter know-how isn't very deep.
@b-pass, could you please help with a more expert review?
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.
Pull Request Overview
Updates the multiple interpreters tests to support Python 3.12–3.14+ and skip cases when the native interpreters
module isn't available.
- Introduce a
get_interpreters
helper to abstract over stdlib and private interpreter APIs by version. - Refactor existing tests to use
run_string
/create
returned from the helper and remove manual cleanup. - Add a new
test_independent_subinterpreters_modern
for Python 3.14+ using theconcurrent.interpreters
API.
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.
Looks good to me. I haven't tried 14b3 yet though...
tests/test_multiple_interpreters.py
Outdated
assert res1 != res2, "internals should differ between interpreters" | ||
|
||
# do this after the two interpreters are destroyed and only one remains | ||
import mod_per_interpreter_gil as m3 |
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.
Ah, yeah, this import and the next assert don't really do anything.
Signed-off-by: Henry Schreiner <[email protected]>
Signed-off-by: Henry Schreiner <[email protected]>
Beta 3 should show up soon: https://github.com/actions/python-versions/releases/tag/3.14.0-beta.3-15723154677
(it did pass locally for me with beta 3 on macOS) |
I wanted to leave it open to the possibility that graalpy might implement the internal module at some later point, although it's not very likely.
Yeah, sorry, I meant 3.12, I permuted the digits 🤦♂️ Thanks for extending the PR. I rechecked the changes on GraalPy and everything is correctly skipped. |
Signed-off-by: Henry Schreiner <[email protected]>
These aren't actually running concurrently, maybe we should do that too in a followup? I've updated the check to be unconditional on 3.14.0b3, and after b3 it should just use |
The latest build used b3. :) |
I just had a minute to look here. There was one failed job due to network issue or similar. I triggered a rerun. |
Yeah, it could make sense. Feel free to open a feature request. FWIW, I purposefully kept PEP 734 as simple as possible, with the expectation that we would build from there. |
We're working on updating GraalPy to 3.12, but we're currently not planning to add this internal module yet, so the few tests using it should get skipped. (I'm doing this now because we run pybind11 tests from master in our CI)
Suggested changelog entry:
📚 Documentation preview 📚: https://pybind11--5732.org.readthedocs.build/