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

PEP 684: Add a PEP for making the GIL per-interpreter #2387

Merged

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrently ericsnowcurrently commented Mar 8, 2022

This is a more focused version of #2212. I'm also considering moving the "Extra Context" and "Consolidating Runtime Global State" out of the PEP.

@AA-Turner
Copy link
Member

You can take 684.

A

Copy link
Member

@AA-Turner AA-Turner left a comment

Choose a reason for hiding this comment

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

The rendering is failing as of the dates:

pep-06XX-per-interpreter-gil.rst Outdated Show resolved Hide resolved
pep-06XX-per-interpreter-gil.rst Outdated Show resolved Hide resolved
@ericsnowcurrently ericsnowcurrently changed the title PEP 6XX: Add a PEP for making the GIL per-interpreter. PEP 684: Add a PEP for making the GIL per-interpreter. Mar 8, 2022
@AA-Turner
Copy link
Member

@ericsnowcurrently the filename needs to be pep-0684.rst -- I can't do review comments on that, sorry!

A

@CAM-Gerlach CAM-Gerlach changed the title PEP 684: Add a PEP for making the GIL per-interpreter. PEP 684: Add a PEP for making the GIL per-interpreter Mar 8, 2022
Copy link
Member

@JelleZijlstra JelleZijlstra left a comment

Choose a reason for hiding this comment

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

A few typo fixes

pep-0684-per-interpreter-gil.rst Outdated Show resolved Hide resolved
pep-0684-per-interpreter-gil.rst Outdated Show resolved Hide resolved
pep-0684-per-interpreter-gil.rst Outdated Show resolved Hide resolved
pep-0684-per-interpreter-gil.rst Outdated Show resolved Hide resolved
pep-0684-per-interpreter-gil.rst Outdated Show resolved Hide resolved
pep-0684-per-interpreter-gil.rst Outdated Show resolved Hide resolved
pep-0684-per-interpreter-gil.rst Outdated Show resolved Hide resolved
@CAM-Gerlach
Copy link
Member

I'll defer my review until the filename is updated, to avoid any review comments getting lost in the move.

Also as a small reminder, no periods in titles (and commit summary lines, which use the same syntax, per standard convention—though those will be squashed here, so no need to worry about that now)

@JelleZijlstra JelleZijlstra self-requested a review March 8, 2022 18:20
@CAM-Gerlach CAM-Gerlach self-requested a review March 8, 2022 18:23
@ericsnowcurrently ericsnowcurrently marked this pull request as ready for review March 8, 2022 22:00
@ericsnowcurrently ericsnowcurrently requested a review from a team as a code owner March 8, 2022 22:00
pep-0684.rst Show resolved Hide resolved
pep-0684.rst Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

Hopefully my review come through, since while all the comments are marked pending and listed here, significantly fewer are actually counted next to the review button up top.

NB, just in case you aren't already aware, if you go to the Files tab, you can apply any or all of the suggestions at once by just clicking Add to batch on each suggestion, and then clicking Commit at the end.

Just to note, there's a fairly wide range of acceptable sentence lengths and plenty of style and judgement involved, but when it gets too far outside of those bounds to either direction and without much variety, readers can really struggle to follow the flow. I've conservatively suggested some limited changes to reduce choppiness in those instances where it is most directly problematic (typically when co-located with other issues) but you might want to think about it elsewhere as well.

Thanks!

pep-0684.rst Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Show resolved Hide resolved
pep-0684.rst Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
pep-0684.rst Outdated Show resolved Hide resolved
ericsnowcurrently and others added 2 commits March 8, 2022 19:14
Co-authored-by: CAM Gerlach <[email protected]>
@JelleZijlstra
Copy link
Member

You should also add yourself to CODEOWNERS (and probably merge main first, or you'll conflict with the changes from PEP 685).

@ericsnowcurrently ericsnowcurrently merged commit 9988fd6 into python:main Mar 9, 2022
@ericsnowcurrently ericsnowcurrently deleted the per-interpreter-gil-focused branch March 9, 2022 03:27
@ericsnowcurrently
Copy link
Member Author

Thanks for the feedback, @CAM-Gerlach, @AA-Turner, and @JelleZijlstra. Your attention to detail reflects your obvious care for this great community.

Copy link
Member

@CAM-Gerlach CAM-Gerlach left a comment

Choose a reason for hiding this comment

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

As GitHub's UI shows, the CODEOWNERS file is now broken again, as @eduardo-elizondo was added as a code owner for a different PEP in this PR, but they do not have write permissions on this repository.

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Mar 9, 2022

Just to note, it seems this was merged while I had a followup review in progress (for which I had reached out to a another highly skilled professional writer to get a second opinion on the objective necessity of several of the suggestions that had not been applied, both for your and my benefit), and before any PEP Editor had approved or had a change to review the final version (thus, missing the CODEOWNERS breakage this caused for another PEP that required a followup PR). In the future, a bit of patience would have avoided these issues.

Also, if there's an issue with either the volume or the extent of the review suggestions, I would appreciate frank and honest feedback. We try to take care to not suggest changes that are a matter of personal preference, as opposed to resolve a defect with the text that would impair readability, understanding or prose quality, in our carefully-considered professional judgement as experienced technical writers and copyeditors. However, we do occasionally make mistakes (which I pay particular attention to, as a rare and consequently valuable learning opportunity), and we also certainly don't want to waste both your and our time with requested changes that will ultimately be rejected.

@ericsnowcurrently
Copy link
Member Author

Well, I appreciate your effort! I suppose I'm not looking to put a ton of effort into the finer points of technical writing, relative to the general content of my proposals. I aim for good enough. Honestly, I rarely have enough free time to polish it up further, nor do I consider that a higher priority than most of the other things on my TODO list. Resolving editorial reviews requires a similar effort to doing my own detailed editing in the first place. I am definitely grateful for your insight but don't ever want you to feel like you wasted your time if I don't take your advice. Alternately, leaving further polish up to someone else than myself makes me uncomfortable if they aren't a co-author on the PEP. So I'm not sure what to say. Getting more than a little feedback on a PEP (outside python-dev) is a new experience for me and I'm not sure what to think.

Anyway, I guess I'm not clear on what the role of the PEP editors is. My recollection is that they used to be the folks with commit rights on the PEPs repo, but then all core devs were given commit rights on PEPs and the editors weren't especially involved any more. Now it seems like we have a few of you that have picked up the torch and are focused on serious caretaking. That's great! I just need to adjust and we all need to find a balance that doesn't add overhead or take away from the free time of our volunteers, ourselves included.

@gvanrossum
Copy link
Member

CAM, you need to back off. As a PEP editor you are supposed to serve PEP authors, not overwhelm them with a barrage of nits and complain if not all of your feedback was incorporated.

Also, be less verbose. Please.

@JelleZijlstra
Copy link
Member

Thanks @ericsnowcurrently for your thoughtful feedback and @CAM-Gerlach for your dedication to technical writing. I agree with Eric and Guido that we have a real risk of making PEP writing too high-friction for busy core devs. Our role as PEP editors should be to help with keeping PEPs high quality, but ultimately authors own their PEPs. I'm going to work with CAM to make sure we improve the situation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants