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

Skip candidate not providing valid metadata #9264

Conversation

uranusjr
Copy link
Member

@uranusjr uranusjr commented Dec 11, 2020

I had a galaxy-brain moment and realised the resolver does not really need to offer additional hooks for this. Since the candidate sequence is already implemented lazily, all we need to do is to check if the build fails during iteration, and skip the candidate when it happens.

Technical details (taken directly from the commit message):

This is done by catching InstallationError from the underlying distribution preparation logic. There are three cases to catch:

  1. Candidates from indexes. These are simply ignored since we can potentially satisfy the requirement with other candidates.
  2. Candidates from URLs with a dist name (PEP 508 or #egg=). A new UnsatisfiableRequirement class is introduced to represent this; it is like an ExplicitRequirement without an underlying candidate. As the name suggests, an instance of this can never be satisfied, and will cause eventual backtracking.
  3. Candidates from URLs without a dist name. This is only possible for top-level user requirements, and no recourse is possible for them. So we error out eagerly.

The InstallationError raised during distribution preparation is cached in the factory, like successfully prepared candidates, since we don't want to repeatedly try to build a candidate if we already know it'd fail. Plus pip's preparation logic also does not allow packages to be built multiple times anyway.

Fix #9203, and also fix #9246 without changing with minimal change to the underlying build code.

@uranusjr uranusjr force-pushed the new-resolver-dont-abort-on-inconsistent-candidate branch 3 times, most recently from ae8d0e0 to 9405d00 Compare December 11, 2020 18:04
This is done by catching InstallationError from the underlying
distribution preparation logic. There are three cases to catch:

1. Candidates from indexes. These are simply ignored since we can
   potentially satisfy the requirement with other candidates.
2. Candidates from URLs with a dist name (PEP 508 or #egg=). A new
   UnsatisfiableRequirement class is introduced to represent this; it is
   like an ExplicitRequirement without an underlying candidate. As the
   name suggests, an instance of this can never be satisfied, and will
   cause eventual backtracking.
3. Candidates from URLs without a dist name. This is only possible for
   top-level user requirements, and no recourse is possible for them. So
   we error out eagerly.

The InstallationError raised during distribution preparation is cached
in the factory, like successfully prepared candidates, since we don't
want to repeatedly try to build a candidate if we already know it'd
fail. Plus pip's preparation logic also does not allow packages to be
built multiple times anyway.
@uranusjr uranusjr force-pushed the new-resolver-dont-abort-on-inconsistent-candidate branch from 9405d00 to d45541c Compare December 11, 2020 18:23
@stonebig
Copy link
Contributor

in pip-20.3.2 ?

Copy link
Member

@pfmoore pfmoore left a comment

Choose a reason for hiding this comment

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

A couple of places where I think explanatory comments could help, but this LGTM!

@pypa-bot
Copy link

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 eligible for code review and hopefully merging!

@pypa-bot pypa-bot added the needs rebase or merge PR has conflicts with current master label Dec 12, 2020
@pypa-bot pypa-bot removed the needs rebase or merge PR has conflicts with current master label Dec 12, 2020
@uranusjr
Copy link
Member Author

Comments added + resolved conflicts with master.

@uranusjr
Copy link
Member Author

Eh damn it, the XML-RPC tests are acting up again.

@pradyunsg pradyunsg merged commit 0f8f3d7 into pypa:master Dec 12, 2020
@pradyunsg
Copy link
Member

HMMM... one of the nuances here, that I’m only thinking of after the merge: if I install package that can’t be installed because no wheels are compatible and I’m missing a build dependency or whatever, pip will work through the entire set of versions... that’s not ideal. :(

@pradyunsg pradyunsg added this to the 20.3.2 milestone Dec 12, 2020
@pradyunsg
Copy link
Member

Hmm... I guess this is OK to release tho, and we can deal with any reactions as they come in. :)

@pfmoore
Copy link
Member

pfmoore commented Dec 12, 2020

pip will work through the entire set of versions

Of course, technically, an older version might have different build dependencies and work. So that's actually correct behaviour. Not really what the user wants, of course, but technically correct 🙁 We could really do with working out a good heuristic for stopping because we've got to versions that are "too old to be worth considering", but I honestly have no idea how we'd do that. That question's being handled on another issue, though, so it's offtopic here.

bors bot referenced this pull request in duckinator/emanate Dec 15, 2020
204: Update pip to 20.3.3 r=duckinator a=pyup-bot


This PR updates [pip](https://pypi.org/project/pip) from **20.3.1** to **20.3.3**.



<details>
  <summary>Changelog</summary>
  
  
   ### 20.3.3
   ```
   ===================

Bug Fixes
---------

- Revert &quot;Skip candidate not providing valid metadata&quot;, as that caused pip to be overeager about downloading from the package index. (`9264 &lt;https://github.com/pypa/pip/issues/9264&gt;`_)
   ```
   
  
  
   ### 20.3.2
   ```
   ===================

Features
--------

- New resolver: Resolve direct and pinned (``==`` or ``===``) requirements first
  to improve resolver performance. (`9185 &lt;https://github.com/pypa/pip/issues/9185&gt;`_)
- Add a mechanism to delay resolving certain packages, and use it for setuptools. (`9249 &lt;https://github.com/pypa/pip/issues/9249&gt;`_)

Bug Fixes
---------

- New resolver: The &quot;Requirement already satisfied&quot; log is not printed only once
  for each package during resolution. (`9117 &lt;https://github.com/pypa/pip/issues/9117&gt;`_)
- Fix crash when logic for redacting authentication information from URLs
  in ``--help`` is given a list of strings, instead of a single string. (`9191 &lt;https://github.com/pypa/pip/issues/9191&gt;`_)
- New resolver: Correctly implement PEP 592. Do not return yanked versions from
  an index, unless the version range can only be satisfied by yanked candidates. (`9203 &lt;https://github.com/pypa/pip/issues/9203&gt;`_)
- New resolver: Make constraints also apply to package variants with extras, so
  the resolver correctly avoids backtracking on them. (`9232 &lt;https://github.com/pypa/pip/issues/9232&gt;`_)
- New resolver: Discard a candidate if it fails to provide metadata from source,
  or if the provided metadata is inconsistent, instead of quitting outright. (`9246 &lt;https://github.com/pypa/pip/issues/9246&gt;`_)

Vendored Libraries
------------------

- Update vendoring to 20.8

Improved Documentation
----------------------

- Update documentation to reflect that pip still uses legacy resolver by default in Python 2 environments. (`9269 &lt;https://github.com/pypa/pip/issues/9269&gt;`_)
   ```
   
  
</details>


 

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pip
  - Changelog: https://pyup.io/changelogs/pip/
  - Homepage: https://pip.pypa.io/
</details>



205: Update pytest to 6.2.1 r=duckinator a=pyup-bot


This PR updates [pytest](https://pypi.org/project/pytest) from **6.1.2** to **6.2.1**.



*The bot wasn't able to find a changelog for this release. [Got an idea?](https://github.com/pyupio/changelogs/issues/new)*

<details>
  <summary>Links</summary>
  
  - PyPI: https://pypi.org/project/pytest
  - Homepage: https://docs.pytest.org/en/latest/
</details>



Co-authored-by: pyup-bot <github-bot@pyup.io>
@uranusjr uranusjr deleted the new-resolver-dont-abort-on-inconsistent-candidate branch February 19, 2021 06:48
@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
5 participants