pypa / pip

The Python package installer
https://pip.pypa.io/
MIT License
9.45k stars 3k forks source link

Follow PEP 440 and install non-dev versions if `--pre` is not given #7579

Open flying-sheep opened 4 years ago

flying-sheep commented 4 years ago

People installing scanpy==1.4.4.post1 (which specifies anndata>=0.6.22rc1) currently get anndata==0.7rc2. I think they should get anndata==0.6.22 instead.

What's the problem this feature will solve? Currently pip treats the >= and rc parts separately:

I don’t think that makes sense: When you specify a dependency as >=X.YrcZ, you mean “I need X.Y and consider the prereleases of X.Y to be stable enough”. It doesn’t mean you extend trust to any future prereleases of that package.

Describe the solution you'd like If rc, alpha, beta, … appears in a version, >= should mean “get me any higher development versions of that version, or any higher release versions” E.g. >=0.6.22a1 should mean 0.6.22a2, 0.6.22b1, 0.6.22rc2, are OK, and any stable version >=0.6.22

Follow PEP 440 as described below:

pfmoore commented 4 years ago

Pip should follow PEP 440 here. From my reading of the section on pre-releases, it states that >=0.6.22rc1 should exclude 0.6.22rc1 unless the user explicitly requests pre-releases (via --pre, in pip's case) - because of the statement

Pre-releases of any kind, including developmental releases, are implicitly excluded from all version specifiers, unless they are already present on the system, explicitly requested by the user, or if the only available version that satisfies the version specifier is a pre-release.

(note, all version specifiers).

So I'd argue that pip is incorrect here¹, but that the correct interpretation is not what you are suggesting.

If you want the behaviour you describe, it would probably require a PEP change.

(There's also a question as to whether the behaviour you're describing just allows pre-releases for the one package, whereas --pre allows pre-releases for all packages. That's an additional complication that should be addressed in any PEP update).

¹ The PEP uses the term SHOULD, so it is arguable that pip is technically compliant, insofar as it doesn't have to satisfy SHOULD clauses. But IMO that's just avoiding the issue.

flying-sheep commented 4 years ago

The way the PEP describes it seems fine, especially the part “[unless] the only available version that satisfies the version specifier is a pre-release”.

In @sjfleming’s case:

The PEP uses the term SHOULD, so it is arguable that pip is technically compliant […] But IMO that's just avoiding the issue.

Yeah, I agree with you, saying “we’re technically correct so fuck what the users expects or wants” wouldn’t exactly be nice or helpful.

uranusjr commented 4 years ago

so fuck what the users expects or wants

I wouldn’t say conclusively (yet?) this is what the users expect as a consensus. And pip maintainers most certainly do not intend to send that message if they do decide to stick with the current behaviour. Please keep things reasonable here.

pfmoore commented 4 years ago

we’re technically correct so fuck what the users expects or wants

Please note that it's very hard to find a respectful way to continue this discussion in the light of describing anybody's attitude like this. I'd strongly request that you keep your tone polite in future - assume everyone is acting with the best intentions, but that no-one is immune from making mistakes.

The reality is that the PEP is specific in what it requires, but the implications of those requirements are subtle. Add to that the fact that some of the PEP's requirements are non-mandatory (which is what "should" means - it's not "if you don't do this you're ignoring user expectations", it's simply "a number of possible approaches exist, and tools are allowed to make their own judgement as to which suits their users best") and there are some complexities to sort out.

Pip's current behaviour is based (as far as I can see) on the flexibility allowed by the PEP over how the user may opt into accepting pre-releases, and is assuming that if the user has a dependency that explicitly mentions pre-releases, that implies that they are willing to accept them. On consideration of the case noted here, it looks like that might be too broad an assumption, and we should remove it. I'd be happy to see that happen, but I don't personally have much need for pre-releases, so I'd rather hear views from other people with an actual stake in the behaviour.

By the way, it should also be noted that pip uses packaging.specifiers to do PEP 440 version matching, so any change would likely end up having to be made there, rather than in pip.

flying-sheep commented 4 years ago

I’m sorry, I didn’t imply that anyone here thinks that way, I was trying to be facetious. Please let me rephrase:

I think the behavior the PEP describes is less surprising and feels more consistent and useful than what pip does right now. You pointed out that pip is technically behaving correctly because the PEP says “SHOULD”. You agree with me anyway (“But IMO that's just avoiding the issue”), but I feel like it’s necessary to mention that IMHO anyone taking up that arguments and fixating on the technicalities here wouldn’t be helpful.

Again: Very sorry that I phrased it that way and hurt you.

flying-sheep commented 4 years ago

I don’t know if pips behavior here is intentional. Could it simply be that pip didn’t “decide” on any path here, and the current way is a byproduct of how several parts of pip interact instead of a design decision? Maybe pip’s logic just didn’t take this case into account, so something like my assumption above happens (making the “rc” and “>=” parts into flags separately)

On the surface, the PEP way just seems plain obvious to be. Maybe we could construct a situation in which the current behavior is more desirable, but I think the best thing would be to find out if indeed there was a conscious decision against the PEP behavior at any point at all.

pfmoore commented 4 years ago

Again: Very sorry that I phrased it that way and hurt you.

No problem - I wasn't trying to defend pip's current behaviour, just pointing out that the PEP leaves a lot of details optional. But either way, thanks for the clarification and no need to worry about it any further.

I don’t know if pips behavior here is intentional

I did a check, and it looks like the packaging library is allowing pre-releases if the specifier includes a pre-release in the constraint:

>>> from pip._vendor.packaging import specifiers as s
>>> spec = s.SpecifierSet(">=0.6.22rc1")
>>> spec.prereleases
True

This appears deliberate. From the documentation:

prereleases (bool) – This tells the SpecifierSet if it should accept prerelease versions if applicable or not. The default of None will autodetect it from the given specifiers.

Maybe pip should avoid passing None and only ever use a value reflecting whether the user supplied the --pre argument. The relevant code path in pip starts here. Conversely, maybe the auto-detection done by packaging is unnecessary - the basic behaviour in the PEP seems to cover the practical use cases better than "if the specifier mentions a pre-release, then accept any pre-preleases".

I'm unlikely to have time to dig into this further at any time in the near future (although the work on the new resolver might touch this code, in which case that would be an opportunity to look at this). But if someone else wants to take this on, I'd be fine with that.

pradyunsg commented 4 years ago

Apologies for the terseness -- short on time right now (yay actual weekend).

I think that to update pip's behavior, we should change the interoperability spec -- i.e. the PEP needs to be updated. pip's current behavior is PEP compliant as currently worded. That said, it might help to have clarification in the PEP (or a change as per your suggestion).

I think this should be discussed with the broader audience at Packaging on discuss.python.org or disutils-sig.

Please start a discussion there, linking back to this issue/PR. :)

flying-sheep commented 4 years ago

Yeah, I think the abstraction here is faulty: rc is an attribute of the specifier text, while --pre is a global switch. I don’t know if a Specifier should carry around global context like this, but if it does, that context is separate from the fact if the specifier contains a prerelease or not.

pip's current behavior is PEP compliant as currently worded

Can you please explain what you mean? Do you mean it complies to the way the PEP says it “should” work or do you mean technically compliant because “should” isn’t “must”? Because in the former case I don’t see it:

Pre-releases of any kind, including developmental releases, are implicitly excluded from all version specifiers, unless they are already present on the system, explicitly requested by the user, or if the only available version that satisfies the version specifier is a pre-release.

I find in the light of the “all” it’s pretty clear that “explicitly requested by the user” means something like --pre and not including rc in the specifier. Of course this doesn’t mean the PEP should be worded more clearly!

I started a discussion, it’s thread 3000!

ntextreme3 commented 4 years ago

Linking https://github.com/pypa/pip/issues/4969 which I saw first but is closed (for the next person who finds their way there). I also feel current behavior is confusing.

ankostis commented 3 years ago

I'm wondering if the following PEP440 rule has been 'silently' dropped from recent dependency resolves (and deserves a separate issue):

accept remotely available pre-releases for version specifiers where there is no final or post release that satisfies the version specifier.

As a real example, the package co2mpas depends on a package without a stable release wltp (latest: 0.1.2a0), and at least pip-20.1.1+ cannot install it anymore.

If i remember correctly, pip-19- could that -- is that change on purpose?

pradyunsg commented 3 years ago

@ankostis pip is able to install black as well as furo, which have only pre-release versions available. Please file an issue using the bug report template, providing us a reproducer with clear instructions, in case you still think there might be a regression here. :)

uranusjr commented 3 years ago

I don’t think there is a change in the version specification logic, but a weird quirk in the dependency discovery logic:

$ pip install wltp  # This installs fine.

$ pip install "wltp<1"  # But this fails.
ERROR: Could not find a version that satisfies the requirement wltp<1 (from versions: 0.0.7a0, 0.0.8a0, 0.0.8a2,
0.0.9a1, 0.0.9a3, 0.0.9a4, 0.0.9a5, 0.1.0a0, 0.1.0a1, 0.1.0a2, 0.1.0a3, 0.1.1a0, 0.1.2a0)

The “difference” between pip 19 and 20 you see is due to co2mpas changing its dependencies. pip did not change.

ankostis commented 3 years ago

You right, thank you! My apologies for a false alarm. (i checked down to pip-10.0.1 and the behavior was what you describe).

I guess if that is desired behavior, a message explaining a workaround(#4795) would be much valuable for end-users,