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

Fix "direct" check in PipProvider.get_preference #12973

Closed
wants to merge 3 commits into from

Conversation

notatallshaw
Copy link
Member

@notatallshaw notatallshaw commented Sep 21, 2024

I noticed this while running tests from a user provided scenario in #12972. I'm going to have to think about how to add a test, but I'm confident that the direct is broken, possibly this happened when backjumping happened.

When has_information is true, candidate is always a tuple, so testing for candidate is not None is equivalent to testing has_information is true. This is clearly not the intent...

Looking at this tuple, the elements can either be None or an instance of Candidate which happens in both the case of ExplicitRequirement or RequiresPythonRequirement, and I assume this logic is trying to detect when a Candidate is based on ExplicitRequirement, but also being true for RequiresPythonRequirement doesn't functionally matter, as requires_python is tested for in front of direct.

Fixes #12975

@notatallshaw notatallshaw marked this pull request as ready for review September 21, 2024 20:21
@notatallshaw
Copy link
Member Author

notatallshaw commented Sep 21, 2024

It seems there are no unit test cases related to ExplicitRequirement at all 🙁, so instead I'm going to mark ready for review and @uranusjr who I think added direct and @pradyunsg who got pip to support resolvelib's backjumping changes.

This bugfix will have a big impact on resolution preference, because "direct" is right near the top of preference and currently it's actually the same as "has_information", which I think is a bad preference choice, which I think means this should be a substantial improvement.

Happy to put in the work to add any extra stuff, but I need some direction.

@notatallshaw
Copy link
Member Author

notatallshaw commented Sep 22, 2024

As I'm starting to get an proper unit test for get_preference together I'm coming around to two thoughts:

  1. This never worked, before backjumping candidate was always a tuple
  2. While I have an intuitive understanding of "direct", it's not clear to me from the code what this was even trying to test

@notatallshaw
Copy link
Member Author

I started adding unit tests for each preference in get_preference, I have successfully added one for requires_python, but trying to add one for direct made me realize I don't understand what the code is intending to do, I wrote up an issue on this: #12975

@notatallshaw
Copy link
Member Author

Closing this, new PR coming shortly with an actual bunch of unit tests.

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

Successfully merging this pull request may close these issues.

direct preference in resolution doesn't work
1 participant