-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Present found conflicts when discarding some criterion #10937
Present found conflicts when discarding some criterion #10937
Conversation
Thanks for doing this. Note that while this is fine as a proof of concept, ultimately the changes to the vendored resolvelib will need to be factored out and submitted as a PR to resolvelib itself (I don't see this as an issue, just noting it for the record in case you weren't aware). |
Thanks @pfmoore! Yep, the changes spanned both |
Thanks for doing this! This works for me in principle. I'd repurpose the Beyond that, I think we probably want to decide how often the warnings are presented and what their verbosity + presentation style should be. Those are minor nitpicky items that we shouldn't spend too much time on (bikeshedding, law of triviality blah blah) so... let's get a resolvelib-side fix in for this! :) |
Thanks @pradyunsg! I just pushed a new commit, repurposing the
Sure thing. I don't have a strong opinion - the current output looks fine for me but I acknowledge that folks might want something better than just "fine" :) This is how it looks for the airflow situation:
|
I didn't imply that we should remove the existing messages that this might take a while; and, yes, it does look like we might want to think about how "noisy" this might end up being. Regardless, I think the obvious next step here is to be make a PR on the resolvelib side; so... Let's do that and then come back to the pip side of the story? |
See discussion at pypa/pip#10937.
Sounds good to me
Done: sarugaku/resolvelib#101 |
See discussion at pypa/pip#10937.
How do we progress this now the resolvelib changes are merged? |
Good question - I think @pradyunsg has some concerns about the verbosity of the output, or at least wanted to discuss a bit. On my side, I think the only missing bit is bringing a new vendorized version of resolvelib after sarugaku/resolvelib#101 was merged. |
You can bring the in-development version of resolvelib in by changing vendor.txt to have And if you wanna pair program on this sometime (in a UK evening): https://calendly.com/pradyunsg/open-calendar :) |
Just remembered about this today because of a comment in another issue. Should I try to push it over the line? |
That would be welcome. I'm still unsure about how to handle the verbosity problem here and don't have any bright ideas about it either. :/ |
FTR, resolvelib 0.9.0 includes sarugaku/resolvelib#101 |
@pradyunsg I think this was superseded by #11783 :) (Not quite) |
c2a4343
to
7c82696
Compare
Okay, with the upstreamed changes from resolvelib, the final diff is very small. I changed to |
This PR still needs a news entry (the details link on the failure for the context + explanation) and it's still marked as a draft. :) Other than that, a unit test would be nice to have for this! |
It'd be just |
Yeah, Sure thing, I'll add a news entry and try putting a test too |
Yup yup, I trust you know that but I don't think an arbitrary reader will, so wanted to explicitly flag that! :) |
@astrojuanlu Could you add a news file entry to this PR and mark it ready for review? |
Fixes pypagh-9254. Closes pypagh-10258. See pypa#10258 (comment) for inspiration.
7c82696
to
0ee6cf9
Compare
Rebased, added news entry, marked ready for review. About the test, I was unsure where to put it. Looks like there are zero tests for |
0ee6cf9
to
1dc37f4
Compare
Test failure seems unrelated?
|
Yea, ignore that failure. :) |
Didn't expect the quick turnaround! So excited to have made my first code contribution to pip. Thanks for the patience 🙏🏽 |
Fixes gh-9254.
Closes gh-10258.
See #10258 (comment) for inspiration.
This is a proof of concept of an alternative approach to gh-10258, hoping that it will (1) provide a better UX when "backtracking" (in quotes, see below) happens, and (2) do it in a way that is acceptable for the pip maintainers. It doesn't have tests or a news entry yet, hoping that I can get maintainer approval & community consensus first.
First of all, I did all of this in a hurry, so forgive me if I'm overloading the terms "requirement", "candidate", "round", "resolve", and such. I only spent 3 hours looking at the pip codebase, and I didn't find high-level docs on "how the new resolver works". By the way, the code comments are great 💯
The tricky thing was summarized, if I understood correctly, in this comment by @pradyunsg:
Indeed, what users call "backtracking" might be two things:
INFO: pip is looking at multiple versions of pyjwt to determine which version is compatible with other requirements. This could take a while.
.In this implementation I'm trying to hook into "the right place" so that conflict information is presented immediately when discarding a certain candidate.
Example of output with direct, impossible resolution example from the documentation:
Example of so-called "backtracking" but-not-quite, when pip is reaching out to older versions of an unpinned dependency to try to satisfy the constraints (and which doesn't produce a "this will take a while" message):
Example with super long backtracking as officially defined in the pip codebase (full output, too long for GitHub):