2020 resolver: list out helpful info on basic resolution failures #9405

MrMino · 2020-12-31T02:32:10Z

Adds the old diagnostic information ("from versions:") to the new resolver.

If there was a failure in resolution of a requirement that stemmed from
a simple fact of there being no candidate found that could possibly
satisfy it, make pip output diagnostic information about which versions
of the package are available to pip (outputs "(none)" if there weren't any).

Note: since finder.find_all_candidates() uses lru_cache, this avoids performing additional API calls to PyPI.

pradyunsg · 2020-12-31T13:26:00Z

Thanks for the PR! Could you also show a sample of how this looks, so that we can discuss the design before we dive into the implementation?

uranusjr · 2020-12-31T17:18:19Z

src/pip/_internal/resolution/resolvelib/factory.py

+                f"Cache dir:\n"
+                f"{cache_dir or '(not used)'}"
+            )
+            logger.warning(diagnostic_hint)


The approach lgtm, but the message formatting is much too complicated imo. I would simplify this to something like

Available versions: <list of versions joined by comma> Sources: <list of source URLs joined by newline and indented>

Some points to note:

The project name is already shown in the message directly above and would be duplicated info here.

Available versions need to be sorted to be readable (they are not right now).

Showing the cache directory is not useful imo. Cache is only a factor during package build and install, not here.

comes_from is not necessarily an index page.

Thanks.

The project name is already shown in the message directly above and would be duplicated info here.

Correct. Removed

Available versions need to be sorted to be readable (they are not right now).

Added sorting to versions using distutils.version.LooseVersion. Indexes are now sorted too, as to ensure that they don't jump around on different runs.

Showing the cache directory is not useful imo.

Yea, I guess even if it was, you might as well just do pip cache info. Having it in every one of these outputs is just too verbose anyway. I removed it.

comes_from is not necessarily an index page.

Huh? What else could it be at that point? Replaced Index(es): with Sources:.

This is how it looks right now:

I've also removed the empty newlines, so now it looks like this:

Instead of making this a separate logging.warning call, I think we should be adding this information to the original logging.critical call.

Added sorting to versions using distutils.version.LooseVersion

We should use the version type from packaging. I don't think we should add a dependency on distutils here, as Python core is planning on removing that at some point.

@pfmoore I didn't know about that. Will do.
@pradyunsg The point of doing that was to make it different from the error messages, as - technically - these aren't, and for readability. I can change that too, no problem there.

"Could not find a version, here are the versions" is OK as an error message. I don't think we need to make it look distinct or whatever.

Huh? What else could it be at that point? Replaced Index(es): with Sources:.

PackageFinder also looks at find-links pages, which are technically not package indexes.

This reminds me though, we should add a check and display this whole message only when the user did not specify any direct URL requirements on the package. It would be quite confusing IMO if the user specify foo @ git+https://github.com/... and pip displays this.

@pfmoore Done.
@pradyunsg Done. This is a comparison of the before / after:

@uranusjr The sources section has been removed. Not sure how to go about the foo@git+... thing though. I was of the impression that it is not possible to get a ResolutionImpossible exception when doing that.

src/pip/_internal/resolution/resolvelib/factory.py

pradyunsg · 2021-01-02T13:27:54Z

src/pip/_internal/resolution/resolvelib/factory.py

+                f"Cache dir:\n"
+                f"{cache_dir or '(not used)'}"
+            )
+            logger.warning(diagnostic_hint)


"Could not find a version, here are the versions" is OK as an error message. I don't think we need to make it look distinct or whatever.

src/pip/_internal/resolution/resolvelib/factory.py

pfmoore · 2021-01-02T14:47:56Z

Also, can I go on record as saying that while reporting the available versions in case of an error is a reasonable thing to do, I am absolutely against making pip install foo== the supported way of asking "what versions of foo exist?"

If that functionality is useful (and I can see that it is) then it should be a command in its own right. And invalid requirement specifiers like foo== should be rejected. I feel that allowing an empty version is a bug in packaging.specifiers.SpecifierSet which should be fixed - I can see nothing in PEP 440 which suggests that it's valid, or why it should be valid.

MrMino · 2021-01-02T16:07:15Z

@pradyunsg
I've brought back the original report format. Here's the comparison.

pfmoore · 2021-01-02T16:31:23Z

@MrMino I'm not going to get into an extended debate on this. I've stated my opinion and I stick by it. Other pip maintainers may differ, and if so we'll come to a consensus at some point. None of which alters the fact that this is useful as diagnostic data after an error. (Which is how I'd prefer to frame this PR).

IMO there's no reasonable way to interpret PEP 440 as allowing foo== as a valid specifier, so I think that packaging should reject it (and as a consequence, pip would never try to resolve). If and when that happens¹, pip should have an alternative available (in the form of a "list available versions" subcommand), rather than complaining that packaging "has" to support it.

¹ Normally, I'd raise a bug report against packaging. I'm not doing so at the moment in this case as I feel that would be needlessly passive-aggressive given that pip users do value this loophole currently.

MrMino · 2021-01-02T16:53:50Z

I'm not going to get into an extended debate on this.

@pfmoore I was asking to find a good mental model that would make me understand your position. Alright - I won't go into that here anymore.

pfmoore · 2021-01-02T18:02:09Z

I was asking to find a good mental model that would make me understand your position

I have no problem with clarifying my thinking if it helps, I just don't intend to try to persuade you if you believe differently 🙂

My model is simple. pip install takes a requirement (project name + specifier) and foo== isn't a valid requirement, because == isn't a valid specifier. In addition, pip install as a command is intended to install stuff, not as a way to trigger an error that provides informative information about a project.

If people want to find out what versions of foo are available, we should support that via a dedicated command (maybe pip list foo --versions, but we can bikeshed the name as much as anyone has energy for 🙂). The fact that pip install foo== has provided that information historically is an accident, and in fact is the accidental consequence of a bug (accepting foo== as a valid requirement). People could just as easily have used foo==999.76.34.post0 to get the information, which would have worked just as well, but might have made it a little clearer that it wasn't the "right" way to get that information.

Does that help you understand my logic?

uranusjr · 2021-01-02T18:20:34Z

I opened a thread in packaging to discuss the behaviour.

MrMino · 2021-01-02T21:40:48Z

@pfmoore I understand the willingness to make it follow a spec. But I'd say that following the spec ≠ usability of a CLI. It's strange to me that you would prioritize former over the latter, but I guess it's a matter of opinion. Agree to disagree?

Anything else I should add to this PR?

pradyunsg · 2021-01-02T21:45:15Z

I don't think there's more needed here.

And, also, I hate that we constantly hit Hyrum's Law. :)

Comments addressed.

pradyunsg · 2021-01-02T21:58:13Z

Apologies, this is the first I'm hearing of Hyrum's Law being considered derogatory. If you would be willing to elaborate/share on why that is, I'd like to know more! :)

I thought it applied here because we made a change to an undocumented only-an-implementation-detail behaviour of pip, which broke users (which is what this PR is fixing).

On a completely different note, thanks a lot @MrMino for being so receptive on feedback and being, overall, a pleasure to collaborate with so far in this PR! Much appreciated! ^.^

MrMino · 2021-01-02T22:23:50Z

@pradyunsg The way I see it, you wouldn't like to tell your son that he's a result of a Murphy's law, it wouldn't be a good PR to tell your contractor that his refactor shows signs of Tesler's law, it wouldn't look good if I told you that your website is a failure in application of Jakob's Law.
On their own and in general these are true observations, but applying most of these witty epigrams to the work of others is just a veiled way of saying "your work is gawky as hell" - that's how I and many others (speaking from experience) hear them.

IMO applications of Hyrum's law as a label should be limited to the places where there's a clear API contract. UX / GUIs / CLIs (for the most part) don't have that.

thanks a lot @MrMino for being so receptive on feedback and being, overall, a pleasure to collaborate with so far in this PR! Much appreciated! ^.^

I'm always surprised to hear this, as to myself I sound like a huge pain in the butt most of the time 😉. Thanks though, appreciated.

MrMino · 2021-01-02T23:02:37Z

Commits squashed, news entry reworded (I'm bad at sticking to my word choices). Should there be anything else to change / add - I'm all ears.

Thanks for the reviews, it was a pleasure to work with you guys :).

benjaoming · 2021-01-29T14:43:50Z

Great work @MrMino !

We're hitting a pytest-xdist bug.

@pradyunsg could you clarify what this bug is about? Is it something that the PR Author should fix or a known bug that is unrelated to the changes?

BrownTruck · 2021-02-21T20:16:13Z

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!

ddelange · 2021-02-22T09:04:28Z

It's worth noting that packaging will be dropping LegacyVersion sometime soon, at which point a specifier of == will no longer be valid. At that point pip install foo== will stop working as a way to list all available versions anyway (as will other variants using non-compliant versions, such as pip install foo=versions).

In this case, what will become the new approach to trigger this output?

pfmoore · 2021-02-22T10:55:28Z

In this case, what will become the new approach to trigger this output?

There isn't one yet, because no-one has proposed an approach or written a PR.

uranusjr · 2021-02-22T20:06:43Z

If you just want a “reasonable hack”, something like foo==9999999999999999 would work most of the time.

BrownTruck · 2021-02-25T21:16:32Z

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!

In the new resolver the "(from versions ...)" message, shown on failure to resolve a package, has been removed. This commit brings it back.

MrMino · 2021-02-27T19:42:00Z

@uranusjr could you please look at it once more, just to make sure that after your changes in 917ecad everything is still in order?

src/pip/_internal/resolution/resolvelib/factory.py

uranusjr · 2021-02-27T23:22:17Z

One nitpick, the rebase looks good to me.

Co-authored-by: Tzu-ping Chung <[email protected]>

MrMino · 2021-02-27T23:45:59Z

Good to know, thank you.

MrMino force-pushed the diagnostic_aversion branch 2 times, most recently from 50f6fdc to 7a5ab7f Compare December 31, 2020 02:42

This comment has been minimized.

Sign in to view

MrMino changed the title ~~List out helpful info on basic resolution failures~~ New resolver: list out helpful info on basic resolution failures Dec 31, 2020

MrMino changed the title ~~New resolver: list out helpful info on basic resolution failures~~ 2020 resolver: list out helpful info on basic resolution failures Dec 31, 2020

uranusjr reviewed Dec 31, 2020

View reviewed changes

pradyunsg requested changes Jan 2, 2021

View reviewed changes

src/pip/_internal/resolution/resolvelib/factory.py Outdated Show resolved Hide resolved

pradyunsg previously requested changes Jan 2, 2021

View reviewed changes

pfmoore reviewed Jan 2, 2021

View reviewed changes

src/pip/_internal/resolution/resolvelib/factory.py Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

MrMino requested a review from pradyunsg January 2, 2021 15:45

MrMino force-pushed the diagnostic_aversion branch from e083038 to d5ce3a0 Compare January 2, 2021 16:09

uranusjr mentioned this pull request Jan 2, 2021

Is an empty string a valid LegacyVersion? pypa/packaging#382

Closed

This comment has been minimized.

Sign in to view

pradyunsg removed their request for review January 2, 2021 21:52

MrMino force-pushed the diagnostic_aversion branch from caaf4d9 to fde5f6f Compare January 2, 2021 22:59

MrMino mentioned this pull request Jan 12, 2021

[2020-resolver] No longer can get list of available versions. #9139

Closed

BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 21, 2021

MrMino force-pushed the diagnostic_aversion branch from 3017504 to 40df5c2 Compare February 21, 2021 22:29

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 21, 2021

BrownTruck added the needs rebase or merge PR has conflicts with current master label Feb 25, 2021

Bring back the "from versions:" message

a2c5794

In the new resolver the "(from versions ...)" message, shown on failure to resolve a package, has been removed. This commit brings it back.

MrMino force-pushed the diagnostic_aversion branch from 40df5c2 to a2c5794 Compare February 27, 2021 19:38

pypa-bot removed the needs rebase or merge PR has conflicts with current master label Feb 27, 2021

MrMino requested a review from uranusjr February 27, 2021 19:41

uranusjr reviewed Feb 27, 2021

View reviewed changes

src/pip/_internal/resolution/resolvelib/factory.py Outdated Show resolved Hide resolved

Refactor set(...) → {...}

d1d9145

Co-authored-by: Tzu-ping Chung <[email protected]>

uranusjr approved these changes Feb 28, 2021

View reviewed changes

MrMino requested a review from pradyunsg February 28, 2021 02:11

pfmoore approved these changes Feb 28, 2021

View reviewed changes

pradyunsg approved these changes Feb 28, 2021

View reviewed changes

pradyunsg merged commit 8223d29 into pypa:master Feb 28, 2021

ddelange mentioned this pull request Feb 28, 2021

🔖 Allow pip 21.1 ddelange/pipgrip#49

Merged

Irahaze5 approved these changes Mar 2, 2021

View reviewed changes

MrMino deleted the diagnostic_aversion branch March 2, 2021 21:05

github-actions bot locked as resolved and limited conversation to collaborators Oct 1, 2021

2020 resolver: list out helpful info on basic resolution failures #9405

2020 resolver: list out helpful info on basic resolution failures #9405

Conversation

MrMino commented Dec 31, 2020 • edited Loading

This comment has been minimized.

pradyunsg commented Dec 31, 2020

This comment has been minimized.

uranusjr Dec 31, 2020 • edited Loading

Choose a reason for hiding this comment

MrMino Jan 1, 2021 • edited Loading

Choose a reason for hiding this comment

MrMino Jan 1, 2021

Choose a reason for hiding this comment

pradyunsg Jan 1, 2021

Choose a reason for hiding this comment

pfmoore Jan 1, 2021

Choose a reason for hiding this comment

MrMino Jan 2, 2021

Choose a reason for hiding this comment

pradyunsg Jan 2, 2021

Choose a reason for hiding this comment

uranusjr Jan 2, 2021

Choose a reason for hiding this comment

MrMino Jan 2, 2021 • edited Loading

Choose a reason for hiding this comment

MrMino Jan 2, 2021 • edited Loading

Choose a reason for hiding this comment

pradyunsg Jan 2, 2021

Choose a reason for hiding this comment

pfmoore commented Jan 2, 2021

This comment has been minimized.

MrMino commented Jan 2, 2021

pfmoore commented Jan 2, 2021

MrMino commented Jan 2, 2021 • edited Loading

pfmoore commented Jan 2, 2021

uranusjr commented Jan 2, 2021

MrMino commented Jan 2, 2021 • edited Loading

pradyunsg commented Jan 2, 2021 • edited Loading

This comment has been minimized.

pradyunsg commented Jan 2, 2021 • edited Loading

MrMino commented Jan 2, 2021 • edited Loading

MrMino commented Jan 2, 2021

benjaoming commented Jan 29, 2021

BrownTruck commented Feb 21, 2021

ddelange commented Feb 22, 2021

pfmoore commented Feb 22, 2021

uranusjr commented Feb 22, 2021

BrownTruck commented Feb 25, 2021

MrMino commented Feb 27, 2021

uranusjr commented Feb 27, 2021 • edited Loading

MrMino commented Feb 27, 2021

MrMino commented Dec 31, 2020 •

edited

Loading

uranusjr Dec 31, 2020 •

edited

Loading

MrMino Jan 1, 2021 •

edited

Loading

MrMino Jan 2, 2021 •

edited

Loading

MrMino Jan 2, 2021 •

edited

Loading

MrMino commented Jan 2, 2021 •

edited

Loading

MrMino commented Jan 2, 2021 •

edited

Loading

pradyunsg commented Jan 2, 2021 •

edited

Loading

pradyunsg commented Jan 2, 2021 •

edited

Loading

MrMino commented Jan 2, 2021 •

edited

Loading

uranusjr commented Feb 27, 2021 •

edited

Loading