-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
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. |
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). |
One thought here - maybe |
f6847e5
to
f613141
Compare
This reduces how much pip will attempt to backtrack.
f613141
to
6a0d1f6
Compare
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. |
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.
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.
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.
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. |
Fair. And I'm still not convinced this is something we should expect users to be able to configure meaningfully.
Agreed, that's a reasonable point.
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. |
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: 2
❯ pip 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: 2
❯ pip 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
|
Even though this already got merged I did run some tests over the last day, summary of my findings:
|
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) <https://github.com/pypa/pip/issues/11358></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) <https://github.com/pypa/pip/issues/11451></code>_)</li> <li><code>--no-binary</code> does not disable the cache of locally built wheels anymore. It only means "don't download wheels". (<code>[#11453](pypa/pip#11453) <https://github.com/pypa/pip/issues/11453></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) <https://github.com/pypa/pip/issues/11859></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) <https://github.com/pypa/pip/issues/11915></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) <https://github.com/pypa/pip/issues/8368></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) <https://github.com/pypa/pip/issues/8559></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) <https://github.com/pypa/pip/issues/10476></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) <https://github.com/pypa/pip/issues/10937></code>_)</li> <li>Display dependency chain on each Collecting/Processing log line. (<code>[#11169](pypa/pip#11169) <https://github.com/pypa/pip/issues/11169></code>_)</li> <li>Support a per-requirement <code>--config-settings</code> option in requirements files. (<code>[#11325](pypa/pip#11325) <https://github.com/pypa/pip/issues/11325></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) <https://github.com/pypa/pip/issues/11681></code>_)</li> <li>Add <code>-C</code> as a short version of the <code>--config-settings</code> option. (<code>[#11786](pypa/pip#11786) <https://github.com/pypa/pip/issues/11786></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) <https://github.com/pypa/pip/issues/11908></code>_)</li> <li>Warn if <code>--hash</code> is used on a line without requirement in a requirements file. (<code>[#11935](pypa/pip#11935) <https://github.com/pypa/pip/issues/11935></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) <https://github.com/pypa/pip/issues/11941></code>_)</li> <li>Support wheel cache when using <code>--require-hashes</code>. (<code>[#5037](pypa/pip#5037) <https://github.com/pypa/pip/issues/5037></code>_)</li> <li>Add <code>--keyring-provider</code> flag. See the Authentication page in the documentation for more info. (<code>[#8719](pypa/pip#8719) <https://github.com/pypa/pip/issues/8719></code>_)</li> <li>In the case of virtual environments, configuration files are now also included from the base installation. (<code>[#9752](pypa/pip#9752) <https://github.com/pypa/pip/issues/9752></code>_)</li> </ul> <h2>Bug Fixes</h2> <ul> <li>Fix grammar by changing "A new release of pip available:" to "A new release of pip is available:" in the notice used for indicating that. (<code>[#11529](pypa/pip#11529) <https://github.com/pypa/pip/issues/11529></code>_)</li> <li>Normalize paths before checking if installed scripts are on PATH. (<code>[#11719](pypa/pip#11719) <https://github.com/pypa/pip/issues/11719></code>_)</li> <li>Correct the way to decide if keyring is available. (<code>[#11774](pypa/pip#11774) <https://github.com/pypa/pip/issues/11774></code>_)</li> <li>More consistent resolution backtracking by removing legacy hack related to setuptools resolution (<code>[#11837](pypa/pip#11837) <https://github.com/pypa/pip/issues/11837></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>
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