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

Move “is yanked” warning out of PackageFinder #8123

Closed
uranusjr opened this issue Apr 24, 2020 · 2 comments
Closed

Move “is yanked” warning out of PackageFinder #8123

uranusjr opened this issue Apr 24, 2020 · 2 comments

Comments

@uranusjr
Copy link
Member

Environment

  • pip version: >=19.3 (i.e. post PEP 592 implementation)
  • Python version: All
  • OS: All

Description
Currently PackageFinder issues a warning immediately when it decides a yanked link is the chosen as “best”:

def sort_best_candidate(
self,
candidates, # type: List[InstallationCandidate]
):
# type: (...) -> Optional[InstallationCandidate]
"""
Return the best candidate per the instance's sort order, or None if
no candidate is acceptable.
"""
if not candidates:
return None
best_candidate = max(candidates, key=self._sort_key)
# Log a warning per PEP 592 if necessary before returning.
link = best_candidate.link
if link.is_yanked:
reason = link.yanked_reason or '<none given>'
msg = (
# Mark this as a unicode string to prevent
# "UnicodeEncodeError: 'ascii' codec can't encode character"
# in Python 2 when the reason contains non-ascii characters.
u'The candidate selected for download or install is a '
'yanked version: {candidate}\n'
'Reason for being yanked: {reason}'
).format(candidate=best_candidate, reason=reason)
logger.warning(msg)
return best_candidate

There are fouer code paths in pip that can reach it. One of them (self_outdated_check.pip_self_version_check) never triggers the warning since the finder is constructed to exclude all yanked versions. Two are in the legacy resolver. The legacy resolver always selects the best available candidate. So the warning is valid because user should be warned if that candidate is yanked.

The other path is from the new resolver, which presents a problem. The candidate it chooses may be “unchosen” later (i.e. backtracking). The result is PackageFinder may show yanked warnings for candidates that are chosen during resolution but eventually discarded.

I worked on this briefly in #7796 but abandoned the work because there’s no easy way to refactor it, and the issue was only theoratical at the time anyway. But now we have the new resolver out for testing, and that PyPI actually starts supporting yanking, we’ll need to look into this soon.

Expected behavior
I think the warning needs to be moved into the resolver. This is not too difficult for the legacy resolver; just move it to the call site. It will be more difficult for the new resolver. We can issue a warning only when we know for sure the candidate is chosen (i.e. after the resolution completes), but before the link is resolved to the wheel cache (otherwise be loose the yank information). But OTOH the cache resolution must be done during the resolution (to fetch package metadata). So some additional refactoring is likely needed to make this work.

@triage-new-issues triage-new-issues bot added the S: needs triage Issues/PRs that need to be triaged label Apr 24, 2020
@uranusjr
Copy link
Member Author

Wheel cache work for the new resolver is #8066.

@uranusjr
Copy link
Member Author

uranusjr commented May 7, 2020

CLosing this since the yanked message is not out of the package finder. I’m opening a new issue to track implementing the yanked message in the new resolver.

@uranusjr uranusjr closed this as completed May 7, 2020
@pradyunsg pradyunsg removed the S: needs triage Issues/PRs that need to be triaged label Feb 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants