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

Poetry does not properly resolve install-requires for certain GIT packages #8774

Closed
3 of 4 tasks
romain-intel opened this issue Dec 10, 2023 · 10 comments · Fixed by #9000
Closed
3 of 4 tasks

Poetry does not properly resolve install-requires for certain GIT packages #8774

romain-intel opened this issue Dec 10, 2023 · 10 comments · Fixed by #9000
Labels
kind/bug Something isn't working as expected

Comments

@romain-intel
Copy link

romain-intel commented Dec 10, 2023

  • Poetry version: 1.7.1
  • Python version: 3.8.18
  • OS version and name: Ubuntu
  • pyproject.toml:
[tool.poetry]
name = "poetry-demo"
version = "0.1.0"
description = ""
authors = ["Romain <romain@email.com>"]
readme = "README.md"

[tool.poetry.dependencies]
python = "^3.8"
clip = {git = "https://github.com/openai/CLIP.git"}


[build-system]
requires = ["poetry-core"]
build-backend = "poetry.core.masonry.api"
  • I am on the latest stable Poetry version, installed using a recommended method.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • I have consulted the FAQ and blog for any relevant entries or release notes.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option) and have included the output below.

Issue

The issue is that when determining the install-requires dependency, it analyzes the setup.py file and notices that there is a install-requires but cannot parse it (it's a list comprehension and not a list). The end result is that this is then silently ignored here: https://github.com/python-poetry/poetry/blob/master/src/poetry/inspection/info.py#L565 because it doesn't check if install-requires is filled (and in fairness, shouldn't since some packages don't have that).

The crux of the issue is basically that in the SetupReader, there is no distinction between "value absent" and "value can't be read".

The proper behavior is probably to fallback to building the package if allowed in the case when the field exists but can't be extracted.

Steps to reproduce:

poetry new poetry-demo
cd poetry-demo
poetry add "git+https://github.com/openai/CLIP.git"

If you look at the CLIP project, you will see it has some requirements like torch but the poetry lock file lists no such thing and looks like this:

[[package]]
name = "clip"
version = "1.0"
description = ""
optional = false
python-versions = "*"
files = []
develop = false

[package.extras]
dev = ["pytest"]

[package.source]
type = "git"
url = "https://github.com/openai/CLIP.git"
reference = "HEAD"
resolved_reference = "a1d071733d7111c9c014f024669f959182114e33"

[metadata]
lock-version = "2.0"
python-versions = "^3.8"
content-hash = "ca6e63d2418191bea92517a4e1a4b2427c67dbcfe40147d16556cd0c674f34c3"
@romain-intel romain-intel added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Dec 10, 2023
@radoering
Copy link
Member

Transferring from #4325 (comment):

Here, we only use the information from setup.py if it is "complete" (otherwise we do a build):

with contextlib.suppress(PackageInfoError):
info = PackageInfo.from_setup_files(path)
if all([info.version, info.name, info.requires_dist]):
return info

So the question is why don't we try a build even though we were not able to parse install_requires? That's because we were able to parse extra_require and both are merged to requires_dist. Thus, we could improve the heuristics.

Nevertheless, it might make more sense to convince projects to specifiy their dependencies in a more static way when using setuptools.

@meln5674
Copy link

Would it be reasonable to add a field to the git dependency specification that indicates "hey, this repo does some funky stuff, skip the heuristics, just build it and get the dependencies and other metadata from the output, I'm okay with that being slower" ?

While I agree that static configuration is "the right way (tm)", at least from my experience, a large chunk of the ecosystem hasn't, and has no interest in switching. The pragmatic approach would be to have some sort of "escape hatch" like this. Otherwise, poetry is basically unusable if one has to fork such a project.

@dimbleby
Copy link
Contributor

Unnecessary complexity. If this is something you're interested in, it would be just as simple to fix it.

Somewhere round about here, poetry is just ignoring things that it doesn't understand.

Instead have it signal that it can't understand them: by returning None or throwing an exception, or whatever.

@romain-intel
Copy link
Author

I will readily admit that I am a total novice to poetry's code base but in general I feel that having heuristics that "produce the wrong result" is probably not a great behavior. I agree with @dimbleby that at a minimum, a failure to parse should raise an exception (it would at least inform the user that poetry cannot be used safely/correctly with that repository). My personal preference though would be:

  • use heuristics to see if you can skip building
  • if heuristics fail (it should be detectable) fallback to building the package

It does seem that right now, in some cases, the package is built just not in all failure modes. I think this is what @radoering is saying -- ie: the heuristic is failing to detect that it couldn't parse the information properly. The behavior of "fallback to build" is the right one imho, but failure cases need to be more properly detected.

@dimbleby
Copy link
Contributor

The code already falls back to building, if reading setup.py fails. It should be straightforward to arrange that the fix I suggest has the desired behaviour.

But this is open source: someone who wants this has to do the work!

@meln5674
Copy link

To be frank, relying on heuristics while demanding others rework their project to use static configuration for reliability and repeatability seems at least a little hypocritical, especially when the justification for both is "it works now, why change?". Heuristics are great up until they don't do what you want or do what you don't want. Being able to statically configure what you expect from poetry, and have it fail if it can't follow through seems like the best choice. The problem with heuristics is that there will always be edge cases that you can't predict, and there is always the possibility that a user might want to force a build for reasons that cannot be conceived of right now. If it failed here, even with this fix, what gives you confidence it won't fail in other ways? Simplicity is great, unless you're simply wrong.

@dimbleby
Copy link
Contributor

This isn't a question of heuristics, it's a straight bug. Poetry is trying to read setup.py, thinks it knows what it is doing, and it gets it wrong.

If this is important to you, please submit a pull request.

dimbleby added a commit to dimbleby/poetry that referenced this issue Feb 21, 2024
if encountering something we don't understand, best assume that we can't
get the right answer

fixes python-poetry#8774
dimbleby added a commit to dimbleby/poetry that referenced this issue Feb 21, 2024
if encountering something we don't understand, best assume that we can't
get the right answer

fixes python-poetry#8774
@meln5674
Copy link

This remains an issue in 1.8.1 . Add kubernetes = {git = "https://github.com/kubernetes-client/python"} as a dependency to reproduce. Let me know if you'd prefer a new issue.

@abn
Copy link
Member

abn commented Feb 28, 2024

@meln5674 please raise a new issue. Thank you.

@abn abn removed the status/triage This issue needs to be triaged label Mar 2, 2024
Copy link

github-actions bot commented Apr 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 Apr 2, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants