Skip to content
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

Maybe Drop "channels" from _xxsubinterpreters #101524

Closed
ericsnowcurrently opened this issue Feb 2, 2023 · 11 comments
Closed

Maybe Drop "channels" from _xxsubinterpreters #101524

ericsnowcurrently opened this issue Feb 2, 2023 · 11 comments
Labels
3.12 bugs and security fixes extension-modules C modules in the Modules dir topic-subinterpreters type-feature A feature request or enhancement

Comments

@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Feb 2, 2023

The _xxsubinterpreters module is essentially the low-level implementation of PEP 554. However, we added it a while back for testing purposes, especially to further exercise the runtime relative to subinterpreters. Since then, I've removed "channels" from PEP 554. So it may make sense to drop that part of the implementation. That part of Modules/_xxsubinterpretersmodule.c is much more code and certainly much more complex than the basic functionality the PEP now describes.

Linked PRs

@ericsnowcurrently ericsnowcurrently added type-feature A feature request or enhancement extension-modules C modules in the Modules dir topic-subinterpreters 3.12 bugs and security fixes labels Feb 2, 2023
ericsnowcurrently added a commit that referenced this issue Feb 4, 2023
This is step 1 in potentially dropping all the "channel"-related code. Channels have already been removed from PEP 554.

#101524
@rhettinger
Copy link
Contributor

This is a bummer. It was one of the few demonstrable benefits of subinterpreters. It seems that we're left with only the most ancient and awkward forms of communcation, shared file descriptors and pipes. Almost nothing in CSP can be readily expressed with those low level tools.

Side note: The PEP seems significantly understate the impact of isolating modules, "This situation is limited to modules that use C globals (or use libraries that use C globals) to store internal state.". As Erland Aasland sweeps through standard library making edits, the cost is becoming clear. Even simple stateless modules like bisect get significant churn. More interesting but stateless modules like itertools have about half the lines in the module changed (see gh-101277 for just one-fourth of the edits). The impact is massive. Along the way, the edits negatively impact performance which is annoying because almost the entire point of writing C accelerator code is to fix performance issues on critical code paths. We're losing freelists, ability of a type to reference its own methods, the _Py_IDENTIFIER() optimization, singletons, etc.

So far, this massive effort seems like all cost and no benefit. Gaining Go-like or CSP-like channels would have been the one win. Otherwise, all we left with is the questionable benefit of "looking like one process to the O/S". It isn't clear at all that that is something we want to pay for.

The cost/benefit proposition of PEP 554 has changed considerably since the beginning. The code churn is huge and is taking many man-months of time. Meanwhile the proposed benefits are down to "looks like a single process" and "ability to experiment with new concurrency models".

@encukou
Copy link
Member

encukou commented Feb 6, 2023

The cost/benefit proposition of PEP 554 has changed considerably since the beginning. The code churn is huge and is taking many man-months of time.

Note that the changes you mention are needed to properly support Py_NewInterpreter, which isn't a newly proposed change -- it predates PEPs. The known issues are similarly old, see e.g. this note in Python 2.0 docs:

(XXX This is a hard-to-fix bug that will be addressed in a future release.)

PEP 554 exposes the ancient functionality to Python code. Withoun channels its benefit decreases (or rather, is moved to a future PEP as this one is too long), but the cost is unrelated to PEP 554.

So where to express concerns with “isolation” changes? Please read https://docs.python.org/3/howto/isolating-extensions.html, and open a Discourse discussion if you disagree with anything there or if the changes do more than necessary. (FWIW, I do think we should be a little more conservative/careful and invest more in groundwork so the individual changes aren't as invasive -- but on the other hand I don't want to stop people from improving things. The groundwork for fixing static types, for example, would probably be multi-year effort with uncertain results.)

@ericsnowcurrently
Copy link
Member Author

This is a bummer. It was one of the few demonstrable benefits of subinterpreters. It seems that we're left with only the most ancient and awkward forms of communcation, shared file descriptors and pipes. Almost nothing in CSP can be readily expressed with those low level tools.

Thanks for speaking up about this, Raymond.

I removed channels from PEP 554 because readers kept getting lost in that part of the proposed API and my descriptions and examples. Ultimately, I'm still not sure the API is quite right. Furthermore, it isn't a good sign that the implementation is so complex.

That said, I still think channels (or whatever we call them) are the best primitive for the concurrency model and anticipate they will be part of the stdlib sooner rather than later. I just don't want PEP 554 to be held up by that. In the meantime, we can give the channels design some time to bake on PyPI.

FWIW, I'm open to more discussion on this if you think channels are important enough to keep in PEP 554. Just keep in mind that I'm hoping to have PEP 554 accepted in time for 3.12.

Side note: The PEP seems significantly understate the impact of isolating modules

Petr has covered everything I would have said.

@erlend-aasland
Copy link
Contributor

I removed channels from PEP 554 because readers kept getting lost in that part of the proposed API and my descriptions and examples. Ultimately, I'm still not sure the API is quite right. Furthermore, it isn't a good sign that the implementation is so complex.

I think it is wise to leave channels out of PEP-554, leaving the resulting PEP more focused, especially since it is targeting 3.12.

That said, I still think channels (or whatever we call them) are the best primitive for the concurrency model and anticipate they will be part of the stdlib sooner rather than later. I just don't want PEP 554 to be held up by that. In the meantime, we can give the channels design some time to bake on PyPI.

This sounds like a very good path forward.

Side note: The PEP seems significantly understate the impact of isolating modules

Petr has covered everything I would have said.

+1

@erlend-aasland
Copy link
Contributor

AFAICS, we can close this.

@ericsnowcurrently
Copy link
Member Author

Thus far I've only split out an _xxinterpchannels module. I haven't removed it. It might still be worth keeping for testing purposes (it has uncovered various bugs), regardless of PEP 554.

@erlend-aasland
Copy link
Contributor

FWIW, I'm fine with keeping it; having a broad test suite is never a bad idea.

carljm added a commit to carljm/cpython that referenced this issue Mar 14, 2023
* main: (50 commits)
  pythongh-102674: Remove _specialization_stats from Lib/opcode.py (python#102685)
  pythongh-102660: Handle m_copy Specially for the sys and builtins Modules (pythongh-102661)
  pythongh-102354: change python3 to python in docs examples (python#102696)
  pythongh-81057: Add a CI Check for New Unsupported C Global Variables (pythongh-102506)
  pythonGH-94851: check unicode consistency of static strings in debug mode (python#102684)
  pythongh-100315: clarification to `__slots__` docs. (python#102621)
  pythonGH-100227: cleanup initialization of global interned dict (python#102682)
  doc: Remove a duplicate 'versionchanged' in library/asyncio-task (pythongh-102677)
  pythongh-102013: Add PyUnstable_GC_VisitObjects (python#102014)
  pythonGH-102670: Use sumprod() to simplify, speed up, and improve accuracy of statistics functions (pythonGH-102649)
  pythongh-102627: Replace address pointing toward malicious web page (python#102630)
  pythongh-98831: Use DECREF_INPUTS() more (python#102409)
  pythongh-101659: Avoid Allocation for Shared Exceptions in the _xxsubinterpreters Module (pythongh-102659)
  pythongh-101524: Fix the ChannelID tp_name (pythongh-102655)
  pythongh-102069: Fix `__weakref__` descriptor generation for custom dataclasses (python#102075)
  pythongh-98169 dataclasses.astuple support DefaultDict (python#98170)
  pythongh-102650: Remove duplicate include directives from multiple source files (python#102651)
  pythonGH-100987: Don't cache references to the names and consts array in `_PyEval_EvalFrameDefault`. (python#102640)
  pythongh-87092: refactor assemble() to a number of separate functions, which do not need the compiler struct (python#102562)
  pythongh-102192: Replace PyErr_Fetch/Restore etc by more efficient alternatives (python#102631)
  ...
Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this issue Mar 27, 2023
ericsnowcurrently added a commit that referenced this issue Jun 2, 2023
…-105258)

The _xxsubinterpreters module was meant to only use public API.  Some internal C-API usage snuck in over the last few years (e.g. gh-28969).  This fixes that.
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 26, 2023
…le (pythongh-105258)

The _xxsubinterpreters module was meant to only use public API.  Some internal C-API usage snuck in over the last few years (e.g. pythongh-28969).  This fixes that.
(cherry picked from commit e6373c0)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit that referenced this issue Jul 27, 2023
…ule (gh-105258) (gh-107303)

The _xxsubinterpreters module was meant to only use public API.  Some internal C-API usage snuck in over the last few years (e.g. gh-28969).  This fixes that.
(cherry picked from commit e6373c0)

Co-authored-by: Eric Snow <ericsnowcurrently@gmail.com>
ericsnowcurrently added a commit that referenced this issue Jul 27, 2023
…-107359)

The _xxsubinterpreters module should not rely on internal API.  Some of the functions it uses were recently moved there however.  Here we move them back (and expose them properly).
@encukou
Copy link
Member

encukou commented Aug 2, 2023

From #107359:

The _xxsubinterpreters module should not rely on internal API. Some of the functions it uses were recently moved there however. Here we move them back (and expose them properly).

To expose unstable API properly, it needs documentation and tests. Are you planning to add those?

@ericsnowcurrently
Copy link
Member Author

I will.

@ericsnowcurrently
Copy link
Member Author

ericsnowcurrently commented Aug 3, 2023

See gh-107784.

@erlend-aasland
Copy link
Contributor

To expose unstable API properly, it needs documentation and tests. Are you planning to add those?

Documentation has been split out to a separate issue. IIRC, there is already an issue regarding test coverage for subinterpreters. Given this, I don't see the need for holding this issue open anymore; I recommend closing this issue.

@erlend-aasland erlend-aasland added the pending The issue will be closed if no feedback is provided label Jan 12, 2024
@encukou encukou closed this as completed Jan 25, 2024
@erlend-aasland erlend-aasland removed the pending The issue will be closed if no feedback is provided label Jan 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.12 bugs and security fixes extension-modules C modules in the Modules dir topic-subinterpreters type-feature A feature request or enhancement
Projects
Status: Done
Development

No branches or pull requests

4 participants