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

Improve the message for "Resolution Conflict" errors #8394

Merged
merged 6 commits into from Jun 18, 2020

Conversation

pfmoore
Copy link
Member

@pfmoore pfmoore commented Jun 4, 2020

Fixes #8377

@nlhkabu @ei8fdb comments appreciated 🙂

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

pfmoore commented Jun 4, 2020

Here's a nasty edge case that the current code does not handle at all well (based on the issue in #8131):

If we have a local directory "pkg" containing the source for "pkg 0.1.0" and the user runs pip install ./pkg "pkg != 0.1.0" they will get the following error:

ERROR: Cannot install oops because these package versions have conflicting dependencies.
The conflict is caused by:
    The user requested ExplicitRequirement(LinkCandidate('file:///C:/Users/Gustav/AppData/Local/Temp/pytest-of-Gustav/pytest-114/test_new_resolver_local_and_re0/workspace/scratch/pkg'))
    The user requested pkg!=0.1.0

There are a number of possible solutions. For instructions on how to do these steps visit: https://pypa.io/SomeLink
ERROR: No matching distribution found for pkg

Problems with this:

  1. That "Cannot install oops" is a placeholder, because none of the causes have "parent" values for us to report.
  2. That ExplicitRequirement(...) is hopeless. We need to trace back from that to the original directory name the user supplied. We may have that info (it gets passed around in came_from attributes, but we may have lost them when we build candidates from requirements).

More work to do here...

Interestingly, we didn't have any sort of test for this case. I've added one as the first stage of fixing the issues 🙂

@pfmoore
Copy link
Member Author

pfmoore commented Jun 4, 2020

Converting this to a draft so we don't accidentally merge it.

@pfmoore pfmoore marked this pull request as draft June 4, 2020 13:27
@uranusjr
Copy link
Member

uranusjr commented Jun 4, 2020

Regarding the ExplicitRequirement problem, IMO it’s good enough if we return the absolute path, if that’s as far as we can get.

@pfmoore
Copy link
Member Author

pfmoore commented Jun 4, 2020

IMO it’s good enough if we return the absolute path

I'm pretty sure getting any closer to "what the user typed" than an absolute path is going to be far more work than the improvement in the message would justify.

If the UX people find, in their research, that it is worth that effort, we can iterate on it later. I'm going to stick with absolute path for now. (Even that likely needs a new method on the Candidate, to abstract away the difference between linked candidates and other types...)

@uranusjr
Copy link
Member

uranusjr commented Jun 4, 2020

If we’re adding a method, would it feel more comfortable to have a format_for_error() or similar to handle all the “show a human-readable representation for this requirement” logic?

@pfmoore
Copy link
Member Author

pfmoore commented Jun 4, 2020

Yeah, that sounds like a nice idea :-)

@pfmoore pfmoore marked this pull request as ready for review June 5, 2020 12:00
@pfmoore
Copy link
Member Author

pfmoore commented Jun 5, 2020

OK, I think I've fixed that case.

return "{} {} (from {})".format(
self.name,
self.version,
self.link.file_path
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 work for remote links?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, I guess that's another test to add :-)
Thanks.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added a fix, but couldn't work out a reasonable way to add a test. Suggestions welcome :-)

@BrownTruck
Copy link
Contributor

Hello!

I am an automated bot and I have noticed that this pull request is not currently able to be merged. If you are able to either merge the master branch into this pull request or rebase this pull request against master then it will be eligible for code review and hopefully merging!

@BrownTruck BrownTruck added the needs rebase or merge PR has conflicts with current master label Jun 10, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Jun 17, 2020
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.

One thought about how the cached packages/links look, rest seems OK to me.

return "{} {} (from {})".format(
self.name,
self.version,
self.link.file_path if self.link.is_file else self.link
Copy link
Member

Choose a reason for hiding this comment

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

How does this look for cached paths?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try it tomorrow, but from my recollection "annoyingly long" ;-)

Copy link
Member

Choose a reason for hiding this comment

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

Can we use the original link w/ (cached) in the name in that case?

OK to do this in a follow up, if you file an issue to aggregate all the call sites that print req.link.

@pfmoore
Copy link
Member Author

pfmoore commented Jun 18, 2020

OK, I've opened #8463 for the follow-up work requested by @pradyunsg. Merging this now, as it's clearly better than what we currently have. We can easily make any further improvements in follow-up PRs.

@pfmoore pfmoore deleted the nr_conflict_message branch June 18, 2020 14:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
skip news Does not need a NEWS file entry (eg: trivial changes) type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ResolutionImpossible error message
5 participants