-
Notifications
You must be signed in to change notification settings - Fork 267
PEP 440 handling of prereleases for Specifier.contains
, SpecifierSet.contains
, and SepcifierSet.filter
#897
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
base: main
Are you sure you want to change the base?
PEP 440 handling of prereleases for Specifier.contains
, SpecifierSet.contains
, and SepcifierSet.filter
#897
Conversation
2519ee8
to
dd2c085
Compare
e1352dc
to
8b051ec
Compare
8b051ec
to
b44bfdb
Compare
Signed-off-by: Henry Schreiner <[email protected]>
Okay, I've reviewed this, I'm open to more simplification but I don't currently see anywhere obvious. @henryiii I think you asked me at the packaging summit if there had been any discussion on this, unfortunately the discussion on interpreting the pre-release specification comes from many scattered sources. My ultimate motivation here is that I would like a standards compliant fast resolver writen in Python. And there are certain logical operations you need to be able to do with specifiers to write fast resolvers, such as taking the intersection of two specifiers, or having a canonical normal form of a specifier. Because of pre-releases these operations aren't obvious even when following the spec, but because packaging doesn't consistently follow the spec it's currently impossible to implement these with packaging right now. There are a couple of discussions at:
These conversations always get stuck because there is a confusion about how SHOULD pre-releases behave, so I have carefully spent time to understand what pre-releases SHOULD do. Feel free to ask any questions about this. But this does also fix a bunch of pip issues (pypa/pip#7579, pypa/pip#9121, pypa/pip#12469). |
BTW, here is a specific invariant that this will fix that is required for certain resolution algorithms:
This does not currently hold for packaging >>> list(SpecifierSet('<=2').filter(['1.0a1']))
[]
>>> list(SpecifierSet('<=2,<=4b1').filter(['1.0a1']))
['1.0a1'] But it SHOULD according to this line in the spec:
|
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'll merge next week unless there are objections!
I'm going to ping @pradyunsg who asked by Brett to review my previous PR #794 that affected pre-releases, in case they have any input. Also in case they have any thoughts, I'm going to ping @pfmoore, as I was very much inspired to raise this PR by our discussion in this thread: https://discuss.python.org/t/proposal-intersect-and-disjoint-operations-for-python-version-specifiers/71888 I will also note, for tools using packaging to determine if a pre-release should be selected this will make that more likely, such as in pip, and such tools may want to give control to the user to prevent pre-releases (for pip I have an open issue pypa/pip#13221). When this lands in pip I will do a careful review of how pip is using |
I'll note that although I was arguing in that thread that the existing spec isn't ambiguous, and tools that don't implement the spec as written are wrong (as opposed to having an "interpretation" of ambiguous wording), I'm not actually a big fan of the current spec1. The whole pre-release handling behaviour is far too much a case of "do what I mean" for my liking. I think we would be much better off with a simpler, clearer spec, even if that meant people had to be more explicit about whether they wanted pre-releases. Of course, that would require a new PEP, rather than just a PR to the packaging project... All of which is to say that if this change causes incompatible behaviour changes in pip, I'd be very sympathetic with users who objected. "The spec made us do it" is a bit of a cop-out IMO. But let's wait and see what the review comes up with - I don't want to start a panic for no reason. Footnotes
|
I would happily support someone else to work on that 😉. I'm of the opinion that having a spec become more mathematically simple would be a big breaking change (namely At the moment, packaging (and by extension pip) almost follows the spec, Poetry does follow the spec, uv has made design choices here that are explictly not following the spec. So I would rather just fix packaging to follow the spec, and deal with future spec changes independently.
I agree with that this should not be motivated by implementing the spec for specs sake. But this causes real problems in pip (pypa/pip#7579, pypa/pip#9121) and packaging (#854), and as mentioned it breaks important invariants for implementing more advanced resolution algorithms (#897 (comment)). Further, packaging implements the spec correctly in some places ( I think these all build a strong case for changing the behavior, I also think the real world impact will be fairly small, there was very little fall out from #794. |
Fixes #854
Fixes #895
Fixes #856
Fixes #917
Builds on top of and, if accepted, supersedes #872
The goal of this is to have packaging specifier methods consistently follow the recommendation of PEP 440 and the specification:
Specifically this PR fixes
Specifier.contains
,SpecifierSet.contains
, andSepcifierSet.filter
, but does not change the behavior ofSepcifier.filter
which already correctly complies with the above points.This PR moves all the logic of implementing this into the
filter
methods and then makes thecontains
methods fairly simple wrappers around thefilter
methods.This PR adds many tests, but could probably do with thousands more.