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

Improve error message for wheels with non-compliant versions #9188

Open
bkurtz opened this issue Nov 30, 2020 · 20 comments
Open

Improve error message for wheels with non-compliant versions #9188

bkurtz opened this issue Nov 30, 2020 · 20 comments
Labels
C: dependency resolution About choosing which dependencies to install type: deprecation Related to deprecation / removal.

Comments

@bkurtz
Copy link

bkurtz commented Nov 30, 2020

What did you want to do?
Our organizational development standards prefer that we include a git/vcs hash in most package version numbers. In other environments, this has typically not been problematic, however with python, including a hash in the version number is not PEP440 compliant.

Previous versions of pip have difficulty applying logic (e.g. ~=, >) to these version numbers, but as long as we pin a specific package version, installations would work.

pip install --extra-index-url https://our.local.package.source/artifactory/simple our-package==0.3.0.f038176.11

(apologies, these are internal code and I can't provide a public URL)

Output

ERROR: Exception:
Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/pip/_vendor/resolvelib/resolvers.py", line 171, in _merge_into_criterion
    crit = self.state.criteria[name]
KeyError: 'our-package'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/cli/base_command.py", line 210, in _main
    status = self.run(options, args)
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/cli/req_command.py", line 180, in wrapper
    return func(self, options, args)
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/commands/install.py", line 319, in run
    reqs, check_supported_wheels=not options.target_dir
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/resolution/resolvelib/resolver.py", line 122, in resolve
    requirements, max_rounds=try_to_avoid_resolution_too_deep,
  File "/usr/local/lib/python3.6/site-packages/pip/_vendor/resolvelib/resolvers.py", line 445, in resolve
    state = resolution.resolve(requirements, max_rounds=max_rounds)
  File "/usr/local/lib/python3.6/site-packages/pip/_vendor/resolvelib/resolvers.py", line 310, in resolve
    name, crit = self._merge_into_criterion(r, parent=None)
  File "/usr/local/lib/python3.6/site-packages/pip/_vendor/resolvelib/resolvers.py", line 173, in _merge_into_criterion
    crit = Criterion.from_requirement(self._p, requirement, parent)
  File "/usr/local/lib/python3.6/site-packages/pip/_vendor/resolvelib/resolvers.py", line 82, in from_requirement
    if not cands:
  File "/usr/local/lib/python3.6/site-packages/pip/_vendor/resolvelib/structs.py", line 124, in __bool__
    return bool(self._sequence)
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/resolution/resolvelib/found_candidates.py", line 96, in __bool__
    return any(self)
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/resolution/resolvelib/found_candidates.py", line 20, in _deduplicated_by_version
    for candidate in candidates:
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/resolution/resolvelib/factory.py", line 203, in iter_index_candidates
    version=ican.version,
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/resolution/resolvelib/factory.py", line 145, in _make_candidate_from_link
    link, template, factory=self, name=name, version=version,
  File "/usr/local/lib/python3.6/site-packages/pip/_internal/resolution/resolvelib/candidates.py", line 290, in __init__
    wheel_version = Version(wheel.version)
  File "/usr/local/lib/python3.6/site-packages/pip/_vendor/packaging/version.py", line 298, in __init__
    raise InvalidVersion("Invalid version: '{0}'".format(version))
pip._vendor.packaging.version.InvalidVersion: Invalid version: '0.3.0.f038176.11'

It appears that when installing from wheel, the package version number is parsed using a regex that only matches PEP 440 compliant version numbers, which fails when given a version number containing a git hash.

Additional information
We have found that installing the affected packages from source (by adding --no-binary our-package to the pip command line) acts as a workaround. It is unclear to me why the source installation does not enforce the same version number checks that the binary installation does.

@dstufft
Copy link
Member

dstufft commented Nov 30, 2020

FWIW you can include a git hash in the version using local version numbers, so something like 0.3.0+f038176.11.

@pradyunsg pradyunsg added C: new resolver kind: crash For situations where pip crashes type: bug A confirmed bug or unintended behavior labels Nov 30, 2020
@bkurtz
Copy link
Author

bkurtz commented Nov 30, 2020

Ah, that's good to know and I'd somehow missed that last time I looked through the specs. I'll see if that might be a better long-term solution to our needs (it would be nice to allow other comparison operators besides strict equality).

Still seems like it's a bug that versioning isn't treated the same between binary/source installs.

@dstufft
Copy link
Member

dstufft commented Nov 30, 2020

Yea it should probably treat them the same between binary and source installs for sure.

One thing to pay attention to is that local versions have some uh "interesting" semantics. If you're just generally using strict equality OR you're using the local version to communicate extra information it'll be totally fine. But it might be surprising if you're trying to use local versions as part of the meaningful part of the version.

@pradyunsg
Copy link
Member

pradyunsg commented Nov 30, 2020

--no-binary circumvents all the metadata validation checks today since it's not installing via wheels, but that "loophole" will be plugged, when we drop setup.py install.

@jre21
Copy link

jre21 commented Nov 30, 2020

When was it decided that pip would only support PEP440 compliant version strings, and why wasn't there a deprecation cycle for this? Given that the pip 10.0.0b1 changelog includes

this seems like exactly the sort of change in policy that needs to be communicated to users ahead of time rather than introduced into a release with no fanfare whatsoever.

@pradyunsg
Copy link
Member

pradyunsg commented Nov 30, 2020

When was it decided that pip would only support PEP440 compliant version strings, and why wasn't there a deprecation cycle for this?

This is a part of the changes being made as part of the new dependency resolver. The new dependency resolver is stricter across the board with metadata generated by projects. This behaviour has been a part of pip's new resolver which has been in beta for the past few months, and has had several rounds of publicity and communication telling users to test it since we are making significant changes (such as this one!) to it. As noted multiple times across various communication channels, we have a long list of changes toward being stricter about what metadata pip accepts from packages.

There's more discussion about this specific stricter behaviour in another recently-filed issue: #9183 (comment)

@bkurtz
Copy link
Author

bkurtz commented Dec 1, 2020

So, that sounds like we're okay for the short term using source installs, but the writing is on the wall and we need to move toward PEP-440 compliant versioning for all our packages.

WRT warning or lack thereof about this particular change, you all have done a generally phenomenal job of publicizing the need for additional testing of the new resolver. I guess where I went astray was that I'd been thinking of the "new resolver" as one set of potentially breaking changes, and so once I'd tested against those back in 20.2.3 or whatever a couple of months ago, I was assuming it was all bug fixes the rest of the way out, which seems not to have been the case. Ah well!

In terms of this ticket, it seems like:

  • the existing behavior with strict version checking of wheels is intentional (not a bug)
  • the existing behavior of non-strict checking for source packages is unintentional (arguably a bug) but has plans (elsewhere?) to be addressed by 21.0.

Is there any reason to keep this ticket open, or shall I go ahead and close?

@uranusjr
Copy link
Member

uranusjr commented Dec 2, 2020

Let’s keep this open until at least we provide a better error message for the failure case.

@jakab922
Copy link

jakab922 commented Dec 3, 2020

Can't we make this optional rather than raising an exception? Unfortunately the way we version packages is not conforming with this and we don't intend to change it.

@uranusjr
Copy link
Member

uranusjr commented Dec 4, 2020

@jakab922 You have to switch at some point. By not conforming, the wheels you generate are technically invalid. The resolver needs version semantics to resolver, and the best it can do is to omit the warning and skip the wheel entirely, which is likely not what you’d want.

@sztomi
Copy link

sztomi commented Jan 26, 2021

You have to switch at some point. By not conforming, the wheels you generate are technically invalid.

This is true, however, it's a very bad day to wake up to your internal packages that used to resolve just fine to be completely broken with a red herring error message (especially that pip always encourages users to update to the latest version). This is user-hostile behavior and part of the reason why python packaging has such bad rep. Not trying to point fingers and I appreciate striving to conform better with the relevant PEPs. However, I think pip needs to be much more conservative with the breaking changes. There is a huge body of packages with "technically invalid" metadata, not just in private indexes but also on PyPI. From a users point of view, it's strictly better if one can install packages versus being conforming.

@skaughtx0r
Copy link

skaughtx0r commented Feb 2, 2021

FWIW you can include a git hash in the version using local version numbers, so something like 0.3.0+f038176.11.

While this does work with pip, we have some internal packages that also have NPM package counterparts and using + in the version does not seem to work correctly with NPM. We were using a - before because it worked with both package managers. Now I've had to waste a bunch of time fixing build pipelines.

@bigunyak
Copy link

bigunyak commented Apr 6, 2021

Sorry, but I also came to point out that such breaking changes must have a grace period with appropriate warning before you start breaking things for people.
It's quite clear that you didn't think much about those this change could cause problems for. Should have rolled back this change already. Poorly done. 👎

@pradyunsg
Copy link
Member

There was a grace period for this. Quoting myself from earlier in this thread:

This is a part of the changes being made as part of the new dependency resolver. The new dependency resolver is stricter across the board with metadata generated by projects. This behaviour has been a part of pip's new resolver which has been in beta for the past few months, and has had several rounds of publicity and communication telling users to test it since we are making significant changes (such as this one!) to it. As noted multiple times across various communication channels, we have a long list of changes toward being stricter about what metadata pip accepts from packages.

@seancaulfield
Copy link

This is the absolute wrong place for this kind of check. It should be done BEFORE the package is even built, as part of the packaging process.

Currently, there exist tons of failing-to-comply-to-PEP-440 packages already released in the wild (the bulk of them in internal/private repos). Saying they are no longer allowed to be installed because of a version check in the installer puts the burden on the end user/developer trying to make use of the package. As a package user, I can't do anything about a bad version; the package's developer is the only one who can fix it.

Unless it's absolutely necessary for the functioning of the dependency resolution system, this feature does not belong here in pip. It belongs at the start of the packaging process.

@pradyunsg
Copy link
Member

pradyunsg commented Apr 8, 2021

Unless it's absolutely necessary for the functioning of the dependency resolution system, this feature does not belong here in pip.

It is, for things to work with a proper resolver.


The new dependency resolver's rollout (which is what contained this change) has had the most communication that I've seen in any change in Python packaging ecosystem; especially when compared to any of the other changes pip has made in it's lifetime. We actively reached out to publicised this release across many avenues and communication channels. This also included reaching out to PSF sponsors and asking them to test the beta as well.

I want to encourage folks to think about how they could've avoided being blindsided by this change. If you didn't see that there was a major change coming and test before the change happened, do take the time to see if any of pip's communication channels (discuss.python.org, Python-announce, pypi-announce, being a PSF sponsor, etc) would've helped you hear about this change before it happened. Honestly, I'm not sure what more we could have done to make it so that folks adapt for this change in time, and I'm not sure what else we can do differently when making breaking changes.

(no, the answer is not to never make breaking changes; if you want that, please use something else or insulate your pipeline from changes in pip)

@pradyunsg pradyunsg changed the title pip 20.3 fails to parse wheel versions for packages that aren't PEP 440 compliant pip 20.3 rejects wheel versions for packages that aren't PEP 440 compliant Apr 8, 2021
@pradyunsg pradyunsg added type: deprecation Related to deprecation / removal. and removed kind: crash For situations where pip crashes type: bug A confirmed bug or unintended behavior labels Apr 8, 2021
@sztomi
Copy link

sztomi commented Apr 9, 2021

@pradyunsg to offer constructive feedback, all the places you mention are for people actively participating in python or pip development. I suspect that's the minority of your users. The best place for this heads-up would have been in the stdout message where pip encourages users to upgrade.

But I'd also like to echo what was said in the thread before: this is placing a burden on the end user who can't fix the package to be compliant. And I'd also argue that never breaking should not be ruled out. There is a huge body of packages with non-compliant metadata, not only in private registries. Removing the ability to install those makes pip fundamentally less useful.

@brainwane
Copy link
Contributor

@sztomi Hi - heads-up that if you depend on Python packaging you should subscribe to pypi-announce, and if you depend on Python software you should consider subscribing to Python-announce. They are relatively low-traffic lists that are meant for end users.

@z3by

This comment was marked as outdated.

@pradyunsg pradyunsg added C: dependency resolution About choosing which dependencies to install and removed C: new resolver labels Oct 12, 2021
@pradyunsg pradyunsg changed the title pip 20.3 rejects wheel versions for packages that aren't PEP 440 compliant 20.3: rejects wheel versions for packages that aren't PEP 440 compliant Oct 29, 2021
@dcupp
Copy link

dcupp commented Nov 4, 2021

Enforcing PEP 440 even for internal packages is unfortunate. Since PEP 440's scheme was designed for open source packages being published on public servers it naturally does not consider some the use cases we are rely on internally (we tried to fit them into PEP 400 but were unsuccessful). Thankfully --use-deprecated=legacy-resolver works for now.

Martchus added a commit to Martchus/openqa_review that referenced this issue Nov 17, 2021
… tox

As of version 20.3 pip requires PEP440 compliant version strings. Not
generating such a version leads to errors when trying to invoke `tox` in
environments which use a newer pip version.

I've been testing this change on a Git commit which is on a tag and one
that is not on a tag. If it is on a tag the tag is used as version (as
before). If it is not on a tag the most recent tag is used and the
development revision appended, e.g. "1.25.0.dev415".

See
* pypa/pip#9188
* https://progress.opensuse.org/issues/102584
Martchus added a commit to Martchus/openqa_review that referenced this issue Nov 17, 2021
As of version 20.3 pip requires PEP440 compliant version strings. Not
generating such a version leads to errors when trying to invoke `tox` in
environments which use a newer pip version.

I've been testing this change on a Git commit which is on a tag and one
that is not on a tag. If it is on a tag the tag is used as version (as
before). If it is not on a tag the most recent tag is used and the
development revision appended, e.g. "1.25.0.dev415".

See
* pypa/pip#9188
* https://progress.opensuse.org/issues/102584
Martchus added a commit to Martchus/openqa_review that referenced this issue Nov 17, 2021
As of version 20.3 pip requires PEP440 compliant version strings. Not
generating such a version leads to errors when trying to invoke `tox` in
environments which use a newer pip version.

I've been testing this change on a Git commit which is on a tag and one
that is not on a tag. If it is on a tag the tag is used as version (as
before). If it is not on a tag the most recent tag is used and the
development revision appended, e.g. "1.25.0.dev76" (before it would have
been "1.25.0-76-g777610a").

See
* pypa/pip#9188
* https://progress.opensuse.org/issues/102584
Martchus added a commit to Martchus/openqa_review that referenced this issue Nov 17, 2021
As of version 20.3 pip requires PEP440 compliant version strings. Not
generating such a version leads to errors when trying to invoke `tox` in
environments which use a newer pip version.

I've been testing this change on a Git commit which is on a tag and one
that is not on a tag. If it is on a tag the tag is used as version (as
before). If it is not on a tag the most recent tag is used and the
development revision appended, e.g. "1.25.0.dev76" (before it would have
been "1.25.0-76-g777610a").

See
* pypa/pip#9188
* https://progress.opensuse.org/issues/102584
Martchus added a commit to Martchus/openqa_review that referenced this issue Nov 17, 2021
As of version 20.3 pip requires PEP440 compliant version strings. Not
generating such a version leads to errors when trying to invoke `tox` in
environments which use a newer pip version.

I've been testing this change on a Git commit which is on a tag and one
that is not on a tag. If it is on a tag the tag is used as version (as
before). If it is not on a tag the most recent tag is used and the
development revision appended, e.g. "1.25.0.dev76" (before it would have
been "1.25.0-76-g777610a").

See
* pypa/pip#9188
* https://progress.opensuse.org/issues/102584
@pradyunsg pradyunsg changed the title 20.3: rejects wheel versions for packages that aren't PEP 440 compliant Improve error message for wheels with non-compliant versions Mar 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: dependency resolution About choosing which dependencies to install type: deprecation Related to deprecation / removal.
Projects
None yet