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

Make message more user friendly when unable to resolve #8033

Merged
merged 3 commits into from
Apr 15, 2020

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Apr 13, 2020

No description provided.

@pfmoore
Copy link
Member Author

pfmoore commented Apr 13, 2020

This requires the new version of resolvelib, so the tests probably won't pass at the moment.

It's only a first stab at handling the worst case where we write a raw traceback on a simple "cannot find requirement" problem. But I's like to get that included in the 20.1 beta so the user experience with the new resolver isn't too awful.

@pfmoore pfmoore added the skip news Does not need a NEWS file entry (eg: trivial changes) label Apr 13, 2020
@pfmoore
Copy link
Member Author

pfmoore commented Apr 13, 2020

For #7951

("" if parent is None else " (from {})".format(
parent.name
))
)
Copy link
Member

Choose a reason for hiding this comment

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

Does this output something like:

Could not find a version that satisfies the requirement Django>2.0 (...)
Could not find a version that satisfies the requirement Django<=1.11 (...)

This could feel confusing to users :| Maybe we can do:

Could not find a version that satisfies the requirement:
    Django>2.0 (...)
    Django<=1.11 (...)

Copy link
Member Author

Choose a reason for hiding this comment

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

It might, and yes that would probably be better. My initial concern was mainly to not get a traceback for pip install misspelled_thing, preferably keeping the output as near to the current output as we can (we don't have the information in the exception to include "(from versions: xxx)" at the moment).

I'm happy to iterate on this - I guess my main question right now is whether we want to get something into the beta, or if we can live with "proper messages" being something for the post-20.1 improvements?

Also, the question of how closely we want to match the current output ties into this. @ei8fdb was pretty strongly of the view that we should try to get as close as possible, so I was working on trying to replicate current output - and that's a case I haven't been able to replicate (probably because the current resolver's "first come, first served" basis means that it never hits that situation.

Copy link
Member

Choose a reason for hiding this comment

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

I am definitely in favour of having something for the release. This PR as-is would br much better than a cruel ResolutionImpossible.

@pradyunsg pradyunsg added this to the 20.1 milestone Apr 13, 2020
@pradyunsg
Copy link
Member

This likely needs a rebase @pfmoore. :)

@pfmoore
Copy link
Member Author

pfmoore commented Apr 13, 2020

Correct. Thanks for the reminder - I'll fix it in the morning :-)

@pfmoore
Copy link
Member Author

pfmoore commented Apr 14, 2020

I'm merging now. I'm going to butcher in my code after @uranusjr's factory.get_installation_error() call, as I think it needs more work to integrate them well (I'm using logger calls as well as an exception to match the existing behaviour). We'll need to look at this in more depth after the release is out of the way.

@pfmoore
Copy link
Member Author

pfmoore commented Apr 14, 2020

OK, applied an expedient-but-ugly fix. This PR is getting less attractive by the minute, but it does at least have the advantage of not shoving raw tracebacks in the user's face whenever the resolver fails to find a solution.

@pradyunsg I'm not going to merge this myself until I've had your OK as RM that this is acceptable to go into 20.1. But even if it goes in, it will need further iterations to develop a proper approach.

@uranusjr
Copy link
Member

If the implementation feels like butchering, it’s definitely not your fault. The current report mechanism (raise an exception about what failed) is just wrong, and I am secretly planning to refactor that after we got the resolver out 🙂

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Happy to see this go in. :)

Copy link
Member

@pradyunsg pradyunsg left a comment

Choose a reason for hiding this comment

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

Wrong button.

@pfmoore pfmoore merged commit 2f3a1be into pypa:master Apr 15, 2020
@pfmoore pfmoore deleted the messages branch April 16, 2020 08:27
@lock lock bot added the auto-locked Outdated issues that have been locked by automation label May 20, 2020
@lock lock bot locked as resolved and limited conversation to collaborators May 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
auto-locked Outdated issues that have been locked by automation skip news Does not need a NEWS file entry (eg: trivial changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants