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

Simplify, fix, and add unit tests for PipProvider.get_preference #12982

Closed
wants to merge 7 commits into from

Conversation

notatallshaw
Copy link
Member

@notatallshaw notatallshaw commented Sep 28, 2024

Fixes: #12975

This PR does the following:

Side note: I think I've found a bug in the way parent_depths is calculated when a requirement with an extra is involved, but it was too involved to fix in this PR, once I've pinned it down I will make a follow up PR.

@notatallshaw
Copy link
Member Author

CI is green, ready for review / merge

@notatallshaw notatallshaw added this to the 24.3 milestone Oct 10, 2024
@notatallshaw
Copy link
Member Author

Tentatively putting this on the 24.3 milestone since it's a small bug fix, a small refactor, and finally unit tests get_preference! Feel free to remove.

@sbidoul
Copy link
Member

sbidoul commented Oct 13, 2024

For this one, we'd need a review by a committer who is more familiar than I am with the innards of the resolver.

@notatallshaw
Copy link
Member Author

I ran all scenarios against: https://github.com/notatallshaw/Pip-Resolution-Scenarios-and-Benchmarks

There is no regression or advantage between this PR and pip 24.2, which is what I expected (there was a small chance removing the broken direct preference would change something, but at at least in all these scenarios it didn't).

* If equal, prefer if any requirement is "pinned", i.e. contains
operator ``===`` or ``==``.
* If a requirement is part of the current cause for backtracking.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sentence seems to be incomplete?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've update to match the style of the surrounding sentances.

operator ``===`` or ``==``.
* If the a member of backtrack causes.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this sentence correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I've updated this point to match what I wrote in the docs

@sbidoul sbidoul modified the milestones: 24.3, 25.0 Oct 26, 2024
@notatallshaw
Copy link
Member Author

notatallshaw commented Nov 10, 2024

Going to make a single follow up PR once #13001 lands, I'll comment here once done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

direct preference in resolution doesn't work
2 participants