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

Ignore period for version parse #4176

Closed
aviramha opened this issue Jun 13, 2021 · 23 comments · Fixed by python-poetry/poetry-core#186
Closed

Ignore period for version parse #4176

aviramha opened this issue Jun 13, 2021 · 23 comments · Fixed by python-poetry/poetry-core#186

Comments

@aviramha
Copy link
Contributor

Following this issue: #4095
I think for me and many other enthusiastic poetry users is a major blocker for upgrading. I do understand and support validations and standardization but it's going to take a while to get all packages to fit the standards.
Will a PR ignoring such errors based on a flag (maybe by default to begin with) be accepted?

@aviramha aviramha mentioned this issue Jun 13, 2021
3 tasks
@finswimmer
Copy link
Member

Hello @aviramha,

I totally understand that it is frustrating for end user if they cannot install a package due to malformed metadata.

Im my opinion it is not the correct solution to include rules which ignores mistakes made by package maintainers. This would add an extra layer of code complexity, just ignoring things can lead to wrong dependency resolutions and if we start to make an exception for one rule it doesn't take long, we will be asked to add more. Forcing only parts of standardized rules do not lead to more packages that follow these rules.

I hope I could get some agreements to my points :)

fin swimmer

@aviramha
Copy link
Contributor Author

Thanks for your response.
I think obviously the long term solution is the packages fixing the metadata.
But - introducing "breaking changes" (even though it was always "broken") in a minor version without warning/deprecation period just leads to extreme frustration.
Imagine the average poetry user who doesn't dig in the GitHub issues - assumes that poetry is just broken forcing it to use alternatives..

@jborean93
Copy link

Just coming from #4201. I don't have strong feelings either way but the wildcard suffix on ordered comparisons may not be valid with PEP 440 but they do work with pip today. It treats something like >=3.7.* as the same as >=3.7 from what I can gather.

For example gssapi has python_requires='>=3.6.*' and when I run pip install gssapi -v on Python 2.7 it will not use those versions when selecting. The verbose output even shows that it doesn't match whereas running on Python 3.6 will match:

  Link requires a different Python (2.7.18 not in: u'>=3.6.*'): https://files.pythonhosted.org/packages/ee/1a/d67711cd055ccf2736cbce2da0c55bd359c8e7a6ba60195e1d626bf086cb/gssapi-1.6.13.tar.gz#sha256=5947b93d8188baab88871c93770a8db394b0e522d4f9
2d5d4f5c9edb48ac0f9f (from https://pypi.org/simple/gssapi/) (requires-python:>=3.6.*)

I'm also reading through https://www.python.org/dev/peps/pep-0440/#public-version-identifiers and while it doesn't explicitly say that .* can be used with the ordered comparisons like it does with version-matching it doesn't explicitly say it cannot. The version matching section even encourages it's use:

The use of == (without at least the wildcard suffix) when defining dependencies for published distributions is strongly discouraged as it greatly complicates the deployment of security fixes

I'm sure it is invalid according to PEP 440 but I believe it's ambiguous enough that I feel it may be used a bit in the wild.

@aviramha
Copy link
Contributor Author

Hey @finswimmer , sorry for being so pushy but I really like to upgrade my poetry and can't until aiopika will release a new version (which will be a major one..)..
I'd appreciate your response.

@aviramha
Copy link
Contributor Author

@abn @finswimmer Any feedback?

@xkortex
Copy link

xkortex commented Jul 15, 2021

Just ran into a bug which I think is related. I cannot use plugins in conjunction with uvicorn. Steps to reproduce:

docker run -it python:3.9.6 bash  # or some other fresh env
pip install poetry
pip install poetry-version-plugin  # install a plugin
poetry plugin show
# works
pip install 'uvicorn==0.14.0'  # install offending package
poetry plugin show 
#   InvalidVersion
#   Invalid PEP 440 version: '7.'
#   ValueError
#  Could not parse version constraint: >=7.*
grep -Pr   /usr/local/lib/python3.9/site-packages/ -e '>=7\.\*' # find the offending line
# /usr/local/lib/python3.9/site-packages/uvicorn-0.14.0.dist-info/METADATA:Requires-Dist: click (>=7.*)

The offending file:

Metadata-Version: 2.1
Name: uvicorn
Version: 0.14.0
...
Description-Content-Type: text/markdown
Requires-Dist: asgiref (>=3.3.4)
Requires-Dist: click (>=7.*)
Requires-Dist: h11 (>=0.8)

This is definitely the offending line, as if I change it to Requires-Dist: click (>=7.0), the problem goes away.

Looks like someone else already raised the issue with uvicorn, but frankly I think the issue is bigger than this.

I totally understand that it is frustrating for end user if they cannot install a package due to malformed metadata.

I think this issue is bigger than just frustration that being able to install a particular package. IMHO, this is a violation of the Robustness Principle. Installing two unrelated packages shouldn't brick poetry plugin show. Irrespective of how malformed any of the installed metadata files are, poetry plugin show should still report the expected output:


  • poetry-version-plugin (0.1.3) Poetry plugin for dynamically extracting the package version from a __version__ variable or a Git tag.
      1 plugin

      Dependencies
        - poetry (>=1.2.0a1,<2.0.0)

@xkortex
Copy link

xkortex commented Jul 15, 2021

Here's the critical region:

if with_dependencies:
for require in distribution.metadata.get_all("requires-dist", []):
dep = Dependency.create_from_pep_508(require)
package.add_dependency(dep)

specifically this line fails when the metadata loaded from distribution._path is bad:

dep = Dependency.create_from_pep_508(require)

I'd take a shot at fixing it, but I'm not sure what the implications of skipping an entire require or dep would be. I'm also not sure how I would put a warning in there. Personally, what I would do is catch the error, warn the user that Package {name} has got bad metadata and the dependency cannot be solved, and skip package.add_dependency(dep) for just that. In this case, this would result in uvicorn looking like it doesn't have a dependency on click.

This might be a helpful stopgap:

                if with_dependencies:
                    for require in distribution.metadata.get_all("requires-dist", []):
                        try:
                            dep = Dependency.create_from_pep_508(require)
                            package.add_dependency(dep)
                        except ValueError as exc:
                            raise ValueError(f"Package '{name}' has malformed metadata; cannot process dependencies from {path}") from exc

At least it makes clear why it's breaking. I had to inject pdb traces to figure out exactly what package was breaking, since the create_from_pep_508 exception has no awareness of the scope/require from whence it was dispatched.

@xkortex
Copy link

xkortex commented Jul 16, 2021

There's also something fruity about installed_repository when there are packages installed from URL, e.g. pip install git+https://github.com/encode/uvicorn.git; poetry plugin show.

Again, poetry barfs with no indication what the offending package or file is.

Also, this isn't even PEP610 compliant:

source_reference = url_reference["vcs_info"]["requested_revision"]

A requested_revision key (type string) MAY be present naming a branch/tag/ref/commit/revision/etc (in a format compatible with the VCS) to install.

https://www.python.org/dev/peps/pep-0610/#:~:text=a%20requested_revision%20key%20(type%20string)%20may%20be%20present%20naming%20a%20branch%2Ftag%2Fref%2Fcommit%2Frevision%2Fetc%20(in%20a%20format%20compatible%20with%20the%20vcs)%20to%20install.

Additionally, even if that string is "not compliant", pip seems to handle it gracefully, which means that 7.* is a de-facto standard of sorts, and poetry ought to handle it as long as pip can.
Requirement already satisfied: click==7.* in /Users/mike/.virtualenvs/poetry395/lib/python3.9/site-packages (from uvicorn==0.13) (7.1.2)

Traceback when direct_url.json lacks "requested_version".

  KeyError

  'requested_revision'

  at ~/src/python-poetry/poetry/poetry/repositories/installed_repository.py:206 in create_package_from_pep610
      202│         elif "vcs_info" in url_reference:
      203│             # VCS distribution
      204│             source_type = url_reference["vcs_info"]["vcs"]
      205│             source_url = url_reference["url"]
    → 206│             source_reference = url_reference["vcs_info"]["requested_revision"]
      207│             source_resolved_reference = url_reference["vcs_info"]["commit_id"]
      208│
      209│         package = Package(
      210│             distribution.metadata["name"],
(poetry395) ➜  encode poetry plugin show -vvv

  Stack trace:

...
   1  ~/src/python-poetry/poetry/poetry/repositories/installed_repository.py:116 in create_package_from_distribution
       114│             and path.joinpath("direct_url.json").exists()
       115│         ):
     → 116│             return cls.create_package_from_pep610(distribution)
       117│
       118│         is_standard_package = env.is_path_relative_to_lib(path)

  KeyError

  'requested_revision'

  at ~/src/python-poetry/poetry/poetry/repositories/installed_repository.py:206 in create_package_from_pep610
      202│         elif "vcs_info" in url_reference:
      203│             # VCS distribution
      204│             source_type = url_reference["vcs_info"]["vcs"]
      205│             source_url = url_reference["url"]
    → 206│             source_reference = url_reference["vcs_info"]["requested_revision"]
      207│             source_resolved_reference = url_reference["vcs_info"]["commit_id"]
      208│
      209│         package = Package(
      210│             distribution.metadata["name"], 

@chrisegner
Copy link

chrisegner commented Jul 29, 2021

FWIW, I am unable to install nltk, which is a fairly significant package. I've submitted a bug report with nltk.

Other than reverting to an earlier version, if there any escape hatch for working around this? It's a bit rough to find out you have to switch out package managers because someone upstream commits invalid metadata.

@bcm0
Copy link

bcm0 commented Jul 30, 2021

Is it a 'feature' to make software fragile to user mistakes?

Don't ask every single package maintainer to add proper metadata because mistakes happen all the time.
Instead poetry should handle errors gracefully and output a warning that dependency resolution can break due to wrong metadata.
If you want to keep this 'feature', please add a descriptive error message.

Python packaging should be easier than that.

@albireox
Copy link

albireox commented Aug 8, 2021

Any news on this? This is still happening on 1.2.0a2 which means that Poetry is getting closer to becoming useless to a lot of us.

The problem with this issue is that punishes the user for a mistake made by an upstream maintainer. That's something that the user has very limited control about. For example, this happened with aiormq and aio-pika for which I opened PRs that were accepted, but more than two months have passed and those developers have not published a new release.

I generally agree with Poetry taking a hard stance on incompatibilities between versions, but this should raise a warning and not an error.

@aviramha
Copy link
Contributor Author

aviramha commented Aug 8, 2021

Any news on this? This is still happening on 1.2.0a2 which means that Poetry is getting closer to becoming useless to a lot of us.

The problem with this issue is that punishes the user for a mistake made by an upstream maintainer. That's something that the user has very limited control about. For example, this happened with aiormq and aio-pika for which I opened PRs that were accepted, but more than two months have passed and those developers have not published a new release.

I generally agree with Poetry taking a hard stance on incompatibilities between versions, but this should raise a warning and not an error.

We face the same and I think we'll soon migrate to pipenv..

@finswimmer
Copy link
Member

@aviramha
Copy link
Contributor Author

aviramha commented Aug 8, 2021

I've started a discussion here: https://discuss.python.org/t/pep-440-wildcard-usage-in-comparison-clause/10045/1

I'm not sure if this discussion is relevant as none here denies the fact that this validation is correct rather we think it's too strict to push in without any way to bypass it.

@finswimmer
Copy link
Member

This discussion is highly relevant.

First, we can make sure that we Interpret the PEP correct. If we do not, we have to fix the behavior.

Second, if we do interpret the PEP correct, the question is why pip doesn't and if there is some action requires by them.

As it stands now, poetry is doing right and pip is doing wrong. And that's a problem, because there are packages out there with wrong metadata, which could not be installed by pip as well if they fix it.

There is a good chance that the PEP will be changed in a way, that using wildcards is discouraged but possible. If so, poetry can adopt its behavior by beeing still PEP 440 compliant.

If poetry decided to implent a PEP it should fully support it, without any bypass. Otherwise a PEP is quite useless.

fin swimmer

@aviramha
Copy link
Contributor Author

aviramha commented Aug 8, 2021

if we lack clarity, why adhere to that vague policy and cause issues to many users until everything is clear?

@bentheiii
Copy link

bentheiii commented Aug 8, 2021

Just my two cents: Regardless of the reason, or who is at fault, I cannot recommend anyone use a package manager that rejects dependencies because of what might as well be bad grammar. As useful as PEPs are, standard for open-source ecosystems are ultimately set by the community. Whatever the community uses as the standard-that is the standard, and package managers should support it.

A package manager that doesn't strive to support anything that pip does is a no-go for new projects. Doubly so for one that's willing to make dependencies unusable over a minor change, without so much as a deprecation warning.

@finswimmer
Copy link
Member

@aviramha: poetry 1.2, which contains the changes, is still in an alpha phase. It's the best time to discuss new features and behavior based on user feedback. But switching back and forth would be to much work. The topic doesn't touch only poetry as it looks now. So there is a need for a broad er discussion.

@bentheiii: Whenever you have rules, there are three ways to handle them:

  1. Ignore them
  2. Follow them
  3. Change them

If to many people ignore them a rule is useless and you can mark them obsolete.

Rules in software development are necessary so that different parts can interact with eachother. So any program should follow rules that are relevant for it.

Rules are never made to live forever. New knowledge or circumstance can make it neccassery to change rules. It's important to recognize those moments and starting a change. It looks like we are at that point now for PEP 440.

@albireox
Copy link

albireox commented Aug 8, 2021

@finswimmer I don't disagree with any of that, but it seems to me that if the Poetry developers want to introduce this breaking change that will make it unusable for many users, and in a minor version, they should be leading the charge to either change the PEP or modify pip's behaviour. And knowing pip, even if they accept the change it will take many months, even years, with deprecation warnings, to implement. By which time Poetry 1.2.0 will be out and many of us will have to modify all of our projects to use a different package manager.

This issue and related have now been opened for many months and the only answer we've got is to talk to the package maintainers. In the meantime, 1.2.0 keeps approaching without any compromise on this topic.

If this really is that critical, it should be added to the list for Poetry 2, with plenty of time to engage the slow-moving wheels of Python and pip.

@layday
Copy link

layday commented Aug 13, 2021

If you'd like some idea of the impact dropping support for .* in less than/greater than comparisons will have, see pypa/packaging#321 (comment). About 0.1% of all dependency specifiers on PyPI around this time last year were not PEP 440-compliant, but that's not to say that all of them were using the asterisk notation. Perhaps there's someone in PyPA you can talk to to get a dump of the SQL data.

@aviramha
Copy link
Contributor Author

Thanks @sdispater !!

@martinxsliu
Copy link

martinxsliu commented Oct 29, 2021

Thanks for the change @sdispater! Do you have an estimate for when we could see a 1.2.0a3 release that would include this?

I just ran into this issue when attempting to install a plugin because one of Poetry's own dependencies, cachecontrol, made a release 0.12.7 with a wildcard suffix. Ironically, I created a simple plugin poetry-pep440-plugin to workaround this exact issue until 1.2.0a3 is released, but I can't even install that at the moment :(

Edit: cachecontrol released 0.12.8 which fixed its wildcard spec.

Copy link

github-actions bot commented Mar 2, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants