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 771: Amendments following pre-publish discussion #4238

Merged
merged 19 commits into from
Feb 6, 2025

Conversation

astrofrog
Copy link
Contributor

@astrofrog astrofrog commented Jan 29, 2025

There has been an extensive discussion going on in this DPO thread while the original PR for PEP 771 was being reviewed. Rather than continuously update that PR, @pradyunsg suggested that that PEP should be published as-is initially and that changes based on the pre-publish discussion should be made in a separate PR. This is the separate PR 🎉

The main changes are:

  • Real-life examples of packages have been added in several places by popular demand
  • Most of the 'How to teach this' section has been moved to a new 'Examples of usage' section, and 'How to teach this' has been filled with new content
  • The backward-compatibility section has been expanded to address concerns raised during the discussion
  • An open issue has been added as it is an unresolved point that will require more discussion
  • The default-optional-dependencies key has been renamed to default-optional-dependency-keys

Once this PR is merged, I'll then open the main discussion thread and open a trivial PR to add the link to the PR.

cc @pradyunsg @DEKHTIARJonathan


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

@astrofrog astrofrog requested a review from pradyunsg as a code owner January 29, 2025 11:44
@astrofrog astrofrog changed the title PEP 771 - edits following pre-publish discussion PEP 771: edits/additions following pre-publish discussion Jan 29, 2025
@astrofrog astrofrog changed the title PEP 771: edits/additions following pre-publish discussion PEP 771: Edits/additions following pre-publish discussion Jan 29, 2025
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.

Brief review, I have only read up to the changes to Specification in depth.

Edit: Please can you add the linked thread to Post-History?

Post-History:
  `15-Jan-2025 <https://discuss.python.org/t/77892/>`__,

peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
@AA-Turner AA-Turner changed the title PEP 771: Edits/additions following pre-publish discussion PEP 771: Amendments following pre-publish discussion Jan 29, 2025
@astrofrog
Copy link
Contributor Author

@AA-Turner - thanks for the review so far! I've added the Post-History as requested (and have implemented the other comments). I also moved the examples section into a sub-section of the specification (as opposed to how to teach this) since it felt more natural as the last subsection of the specification, and there is already an extensive 'how to teach this' - however if you feel strongly this should be a subsection of 'how to teach this' I can move it, so just let me know.

Copy link
Contributor

@willingc willingc 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 taking this on @astrofrog. I have several comments and questions after reading the PEP but not all of the DPO discussion. I agree with the Motivations, but I'm wrestling a bit if we're communicating the approach clearly enough. I'm not advocating changing the approach but making it more explicit for readers.

peps/pep-0771.rst Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
@astrofrog
Copy link
Contributor Author

@willingc - thank you for your review! I will try and implement the comments by early next week.

astrofrog and others added 3 commits February 3, 2025 10:46
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>
@astrofrog
Copy link
Contributor Author

@willingc - thanks again for the review! I've tried to implement your comments - there are a couple of remaining questions in some of the in-line discussions regarding whether any further text needs to be added or modified, so just let me know what you think.

@willingc
Copy link
Contributor

willingc commented Feb 3, 2025

Skimming through this @astrofrog, I think that you have captured well the edits. 😄 I will take a look at the inline later tomorrow. Thank you!

@astrofrog
Copy link
Contributor Author

This branch is now out of date with main - should I just add a merge commit to fix that? (in the last PR I rebased but was told it wasn't ideal for reviews so just want to make sure I do the right thing)

@hugovk
Copy link
Member

hugovk commented Feb 4, 2025

It's fine, there's no need to update from main, unless there's something you specifically need from there (or maybe if this PR had been open a long time and we want to check if the CI still passes).

Copy link
Contributor

@willingc willingc left a comment

Choose a reason for hiding this comment

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

Beautiful. Thanks @astrofrog.

@astrofrog
Copy link
Contributor Author

Thanks for the approval! Once this is merged, I will open the main discussion thread on DPO and will open a follow-up PR updating the discussion link.

Copy link
Member

@hugovk hugovk left a comment

Choose a reason for hiding this comment

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

Any final comments from @pradyunsg? Otherwise let's merge.

peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Outdated Show resolved Hide resolved
astrofrog and others added 2 commits February 5, 2025 14:13
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>
@pradyunsg
Copy link
Member

Any final comments from @pradyunsg?

Nope. :)

@astrofrog
Copy link
Contributor Author

astrofrog commented Feb 6, 2025

@AA-Turner @hugovk @willingc - I've made the thread and have added Discussion-To here - would you or someone else be able to merge shortly? Thanks!

peps/pep-0771.rst Outdated Show resolved Hide resolved
peps/pep-0771.rst Show resolved Hide resolved
@AA-Turner AA-Turner enabled auto-merge (squash) February 6, 2025 17:00
@AA-Turner AA-Turner merged commit 0899c62 into python:main Feb 6, 2025
4 of 5 checks passed
@astrofrog
Copy link
Contributor Author

@AA-Turner - thank you for fixing and merging this! 🙏 (Sorry I didn't realize the thread also needed to go in Post-History)

Comment on lines +732 to +740
Packaging repository maintainers
--------------------------------

The impact on individuals who repackage Python libraries for different
distributions, such as `conda <https://docs.conda.io>`_, `Homebrew
<https://brew.sh/>`_, linux package installers (such as ``apt`` and ``yum``) and
so on, needs to be considered. Not all package distributions have mechanisms
that would line up with the approach described. In fact, some distributions such
as conda, do not even have the concept of extras.

Choose a reason for hiding this comment

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

Speaking from "the other side", I would have to say that pip functionality is a strict subset of the functionalities available by repackagers. Primarily based on my knowledge of linux package managers, but also with regard to conda and spack. I know you're specifically calling out conda here as not being able to support the PEP, but I'm saying conda specifically can -- and so can everyone else.

I feel like the python community by and large has a lot of expertise in how pip works but very little study into how anything other than pip works. :( There is a lot of interesting practical research that's been done over the decades. Perhaps you're already planning to do that though -- the wording of this section somehow gives me the feeling that it's a bit of an open TODO to flesh out the actual considerations! Feel free to ask me for more details if so.

(Incidentally: I oppose the PEP anyway because despite all the functionality of it being representable by repackagers, the PEP largely makes decisions that explicitly close the door on what I would consider as the primary functionality of "extras" as they are used in other ecosystems. There is very little to gain here without taking a holistic view of things. And much to lose if you e.g. define what is currently unspecified/invalid as newly-valid without adding new functionality using it. :P)

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.

6 participants