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

What's the expected behavior of Term.satisfies() when one term has optional deps and the other doesn't? #5469

Closed
3 tasks done
davidxia opened this issue Apr 19, 2022 · 5 comments
Labels
kind/bug Something isn't working as expected

Comments

@davidxia
Copy link

  • I am on the latest Poetry version.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option).
  • OS version and name: macosx 12.3.1 arm64/Apple silicon
  • Poetry version: master commit dca0c56, currently v1.2.0-beta.2.dev0

Issue

I don't think poetry's Term.satisfies() currently supports optional dependencies correctly.

from poetry.mixology.term import Term
from poetry.core.packages.dependency import Dependency

a = Term(Dependency("tfx", ">=1.4.0,<1.5.0"), True)
b = Term(Dependency("tfx[gcp]", ">=1.4.0,<1.5.0"), True)
c = Term(Dependency("tfx", ">=1.3.0,<1.5.0"), True)

a.satisfies(b)
# False

I expect the last statement to return True because tfx is narrower than tfx[gcp] since tfx[gcp] installs more packages than tfx.

  1. Is this the right way to think about it? If so, would you be open to a PR?
  2. If yes to 1, what should b.satisfies(a) return?
  3. Or does it not make sense to compare between Terms unless they are an exact string match?
  4. If yes to 3, perhaps satisfies() should throw an error instead of returning False? If yes, would you be open to a PR?
@davidxia davidxia added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Apr 19, 2022
@radoering
Copy link
Member

Is this the right way to think about it? If so, would you be open to a PR?

I'm not sure but my first thought is: I would not say that tfx satisfies tfx[gcp]. At least, not completely because it does not say anything about gcp. It's more the other way around.

Or does it not make sense to compare between Terms unless they are an exact string match?

Maybe, but raising an error would probably break something because the callers of Term.satisfies() don't expect an error.

I noticed that the comparison in Term.satisfies() has been changed from name to complete_name in #2887. Further, the description of this PR says:

The main trick here is that a package with extras will now have its non-extra version as a dependency to help the resolver figure out what needs to be done.

Considering this information, I would expect that the current behavior should be fine. What's your actual use case? (Maybe, there is another place for a fix.)

@davidxia
Copy link
Author

Thanks for the great info! My use case:

I have two requirements-1.txt and requirements-2.txt. 2's deps' version ranges must be narrower or equal to 1's. I have tfx[gcp] in 1 and tfx in 2 like my example. I'm lazy and want to use poetry's great version comparison utility functions. :)

I'm not sure but my first thought is: I would not say that tfx satisfies tfx[gcp]. At least, not completely because it does not say anything about gcp. It's more the other way around.

Currently b.satisfies(a) also returns False. Are you saying it should return True?

@radoering
Copy link
Member

I don't know. Maybe, it should return True. However, the impact of such a change would need to be investigated. Maybe, callers of the method rely on it returning False if complete_name is not equal.

@davidxia
Copy link
Author

I see. No worries. It seems the backwards incompat are not worth any benefits here. I'm able to workaround this by stripping out the brackets from the package string and creating a new Term with it. Feel free to close this issue and thanks for the help.

@mkniewallner mkniewallner removed the status/triage This issue needs to be triaged label Jun 11, 2022
Copy link

github-actions bot commented Mar 1, 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 1, 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

No branches or pull requests

3 participants