-
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 trying to resolve them #10258
Conversation
c24e359
to
a9ef2de
Compare
functools.partial( | ||
self.factory.get_backtracking_reason_message, | ||
constraints=collected.constraints, | ||
) |
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.
this feels somewhat ugly to me, but passing the constraints and factory to the reporter felt weird as well.
would love to get your decision on that.
Hey, sorry for the delay, just want to say I still remember this and will eventually come back to review this (and the accompanying resolvelib PR). But I need to prioritise fixing 21.2.x over this (which targets 21.3 and beyond). |
sure, np. :) |
Would it make sense to display this message every time the causes change during the backtracking? Not just when backtracking first starts. E.g.
Or am I missing something and this is way beyond the scope of the PR? |
@notatallshaw do you have an example of where you did not get what you expected? |
Apologies, I think I misread the description of the PR and title. I'll try actually running the PR against a few examples I've built up on #10201 where it's not clear to the user why backtracking is happening. |
Thanks for your patience, I was trying to test this with the following real world use case ad I couldn't get it to trigger:
Am I missing something? |
Hey, sorry for the long time.
|
Great! I must of tested it wrong somehow. I love this change, even with the performance improvements I'm hoping my PRs will bring to backtracking it will still be very useful to users to know why backtracking started. |
d19c4f8
to
5047743
Compare
@@ -47,6 +47,8 @@ | |||
from pip._internal.utils.hashes import Hashes | |||
from pip._internal.utils.packaging import get_requirement | |||
from pip._internal.utils.virtualenv import running_under_virtualenv | |||
from pip._vendor.resolvelib.resolvers import RequirementInformation | |||
from pip._vendor.resolvelib.structs import RT, CT |
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.
Since they are from the .pyi
file, these are not available at runtime and must be hidden inside if TYPE_CHECKING
.
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.
honestly I just added those so mypy won't fail... 😬
should I use them with the if TYPE_CHECKING
or should I use a more generic type?
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.
RT
and CT
are needed in resolvelib because it treats requirement and candidate classes are opaque types; those have concrete implementations in pip though, so you can just do
RequirementInformation[Requirement, Candidate]
but you still need to do some if TYPE_CHECKING
dance since RequirementInformation
is only a generic at type-checking level, not at runtime (another restriction with pyi files). I think there's an existing usage for that you can copy.
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.
oh, right. I get it now.
resolvelib is completly unware of the types, but in pip we have concrete types.
thanks, that makes total sense.
and i think i found an example in the provider:
PreferenceInformation = RequirementInformation[Requirement, Candidate] |
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.
Yup, I think that's the one.
from running this a bit in the wild i see that |
Hard to tell, we probably need to sample some real examples to see which presentation works best from the user's perspective. |
I think i'll start with having this just once, and we can decide later to change this to some other logic. |
news/10210.feature.rst
Outdated
@@ -0,0 +1 @@ | |||
found conflicts are output before pip delves into version resolution. |
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.
found conflicts are output before pip delves into version resolution. | |
Display found conflicts are before pip delves into version resolution. |
(Not sure if I understand this message correctly. The point is to turn this into a complete sentence so the changelog reads better.)
@uranusjr I still want to change this to a single message, so let's wait and not merge yet. Anyhow, thanks for your reviews and patience for a first-time contributor! :) |
If you’re looking for an interesting test case, try |
3266df7
to
7ad1da1
Compare
Hi @uranusjr, |
@pradyunsg are you able to bring this forward with this information? I think there is consensus that "backtracking" needs to be noisy and that a summary at the end is useful. It also seems useful to put a hard limit on the number of versions to try – perhaps expressed as a total number of packages downloaded (regardless of whether a package was cached or not). |
@malthe while there may be consensus on that, it's only really in the case where resolution fails (I've not re-read the threads to be sure, and personally I find that too much output in the "everything worked" case is annoying). I don't think there's been much consideration of how much of a problem the additional verbosity would be in simpler cases where everything works as expected. I don't have any good answers - UX is hard - but the tricky part of discussions like this is that the participants are, of necessity, self-selected to be people hitting the problem cases where extra information is self-evidently useful... This makes it hard to get a good picture of the actual impact. |
@pfmoore but is this really what a user wants – to download lots of software just to realize that versions are incompatible? I think there should be some noise in the output to let the user know that they're using software that isn't constrained properly. |
As I said, if resolution is impossible then yes, knowing what's going on in that case is likely to be good. But users definitely want pip to try different versions in order to find a working installation (that's exactly why we implemented the new resolver). And if it manages to, then they probably want that to "just work" without pages of irrelevant (to them) messages. The problem is that we can't tell in advance whether the resolution will work or fail, so we can't decide up front whether to be quiet or noisy. |
The soon as backtracking starts going more than a few extra downloads, my users want to debug the conflict asap and have no tools to do that without waiting a long and indeterminate amount of time. On CI, a bad case can potentially exceed the running time allotment of the CI runner so no output is provided. I don't think users regard more than a small hiccup as anything other than a bug in installation. |
@pfmoore seems to me that the correct solution to this problem is to allow pip to use an external service |
I don't agree that there's consensus in presenting a summary at the end. At least in my case I was advocating for presenting the conflicts right when the backtracking starts, see my earlier comment. |
That seems to me like a much bigger change than the simple "report on backtracking" that's being discussed here. If you think that's worthwhile, feel free to try to develop such a thing and propose a PR, but I'm skeptical that's likely to be practical (speaking as one of the people who developed the current resolver, and hence has a feel for the scale of such a job). |
I perfectly understand that we cannot decide upfront, and yeah - it is an extremely complex problem to solve, so I also sympathise with But if the algorithm cannot decide, let the user decide when they want to stop and how much of backtracking to do and when "too long" or "too much" of the backtracking to do. I am fine with either "time based" limit or "iteration based" or any other limit that will be "reasonable" from implementation point of view. But simply letting it running for an indefinite amount of time when things go wrong is the "worst" UI choice from the user perspective. Because the user cannot do anything about it when it happens. While I understand that it's extremely "hard" to decide when "long" is "too long" (this is the problem This is what I propose - give the users (those who need it - should be optional) the ability to make their own decision when "long" is "too long" for their particular case, and print diagnostics information if this limit is hit. I think not all decisions have to be made by the algorithm and |
The users who know what they're doing can already do whatever they want: pass There's two avenues to improve this from, IMO, giving users a knob for "stop backtracking after X attempts" and "present more information when we start backtracking". This is purely intended for the latter, and if someone wants to ask for the former, that's a separate discussion. I'm gonna bow out from this one, since I think my energy is better spent elsewhere. :) @nadavwe If you update this PR, please @-mention me! :) |
Just to clarify a bit - because it was maybe a misunderstanding. I do want to tap into the powers of
Surely if you say so. Happy to open it. |
I submitted a new issue about timeout separating it out from this one #10932 |
@astrojuanlu oh I meant both: immediate feedback and a summary at the end – and this should also be generated if the program is interrupted. |
@pfmoore why wouldn't a resolver service be practical given that we're asking all clients to do exactly that? This is a digression of course, but the point is to clarify that the current practice is not an optimal solution. It's basically the gentoo of package distribution – everyone compiles their own. Perhaps the analogy doesn't hold all the way, but if you've used a binary distribution, you appreciate the difference in installation time. I think there is a scope now to this which is to "present found conflicts when trying to resolve them". Then we've got a timeout spinoff in #10932 and one can imagine some toggles to control the behavior of the backtracking system in a future issue. |
Maybe it would. If you want to write a PR that demonstrates how it would work, I'd be happy to be proved wrong. I just don't personally know how you'd do something like that, in a way that would be any better than the current resolver. But let's not endlessly debate this - I'll keep an open mind until someone produces a PR. |
@pradyunsg okay but with regards to this PR and not the extended discussion do you have any blocking requirements before it's merged? As discussed capturing Would you accept the compromise I proposed in #10258 (comment) ? |
@pfmoore now that I'm properly reading up on this, I can see how there are certain details ... that make the problem quite hard: https://dustingram.com/articles/2018/03/05/why-pypi-doesnt-know-dependencies/. But of course with today's cloud tech, not exactly impossible. |
Fixes pypagh-9254. Closes pypagh-10258. See pypa#10258 (comment) for inspiration.
I've been completely nerd-sniped and I rage-implemented an alternative in gh-10937, hopefully encoding @pradyunsg ideas from his first rejection to this attempt. I will complete the description later, I have to... get back to what I was supposed to do. Edit: I now wrote a proper summary. |
Okay, looks like gh-10937 was deemed a reasonable approach to tackle this issue. I think the output now is much more informative, as it captures the conflicts right as they appear, and then the user can (a)
Folks are welcome to try it out and see if it helps debugging backtracking situations. |
This looks fantastic. I am a bit swamped now but I can try to reproduce some of the recent cases we had with this version and see if this will aid in siimilar problem resolution (but from the quick look it should be extremely helpful). |
Fixes pypagh-9254. Closes pypagh-10258. See pypa#10258 (comment) for inspiration.
Fixes pypagh-9254. Closes pypagh-10258. See pypa#10258 (comment) for inspiration.
Resolves #9254