-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
resolvelib: emit Requires-Python dependency first #13270
Conversation
This makes the resolver always inspect Requires-Python first when checking a candidate's consistency, ensuring that no other candidates are prepared if the Requires-Python check fails. This regression was masked due to a broken test which checked for the (nonpresence of the) wrong package name. --- The resolvelib provider was also updated to return dependencies lazily. While ideally we wouldn't prepare candidates unnecessarily, pip has grown numerous metadata checks (for reporting bad metadata, skipping candidates with unsupported legacy metadata, etc.) so it's infeasible to stop preparing candidates upon creation (without a serious architectural redesign). However, we can create the candidates one-by-one as they're processed instead of all dependencies at once. This is necessary so the resolver can process Requires-Python first without processing other dependencies. Co-authored-by: Tzu-ping Chung <[email protected]>
@notatallshaw Given the actual resolvelib changes are still the same from #13160, I'm going to consider your review still valid... however, I'm curious to whether PR #13253 will reduce the small overhead introduced here as you explained earlier. |
While I'm here, I'm curious: do people read the commit messages when reviewing a PR? I strive to provide additional context and generally write good, informative commit messages, but I'm wondering if they're too hidden. I usually read the commit messages, but I tend to be a slow reviewer anyway. |
Personally, I tend to just review the final "all commits" diff. Going through commit by commit doesn't really work for me, I find it too easy to lose context while reviewing. That's probably more a comment on my review process than on the value of having well structured and described commits though 🙄 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Yes, the cost of prefering Requires-Python is now the lowest of any requirement, it will in general immediately short-circuit the preference calculation, ignoring all other options. |
You're going to need to ELI5 this :) What does it mean to prefer a "candidate"? If a candidate is preferred, is it the first to be attempted while pinning a package? |
The terminology is overloaded here, I don't think I said "candidate" ? I'm going to try my best to clarify but be aware I'm not in front of my computer right now: For each resolution round there are a choice of requirements, the provider must tell the resolver which one is preferred, via the Prior to resolvelib 1.1 for each round |
Ah, that's my bad. I definitely pulled the "candidate" term out of thin air. I don't think I understand why |
Thanks @pfmoore and @notatallshaw for reviewing! |
A candidate is a specific project/version item ("foo 1.0", for example), whereas a requirement is a specification of what is valid ("foo >= 1.0"). The resolution algorithm is, at the most basic level, a process of picking a requirement from the set that still need resolving, and "pinning" it by choosing a specific candidate. That proceeds until a resolution is found, or we hit a dead end when we backtrack, unpinning things until we can try a different solution. I don't fully remember how I think the point of @notatallshaw's comment is that now, if we can see a |
Because of this line: https://github.com/sarugaku/resolvelib/blob/1.1.0/src/resolvelib/resolvers/resolution.py#L440 A resolver, in general, needs to know which unsatisfied requirement to try next, The new |
Ah, I thought That makes much more sense, indeed 🙂 |
Thanks @notatallshaw - the point I'd forgotten was that |
Revived version of #11398. This is a rewrite of #13160, focused on minimizing the diff to the strictly necessary changes.
Closes #11398.
Fixes #13146.
Fixes #11142.
@pfmoore PTAL. I believe I have addressed all of your concerns (namely the confusion with the tests and adding a comment about why the change to a generator is important).