PEP 771: Amendments following discussion #4428
Conversation
…no default extras
There was a problem hiding this comment.
Thanks for the update @astrofrog. I think you have done a great job clarifying points. I had some confusion when reading the pytest and 1500 plugins example. Otherwise, I found the changes improved clarity and understanding.
|
Thanks a lot @astrofrog for the revision. All good with me 👌 |
Co-authored-by: Carol Willing <carolcode@willingconsulting.com> Co-authored-by: Pradyun Gedam <pradyunsg@gmail.com>
|
@willingc - thank you for the review, I think all the issues/suggestions you raised should be addressed. @pradyunsg - thank you for the rewording suggestion! |
|
@willingc - thank you for approving! Once this is deemed ready to merge, based on previous iterations it would be easier to merge and I can then open a new DPO thread and open a PR to add the new DPO link to the PEP. |
peps/pep-0771.rst
Outdated
| If there is a desire to implement a better solution in future, we believe this | ||
| PEP should not preclude this. For example, one could imagine in future adding the | ||
| ability for an extra to specify *which* default extras it disables rather than | ||
| disabling all default extras, but the default could still be for explicitly |
There was a problem hiding this comment.
There's a lot of "default"s floating around here, which makes this paragraph a bit difficult to parse. I'm not taking a shot at rewording it though.
There was a problem hiding this comment.
Wow yes that was a lot of 'defaults' - reworded
| * Since prior to this PEP, ``package[]`` was equivalent to ``package``, | ||
| authors will be able to document ``package[]`` as a backward-compatible | ||
| universal way of getting a minimal installation. For packages that define | ||
| default extras, installing ``package[]`` will always give a minimal |
There was a problem hiding this comment.
A question occurs to me as I'm reading this: is package[] allowed even for packages that do not define a default extra? Presumably, that would just be a no-op. But I wonder if the PEP needs to be explicit about that case (if it isn't already)?
There was a problem hiding this comment.
Yes package[] will continue to be allowed for packages that don't use default extras, and I've now added a sentence to make this explicit.
peps/pep-0771.rst
Outdated
| dependency. In some cases, if a package is widely used by many others, if it | ||
| adds a default extra, then unless all downstream packages update their | ||
| dependencies to specifically request a minimal installation, the defaults will | ||
| Default extras should generally be treated with the same "weight" as required dependencies. When a package is widely used, introducing a default extra will result in that extra's dependencies being transitively included -- unless all downstream packages are updated to explicitly opt out using minimal installation specifications. |
There was a problem hiding this comment.
Yes I think this was the result of a problematic GitHub suggestion earlier in this PR - fixed
…] will continue to be equivalent to package.
|
@warsawnv - thanks for the review! I've implemented your suggestions/comments. |
| It is worth noting that the recently-accepted :pep:`751` defines a new file | ||
| format which is intended to replace alternatives such as the ``pip freeze`` | ||
| output and other tools in future. The new file format is designed so that the | ||
| packages in the file are installed *without* resolving dependencies, which means | ||
| that it will be fully compatible with default extras as specified in this PEP, | ||
| and will avoid the issue with ``pip freeze``/``pip install -r`` mentioned above. |
There was a problem hiding this comment.
... but since package[] is now recommended as the canonical "no extras", freeze could simply, trivially be "bugfixed" to always list every package with []. A new format is not needed, and the change in output format is backwards-compatible.
There was a problem hiding this comment.
Yes that's definitely an option though I think some people had objections to this approach previously - in any case this could be discussed in the next DPO thread, but my feeling is that it isn't needed since PEP 751 is happening anyway, regardless of PEP 771
There was a problem hiding this comment.
pip freeze is blatantly in violation of this proposed PEP, if it does not include [] for at least any packages that include default extras.
Whether it does so for all packages or just the ones that have default extras is a development tradeoff between slightly uglier files (more [] than are necessary) and more work spent to analyze the metadata of installed packages to determine whether there are Default-Extra fields to suppress.
Of course "pip freeze is deprecated so it will just not follow the PEP" is a potential decision to make but that should be explicitly spelled out here. You went to quite some effort to write this section of the backwards compatibility documentation, why stop halfway through? :)
There was a problem hiding this comment.
That's interesting. I don't love that [] syntax will essentially be propagated into every entry in the lockfile, but maybe that's okay since people shouldn't be worried too much about the lockfile contents.
There was a problem hiding this comment.
@warsawnv to be clear, the new lock file format from PEP 751 won't have a need for [] even for packages with default extras since that lock file should be interpreted without resolving dependencies.
There was a problem hiding this comment.
In any case, the suggestion of including [] in pip freeze output is probably something that should be discussed in the next DPO thread rather than discuss it extensively here?
|
Just to check, what are the next steps here - do we need @pradyunsg to approve the changes before this can be merged? |
Following the initial publication of PEP 771, there has been a lot of discussion in this DPO thread. This PR makes changes to PEP 771 that hopefully addresses most of the concerns there.
The main changes are:
package[]is now considered to be the primary way to specify that a package without any default extras is being requestedpip freezeissues that might arise from PEP 771In addition to this, there are other smaller additions that aim to address some of the concerns in the DPO discussion.
Once this PR is merged, I'll then open a new discussion thread and open a trivial PR here to add the link to the PR.
cc @pradyunsg @DEKHTIARJonathan @henryiii
@willingc @hugovk @AA-Turner - just thought I'd also tag you since you were involved in the reviews of the previous iterations.
📚 Documentation preview 📚: https://pep-previews--4428.org.readthedocs.build/