Skip to content

PEP 788: Address feedback from first discussion round #4400

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

Merged
merged 55 commits into from
May 28, 2025

Conversation

ZeroIntensity
Copy link
Member

@ZeroIntensity ZeroIntensity commented May 3, 2025

  • Change is either:
    • To a Draft PEP
    • To an Accepted or Final PEP, with Steering Council approval
    • To fix an editorial issue (markup, typo, link, header, etc)
  • PR title prefixed with PEP number (e.g. PEP 123: Summary of changes)

The key changes I made here are:

  • Interpreter references now have a dedicated type (PyInterpreterRef and PyInterpreterWeakRef *).
  • Add a terminology section for clarification on what things like "interpreter" refer to in the PEP.
  • Add a rejected idea for the previous approach (mainly using PyInterpreterState * for refs).
  • Shifted away from "non-daemon thread states," so now interpreter references and thread states are
    kept separate.
  • Editorial: Lots of clarity fixes (many had trouble understanding the PEP, unfortunately).
  • Editorial: Headings are now in title case ("The Apple is Weird and Confusing") rather than sentence case ("The apple is weird and confusing").

📚 Documentation preview 📚: https://pep-previews--4400.org.readthedocs.build/pep-0788/

@ZeroIntensity ZeroIntensity marked this pull request as ready for review May 4, 2025 17:55
@ZeroIntensity ZeroIntensity requested a review from vstinner as a code owner May 4, 2025 17:55
@ZeroIntensity
Copy link
Member Author

@godlygeek I can't add you as a reviewer, but I'd appreciate your opinion on some of the new things I'm proposing here.

@ZeroIntensity ZeroIntensity requested a review from AA-Turner May 4, 2025 20:02
@ZeroIntensity
Copy link
Member Author

Bump @AA-Turner @vstinner now that the beta is out. There's no rush considering we have a year until the 3.15 freeze, though.

ZeroIntensity and others added 3 commits May 9, 2025 07:21
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
Co-authored-by: Victor Stinner <vstinner@python.org>
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. I haven't gotten though the whole proposal, but did start by trying to understand what we're trying to solve here. I've left short notes for each of the motivation sections. Ultimately, I think are some bugs to be addressed separately and the other concerns could be solved without replacing the existing API. That doesn't mean a new API isn't warranted, but let's see how much value it would add first. (I'd be glad to hop into a call to discuss this more.)

@ZeroIntensity
Copy link
Member Author

I'd be glad to hop into a call to discuss this more.

That'd be great. Shoot me a message on Discord so we can figure something out. (Note that I'll have limited availability this week.)

@ZeroIntensity
Copy link
Member Author

I think most of Eric's comments will fit better on the public discussion thread, so I didn't deal with them too much.

@colesbury I've addressed most of your concerns. In particular, thread states and interpreter refs are now separate, so there's no more "non-daemon thread states." I'd appreciate your rubber-stamp before landing this, would you mind doing a quick once-over when you get a chance?

@ZeroIntensity
Copy link
Member Author

I had to resolve many of the comments because it won't let us merge this while things are open. If there's anything that wasn't fully addressed, please bring it up on the thread!

@vstinner, we're ready to go.

@vstinner vstinner merged commit 81424fc into python:main May 28, 2025
5 checks passed
@vstinner
Copy link
Member

I merged the PR, thanks.

@ZeroIntensity ZeroIntensity deleted the pep-788-round-1 branch May 28, 2025 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants