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

Reduce resolver rounds by an order of magnitude #11908

Merged
merged 1 commit into from
Mar 29, 2023

Conversation

pradyunsg
Copy link
Member

@pradyunsg pradyunsg commented Mar 28, 2023

This ensures that we're not trying "too hard" to resolve a dependency graph. Combined with the recent improvements to resolvelib, this should ensure that the resolution process takes a limited amount of wall clock time for complex graphs.

Toward #11480

@pradyunsg pradyunsg added the type: enhancement Improvements to functionality label Mar 28, 2023
@pfmoore
Copy link
Member

pfmoore commented Mar 28, 2023

In general, I'm +1 on this, but with my RM hat on, is this appropriate for 23.1, or do we need to do anything to avoid the risk of breaking complex installs with the lower limit? While I know that no-one tests with github main, I sort of feel that we'd have been less exposed if we could say it's been in main for a month or so - but a last minute reduction feels more rushed.

I'm not going to block this PR, but we should consider how we'd handle possible issues (I would be fairly unhappy if we needed to do a fast bugfix release just to revert this). If people are comfortable that the risk is low, then that's good enough for me. Maybe @notatallshaw could test some of the complex installs he's been checking with the reduced limit? I for one would appreciate that.

@notatallshaw
Copy link
Member

notatallshaw commented Mar 28, 2023

Maybe @notatallshaw could test some of the complex installs he's been checking with the reduced limit? I for one would appreciate that.

Sure, I can check later this week, and I'll see if I can come up with an even more problematic requirements file.

I'm neutral on this change, because there isn't an understandable relationship between a given set of requirements and what the max rounds should be set to.

To me the only 2 numbers that have a clear understanding of what they mean are 0 and infinity, if infinity is set then Pip should find a solution if one exists (assuming no other limits) and if 0 then no backtracking should be done and the user has to fix their requirements/constraints.

The main pro of this change seem to be most related to CI tools to exit early and save energy / cost, as a user can exit the process at any time. Though a secondary pro would be for new users to Pip/Python not waiting several hours for an error.

The cons are what if a specific set of requirements needed just a small number of more rounds to solve and the requirements are complex enough the user is expecting them to take time to resolve, if resolution is allowed to complete then user can use the resolved requirements to improve their requirements going forward, but the max rounds error produced isn't particularly useful (however hopefully #10937 helps a little bit when the max rounds error happens).

@pfmoore
Copy link
Member

pfmoore commented Mar 28, 2023

One thought here - maybe pip install --dry-run --report should have a larger number of rounds, as presumably the user is willing to wait longer to get output that they can then use to create a pinned requirements file?

@pradyunsg pradyunsg force-pushed the reduce-resolver-rounds branch 2 times, most recently from f6847e5 to f613141 Compare March 28, 2023 22:34
This reduces how much pip will attempt to backtrack.
@uranusjr
Copy link
Member

IIRC there were requests making this configurable. Although I don’t generally like this, maybe it is a good idea to introduce an undocumented environment variable for this while we test the water? This can help us figure out with the user whether increasing the number is actually a good solution for various situations.

@pradyunsg pradyunsg marked this pull request as ready for review March 29, 2023 08:00
@pradyunsg
Copy link
Member Author

pradyunsg commented Mar 29, 2023

IIRC there were requests making this configurable.

The request to make this configurable is in #11480 -- which basically boils down to "things take too long". This PR reduces the "too long" time by an order of magnitude, which brings down the failure mode from half-a-work-day to a "run this and grab a meal/coffee" order of time.

If we want to make this configurable, I'd prefer if we do that in a separate change. I think this one is a small independently sensible change that makes a lot of sense in the context of backjumping being introduced, which reduces the number of rounds needed for complex resolves by skipping over irrelevant packages.

maybe pip install --dry-run --report should have a larger number of rounds

I'm wary of having different behaviours for flags that don't say anything about that. If we want this to be changed, then we should make this a knob.

do we need to do anything to avoid the risk of breaking complex installs with the lower limit?

There's a small risk, but I think it's largely mitigated by the fact that the resolver is going to use fewer rounds to get to a relevant package.

To me the only 2 numbers that have a clear understanding of what they mean are 0 and infinity, if infinity is set then Pip should find a solution if one exists (assuming no other limits) and if 0 then no backtracking should be done and the user has to fix their requirements/constraints.

Well... I think that understanding is incorrect.

A round is the resolver attempting to "pin" a single unsatisfied name in the graph. Having max_rounds=0 means that the resolver will be able to resolve at most 0 packages. Backtracking means that if you have 100 names in the final graph, it would take the resolver >100 rounds to figure out a solution. Having max_rounds=infinity means that the resolver will work through as much of the search graph as necessary to get to resolution impossible / resolution found.

@pfmoore
Copy link
Member

pfmoore commented Mar 29, 2023

If we want to make this configurable, I'd prefer if we do that in a separate change.

Fair. And I'm still not convinced this is something we should expect users to be able to configure meaningfully.

I'm wary of having different behaviours for flags that don't say anything about that.

Agreed, that's a reasonable point.

There's a small risk, but I think it's largely mitigated by the fact that the resolver is going to use fewer rounds to get to a relevant package.

Makes sense.

OK, I think the options have been considered, and on reflection I'm happy that we've thought about them but it's not worth doing anything specific. +1 from me on this.

@notatallshaw
Copy link
Member

A round is the resolver attempting to "pin" a single unsatisfied name in the graph. Having max_rounds=0 means that the resolver will be able to resolve at most 0 packages.

Does that mean if a user has a frozen set of requirements that needed no backtracking and they had 1 more requirement that max rounds it would fail?

@pradyunsg
Copy link
Member Author

pradyunsg commented Mar 29, 2023

Does that mean if a user has a frozen set of requirements that needed no backtracking and they had 1 more requirement that max rounds it would fail?

Yup. Even equal to max rounds will fail, because it'll have a round of "I need nothing more".

pip --version
pip 23.1.dev0 from /Users/pradyunsg/Developer/github/pip/src/pip (python 3.11)g diff
diff --git a/src/pip/_internal/resolution/resolvelib/resolver.py b/src/pip/_internal/resolution/resolvelib/resolver.py
index 47bbfecce..ce92a5f6c 100644
--- a/src/pip/_internal/resolution/resolvelib/resolver.py
+++ b/src/pip/_internal/resolution/resolvelib/resolver.py
@@ -88,7 +88,7 @@ def resolve(
         )
 
         try:
-            limit_how_complex_resolution_can_be = 200000
+            limit_how_complex_resolution_can_be = 2
             result = self._result = resolver.resolve(
                 collected.requirements, max_rounds=limit_how_complex_resolution_can_be
             )pip install sampleproject
Collecting sampleproject
  Using cached sampleproject-3.0.0-py3-none-any.whl (4.7 kB)
Collecting peppercorn (from sampleproject)
  Using cached peppercorn-0.6-py3-none-any.whl (4.8 kB)
ERROR: Exception:
Traceback (most recent call last):
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/cli/base_command.py", line 160, in exc_logging_wrapper
    status = run_func(*args)
             ^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/cli/req_command.py", line 248, in wrapper
    return func(self, options, args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/commands/install.py", line 398, in run
    requirement_set = resolver.resolve(
                      ^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/resolution/resolvelib/resolver.py", line 92, in resolve
    result = self._result = resolver.resolve(
                            ^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_vendor/resolvelib/resolvers.py", line 546, in resolve
    state = resolution.resolve(requirements, max_rounds=max_rounds)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_vendor/resolvelib/resolvers.py", line 457, in resolve
    raise ResolutionTooDeep(max_rounds)
pip._vendor.resolvelib.resolvers.ResolutionTooDeep: 2pip install sampleproject==3.0.0 peppercorn==0.6  
Collecting sampleproject==3.0.0
  Using cached sampleproject-3.0.0-py3-none-any.whl (4.7 kB)
Collecting peppercorn==0.6
  Using cached peppercorn-0.6-py3-none-any.whl (4.8 kB)
ERROR: Exception:
Traceback (most recent call last):
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/cli/base_command.py", line 160, in exc_logging_wrapper
    status = run_func(*args)
             ^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/cli/req_command.py", line 248, in wrapper
    return func(self, options, args)
           ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/commands/install.py", line 398, in run
    requirement_set = resolver.resolve(
                      ^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_internal/resolution/resolvelib/resolver.py", line 92, in resolve
    result = self._result = resolver.resolve(
                            ^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_vendor/resolvelib/resolvers.py", line 546, in resolve
    state = resolution.resolve(requirements, max_rounds=max_rounds)
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/pradyunsg/Developer/github/pip/src/pip/_vendor/resolvelib/resolvers.py", line 457, in resolve
    raise ResolutionTooDeep(max_rounds)
pip._vendor.resolvelib.resolvers.ResolutionTooDeep: 2pip install peppercorn==0.6                     
Collecting peppercorn==0.6
  Using cached peppercorn-0.6-py3-none-any.whl (4.8 kB)
Installing collected packages: peppercorn
Successfully installed peppercorn-0.6

@pradyunsg pradyunsg merged commit 10d9cbc into pypa:main Mar 29, 2023
@pradyunsg pradyunsg deleted the reduce-resolver-rounds branch March 29, 2023 21:59
@notatallshaw
Copy link
Member

Even though this already got merged I did run some tests over the last day, summary of my findings:

  • It's possible to at least contrive requirements that takes more than 200'000 rounds to finish
  • In my testing these requirements were considerably more complex than issues posted to Pip's issue tracker
  • Each individual round of resolution took much longer, presumably related to the complexity of the requirements, in my examples that pass 200'000 rounds it took over 6 hours on my very fast computer
  • In my examples I was able to show they were not resolvable as I took a subset of the requirements and got a resolution impossible error

kai687 pushed a commit to kai687/sphinxawesome-theme that referenced this pull request Apr 16, 2023
Bumps [pip](https://github.com/pypa/pip) from 23.0.1 to 23.1.
<details>
<summary>Changelog</summary>
<p><em>Sourced from <a
href="https://github.com/pypa/pip/blob/main/NEWS.rst">pip's
changelog</a>.</em></p>
<blockquote>
<h1>23.1 (2023-04-15)</h1>
<h2>Deprecations and Removals</h2>
<ul>
<li>Remove support for the deprecated <code>--install-options</code>.
(<code>[#11358](pypa/pip#11358)
&lt;https://github.com/pypa/pip/issues/11358&gt;</code>_)</li>
<li><code>--no-binary</code> does not imply <code>setup.py
install</code> anymore. Instead a wheel will be
built locally and installed.
(<code>[#11451](pypa/pip#11451)
&lt;https://github.com/pypa/pip/issues/11451&gt;</code>_)</li>
<li><code>--no-binary</code> does not disable the cache of locally built
wheels anymore. It only
means &quot;don't download wheels&quot;.
(<code>[#11453](pypa/pip#11453)
&lt;https://github.com/pypa/pip/issues/11453&gt;</code>_)</li>
<li>Deprecate <code>--build-option</code> and
<code>--global-option</code>. Users are invited to switch to
<code>--config-settings</code>.
(<code>[#11859](pypa/pip#11859)
&lt;https://github.com/pypa/pip/issues/11859&gt;</code>_)</li>
<li>Using <code>--config-settings</code> with projects that don't have a
<code>pyproject.toml</code> now print
a deprecation warning. In the future the presence of config settings
will automatically
enable the default build backend for legacy projects and pass the
setttings to it.
(<code>[#11915](pypa/pip#11915)
&lt;https://github.com/pypa/pip/issues/11915&gt;</code>_)</li>
<li>Remove <code>setup.py install</code> fallback when building a wheel
failed for projects without
<code>pyproject.toml</code>.
(<code>[#8368](pypa/pip#8368)
&lt;https://github.com/pypa/pip/issues/8368&gt;</code>_)</li>
<li>When the <code>wheel</code> package is not installed, pip now uses
the default build backend
instead of <code>setup.py install</code> for project without
<code>pyproject.toml</code>.
(<code>[#8559](pypa/pip#8559)
&lt;https://github.com/pypa/pip/issues/8559&gt;</code>_)</li>
</ul>
<h2>Features</h2>
<ul>
<li>Specify egg-link location in assertion message when it does not
match installed location to provide better error message for debugging.
(<code>[#10476](pypa/pip#10476)
&lt;https://github.com/pypa/pip/issues/10476&gt;</code>_)</li>
<li>Present conflict information during installation after each choice
that is rejected (pass <code>-vv</code> to <code>pip install</code> to
show it) (<code>[#10937](pypa/pip#10937)
&lt;https://github.com/pypa/pip/issues/10937&gt;</code>_)</li>
<li>Display dependency chain on each Collecting/Processing log line.
(<code>[#11169](pypa/pip#11169)
&lt;https://github.com/pypa/pip/issues/11169&gt;</code>_)</li>
<li>Support a per-requirement <code>--config-settings</code> option in
requirements files.
(<code>[#11325](pypa/pip#11325)
&lt;https://github.com/pypa/pip/issues/11325&gt;</code>_)</li>
<li>The <code>--config-settings</code>/<code>-C</code> option now
supports using the same key multiple
times. When the same key is specified multiple times, all values are
passed to
the build backend as a list, as opposed to the previous behavior, where
pip would
only pass the last value if the same key was used multiple times.
(<code>[#11681](pypa/pip#11681)
&lt;https://github.com/pypa/pip/issues/11681&gt;</code>_)</li>
<li>Add <code>-C</code> as a short version of the
<code>--config-settings</code> option.
(<code>[#11786](pypa/pip#11786)
&lt;https://github.com/pypa/pip/issues/11786&gt;</code>_)</li>
<li>Reduce the number of resolver rounds, since backjumping makes the
resolver more efficient in finding solutions. This also makes
pathological cases fail quicker.
(<code>[#11908](pypa/pip#11908)
&lt;https://github.com/pypa/pip/issues/11908&gt;</code>_)</li>
<li>Warn if <code>--hash</code> is used on a line without requirement in
a requirements file.
(<code>[#11935](pypa/pip#11935)
&lt;https://github.com/pypa/pip/issues/11935&gt;</code>_)</li>
<li>Stop propagating CLI <code>--config-settings</code> to the build
dependencies. They already did
not propagate to requirements provided in requirement files. To pass the
same config
settings to several requirements, users should provide the requirements
as CLI
arguments. (<code>[#11941](pypa/pip#11941)
&lt;https://github.com/pypa/pip/issues/11941&gt;</code>_)</li>
<li>Support wheel cache when using <code>--require-hashes</code>.
(<code>[#5037](pypa/pip#5037)
&lt;https://github.com/pypa/pip/issues/5037&gt;</code>_)</li>
<li>Add <code>--keyring-provider</code> flag. See the Authentication
page in the documentation for more info.
(<code>[#8719](pypa/pip#8719)
&lt;https://github.com/pypa/pip/issues/8719&gt;</code>_)</li>
<li>In the case of virtual environments, configuration files are now
also included from the base installation.
(<code>[#9752](pypa/pip#9752)
&lt;https://github.com/pypa/pip/issues/9752&gt;</code>_)</li>
</ul>
<h2>Bug Fixes</h2>
<ul>
<li>Fix grammar by changing &quot;A new release of pip available:&quot;
to &quot;A new release of pip is available:&quot; in the notice used for
indicating that.
(<code>[#11529](pypa/pip#11529)
&lt;https://github.com/pypa/pip/issues/11529&gt;</code>_)</li>
<li>Normalize paths before checking if installed scripts are on PATH.
(<code>[#11719](pypa/pip#11719)
&lt;https://github.com/pypa/pip/issues/11719&gt;</code>_)</li>
<li>Correct the way to decide if keyring is available.
(<code>[#11774](pypa/pip#11774)
&lt;https://github.com/pypa/pip/issues/11774&gt;</code>_)</li>
<li>More consistent resolution backtracking by removing legacy hack
related to setuptools resolution
(<code>[#11837](pypa/pip#11837)
&lt;https://github.com/pypa/pip/issues/11837&gt;</code>_)</li>
</ul>
<!-- raw HTML omitted -->
</blockquote>
<p>... (truncated)</p>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a
href="https://github.com/pypa/pip/commit/6424ac4600265490462015c2fc7f9a402dba9ed8"><code>6424ac4</code></a>
Bump for release</li>
<li><a
href="https://github.com/pypa/pip/commit/868338f9f79b58eff34dafb168aed65480d080d5"><code>868338f</code></a>
Update AUTHORS.txt</li>
<li><a
href="https://github.com/pypa/pip/commit/4f3a4f72697299da1a412cf10c919a989e0692f5"><code>4f3a4f7</code></a>
Merge pull request <a
href="https://redirect.github.com/pypa/pip/issues/11919">#11919</a> from
sbidoul/deprecate-legacy-ignore-config-setting...</li>
<li><a
href="https://github.com/pypa/pip/commit/dbf4e6842c9603792f6d3944a5c9cec17bd0a92a"><code>dbf4e68</code></a>
Merge pull request <a
href="https://redirect.github.com/pypa/pip/issues/11897">#11897</a> from
sbidoul/cache-hash-checking-sbi</li>
<li><a
href="https://github.com/pypa/pip/commit/efe2d27451d50b165df78093bf5885da713fbdf8"><code>efe2d27</code></a>
Further refactor is_wheel_from_cache</li>
<li><a
href="https://github.com/pypa/pip/commit/4beca6b4c9c510b19dbb6180e962425b89e8c839"><code>4beca6b</code></a>
Improve test</li>
<li><a
href="https://github.com/pypa/pip/commit/bd746e3136e5e1be2374a079bac66071dd967a8c"><code>bd746e3</code></a>
Introduce ireq.cached_wheel_source_link</li>
<li><a
href="https://github.com/pypa/pip/commit/caafe6e87d4f2998a77b194297e1c204cf6e10c2"><code>caafe6e</code></a>
Add a couple of asserts</li>
<li><a
href="https://github.com/pypa/pip/commit/a6ef6485be9512f18121298b058797c578f65d45"><code>a6ef648</code></a>
Rename original_link_is_in_wheel_cache to is_wheel_from_cache</li>
<li><a
href="https://github.com/pypa/pip/commit/ff8c8e38887880ad81ffd7cfc6a8373213c087b7"><code>ff8c8e3</code></a>
Cosmetics</li>
<li>Additional commits viewable in <a
href="https://github.com/pypa/pip/compare/23.0.1...23.1">compare
view</a></li>
</ul>
</details>
<br />


[![Dependabot compatibility
score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=pip&package-manager=pip&previous-version=23.0.1&new-version=23.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)

Dependabot will resolve any conflicts with this PR as long as you don't
alter it yourself. You can also trigger a rebase manually by commenting
`@dependabot rebase`.

[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)

---

<details>
<summary>Dependabot commands and options</summary>
<br />

You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits
that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after
your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge
and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating
it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop
Dependabot creating any more for this major version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop
Dependabot creating any more for this minor version (unless you reopen
the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop
Dependabot creating any more for this dependency (unless you reopen the
PR or upgrade to it yourself)


</details>

Signed-off-by: dependabot[bot] <support@github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type: enhancement Improvements to functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants