-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
base: main
Are you sure you want to change the base?
Conversation
…amples in each sub-section
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.
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
* `astropy <https://www.astropy.org/>`_, a core package for astronomy, which | ||
recommends that users install ``astropy[recommended]``. Some of the | ||
dependencies in ``recommended``, such as `scipy <http://www.scipy.org>`_, are | ||
used in a number of places in astropy for common | ||
functionality and scipy should ideally be a required dependency, but there has | ||
been reluctance to so as it is not a lightweight dependency and advanced users | ||
who want minimal installs want the ability to opt out of it (however, as | ||
described above, this places a burden on the majority of regular uses to | ||
satisfy a use case for advanced users). | ||
* `fastapi <https://fastapi.tiangolo.com/>`_, a web framework, which recommends | ||
that users install ``fastapi[standard]`` in order to include commonly needed | ||
dependencies. | ||
* `tensorflow <https://www.tensorflow.org>`_, a machine learning platform, which | ||
recommends that users install ``tensorflow[and-cuda]`` since CUDA support is | ||
commonly needed by users. |
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.
This section feels a bit wordy, and repeats the prose description from this section. Perhaps you could list the packages, and say that these fit the description in the paragraph above?
E.g.
There are several packages that demonstrate this pattern by encouraging users to include extra dependencies by default, such as:
* (link to astropy): ``astropy[...]``
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.
Done
peps/pep-0771.rst
Outdated
Real-world examples of packages that allow users to select different backends or | ||
frontends with extras include: |
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.
Likewise, I wonder if there's a better (more concise) way of integrating the description of the pattern with the examples.
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.
Done
peps/pep-0771.rst
Outdated
Examples of usage | ||
================= |
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.
Please stick to the suggested sections in PEP 12. Perhaps you could add "examples of usage" as a sub-section, but "how to teach this" is broader than simply stating examples.
Examples of usage | |
================= | |
How to teach this | |
================= |
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 moved the examples into the Specification section
Co-authored-by: Adam Turner <[email protected]>
@AA-Turner - thanks for the review so far! I've added the |
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.
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.
@@ -70,18 +82,31 @@ or backend package. There is currently no mechanism in Python packaging | |||
infrastructure to disallow conflicting or incompatible extras to be installed, | |||
and this PEP does not change that. | |||
|
|||
Examples of packages that require at least one backend or frontend to work and |
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.
Thanks for bring up this @astrofrog. This has been a usability pain point for a long while.
|
||
the ``extra1`` dependency would be included. If the user instead uses:: | ||
the ``extra1`` dependency would be included. If the user instead uses: |
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.
the ``extra1`` dependency would be included. If the user instead uses: | |
the default ``extra1`` dependency would be included. If the user instead uses: |
pip install package[extra2] | ||
.. code-block:: console | ||
|
||
$ pip install package[extra2] |
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 recommend updating line 196 to be explicit that the non-default extra2
would be installed, but the default extra1
would not be installed.
|
||
pip install package package[extra2] | ||
$ pip install package package[extra2] |
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.
It may make sense to include a sample dependency tree here since I think that is far more likely than the above pip
command.
The first time I read this my eyes skipped over the first package in the command.
|
||
should install the default extras. | ||
|
||
Note that ``package[]`` would continue to be equivalent to ``package`` and would | ||
not be provided as a way to install without default extras (see the `Rejected | ||
Ideas`_ section for the rationale). | ||
|
||
We also note that some tools such as `pip`_ currently ignore unrecognized | ||
extras, and emit a warning to the user to indicate that the extra has not been |
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.
...unrecognized non-default extras ???
extras further down the road, as it will allow ``package[minimal]`` to work | ||
for versions prior to when defaults were adopted. | ||
|
||
Avoiding adding too many default dependencies |
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.
Avoiding adding too many default dependencies | |
Avoiding the addition of many default dependencies |
incompatible. In this case, we recommend against making using the default extra | ||
feature for any extra that contains a dependency that could be incompatible with | ||
another. |
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.
incompatible. In this case, we recommend against making using the default extra | |
feature for any extra that contains a dependency that could be incompatible with | |
another. | |
incompatible. In this case, we recommend against using the default extra | |
feature for any extra that contains a dependency that could be incompatible with | |
another. |
|
||
.. code-block:: console | ||
|
||
$ pip install package |
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.
Perhaps add a comment after the command to specify what is being installed (A as a default)
Package users | ||
------------- | ||
|
||
Package users should be provided with clear installation instructions that show |
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.
As a napari developer that deals with qt versions and dependencies, clarity is key as a user.
will need to carefully consider how to treat default extras, and this may | ||
imply a non-negligible amount of work and discussion. | ||
|
||
It is impossible for a PEP such as this to exhaustively consider each of the |
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.
Hmm... @astrofrog, do you expect the impact to be felt most by package maintainers, packaging tool providers, packaging repositories, or package users? It's not clear to me.
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:
default-optional-dependencies
key has been renamed todefault-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/