-
Notifications
You must be signed in to change notification settings - Fork 933
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
Change backtracking when packages conflict too much #9843
Conversation
CodSpeed Performance ReportMerging #9843 will degrade performances by 23.19%Comparing Summary
Benchmarks breakdown
|
I'm curious why you decided to choose a threshold for this rather than some other heuristic, such as the nature of the version specifier (e.g. triggering if the conflict was driven by one of In particular, as an end user I would find it surprising that I would get problematic behavior on a new release of a package, and then without any of its dependencies changing it just fixed itself once that package was released 4 more times. |
These pathological behaviors can occur independent of the version specifiers used, tracking conflicts gives us a stronger heuristic. In the sentry example, we have both lower and upper bounds, so we can only avoid the wrong backtracking with conflict tracking. In all cases tested, the problematic behavior was resolved by current heuristic. While we're technically using a SAT solver and each solution that fulfills all constraints is correct to emit, we also have a goal to emit desirable resolutions, where desirable is a vague notion of using newer packages and avoiding old packages. In that sense we're somewhat more like integer linear programming because we're also optimizing for a (vague) target function. (There are approaches for a globally optimal solution to resolution that use a real target function for the most desirable solution, but they are prohibitively expensive.) In one framing, this switching priorities is trying to avoid a branch in which we would get a bad score. For the fastapi and apache-beam cases this is working: We're avoiding the bad solution with the very old fastapi and the apache-beam version that doesn't build anymore.
While i agree that this would be annoying, it's a rare case that needs very specific circumstances. It requires that there are already two possible solutions, one with the older and one with the newer version of the package, that are only decided by prioritization, i.e. we consider both as acceptable solutions. |
Thanks for the explanation, I'm eager to see the results in the real world, i.e. once it's unleashed on to the many users of uv, I think I can adapt this optimization to the pip-resolvelib stack (I have a lot more fixing of basic issues first, but I'm putting it on my to-do list to investigate). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this makes sense to me!
3ff3d9e
to
6b8fe16
Compare
/// Package(s) to be de-prioritized after the next unit propagation | ||
/// | ||
/// Distilled from `culprit` for fast checking in the hot loop. | ||
depriotize: Vec<Id<PubGrubPackage>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to note here is that Id<PubGrubPackage>
is different for proxies, like Package::Extra
and Package::Package
have different IDs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surprisingly, keeping them separate showed better performance than conflict counts over the proxies of a package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent.
Whenever we either discard a version due to its dependencies or perform conflict resolution, we return the last conflict that led to discarding them. In cargo, we use this information for prioritization, which speeds up resolution (`cargo run -r -- -m pub --with-solana --filter solana-archiver-lib -t 16` goes from 90s to 20s on my machine). Configurations that are noticeably slower for the solana test case: * All incompatibilities unit propagation * Only the last root cause in unit propagation * No incompatibilities from unit propagation * No incompatibilities from `add_version` * Only affect counts (without culprit counts) * Backtracking with the same heuristic as astral-sh/uv#9843 (backtracking once after affected hits 5) In uv, we use this to re-prioritize and backtrack when a package decision accumulated to many conflicts. Since we have our own solver loop, we add the incompatibility to our own tracking instead. Built on #291 ## Benchmarks Main: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 1215.49s == 20.26min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 80.58s == 1.34min ``` With #291: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 467.73s == 7.80min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 34.76s == 0.58min ``` This PR: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 271.79s == 4.53min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 20.17s == 0.34min ```
Whenever we either discard a version due to its dependencies or perform conflict resolution, we return the last conflict that led to discarding them. In cargo, we use this information for prioritization, which speeds up resolution (`cargo run -r -- -m pub --with-solana --filter solana-archiver-lib -t 16` goes from 90s to 20s on my machine). Configurations that are noticeably slower for the solana test case: * All incompatibilities unit propagation * Only the last root cause in unit propagation * No incompatibilities from unit propagation * No incompatibilities from `add_version` * Only affect counts (without culprit counts) * Backtracking with the same heuristic as astral-sh/uv#9843 (backtracking once after affected hits 5) In uv, we use this to re-prioritize and backtrack when a package decision accumulated to many conflicts. Since we have our own solver loop, we add the incompatibility to our own tracking instead. Built on pubgrub-rs#291 ## Benchmarks Main: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 1215.49s == 20.26min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 80.58s == 1.34min ``` With pubgrub-rs#291: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 467.73s == 7.80min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 34.76s == 0.58min ``` This PR: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 271.79s == 4.53min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 20.17s == 0.34min ```
This PR is the child of #36 and pubgrub-rs#291, providing an implementation that works for both cargo and uv. Upstream PR: pubgrub-rs#298. Specifically, we use the returned incompatibility in astral-sh/uv#9843, but not `PackageResolutionStatistics`. --- Whenever we either discard a version due to its dependencies or perform conflict resolution, we return the last conflict that led to discarding them. In cargo, we use this information for prioritization, which speeds up resolution (`cargo run -r -- -m pub --with-solana --filter solana-archiver-lib -t 16` goes from 90s to 20s on my machine). Configurations that are noticeably slower for the solana test case: * All incompatibilities unit propagation * Only the last root cause in unit propagation * No incompatibilities from unit propagation * No incompatibilities from `add_version` * Only affect counts (without culprit counts) * Backtracking with the same heuristic as astral-sh/uv#9843 (backtracking once after affected hits 5) In uv, we use this to re-prioritize and backtrack when a package decision accumulated to many conflicts. Since we have our own solver loop, we add the incompatibility to our own tracking instead. Built on pubgrub-rs#291 ## Benchmarks Main: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 1215.49s == 20.26min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 80.58s == 1.34min ``` With pubgrub-rs#291: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 467.73s == 7.80min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 34.76s == 0.58min ``` This PR: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 271.79s == 4.53min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 20.17s == 0.34min ```
Make our implementation compatible with pubgrub-rs/pubgrub#298
4131f45
to
c92bf36
Compare
Packse publishing broke (https://github.com/astral-sh/packse/actions/runs/12349693707/job/34461019415), I'll add the test case later. |
Add the missing test for #9843.
Add the missing test for #9843.
* Return and track affected and culprit on conflicts Whenever we either discard a version due to its dependencies or perform conflict resolution, we return the last conflict that led to discarding them. In cargo, we use this information for prioritization, which speeds up resolution (`cargo run -r -- -m pub --with-solana --filter solana-archiver-lib -t 16` goes from 90s to 20s on my machine). Configurations that are noticeably slower for the solana test case: * All incompatibilities unit propagation * Only the last root cause in unit propagation * No incompatibilities from unit propagation * No incompatibilities from `add_version` * Only affect counts (without culprit counts) * Backtracking with the same heuristic as astral-sh/uv#9843 (backtracking once after affected hits 5) In uv, we use this to re-prioritize and backtrack when a package decision accumulated to many conflicts. Since we have our own solver loop, we add the incompatibility to our own tracking instead. Built on #291 ## Benchmarks Main: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 1215.49s == 20.26min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 80.58s == 1.34min ``` With #291: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 467.73s == 7.80min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 34.76s == 0.58min ``` This PR: ``` index commit hash: 82086e46740d7a9303216bfac093e7268a95121f index commit time: 2024-11-30T18:18:14Z index size: 32 solana in index: 32 Pub CPU time: 271.79s == 4.53min Cargo CPU time: skipped Cargo check lock CPU time: skipped Pub check lock CPU time: skipped Wall time: 20.17s == 0.34min ``` * Use smallvec for root causes * Add more docs * Review
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [astral-sh/uv](https://github.com/astral-sh/uv) | patch | `0.5.8` -> `0.5.11` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>astral-sh/uv (astral-sh/uv)</summary> ### [`v0.5.11`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0511) [Compare Source](astral-sh/uv@0.5.10...0.5.11) ##### Enhancements - Normalize `platform_system` to `sys_platform` ([#​9949](astral-sh/uv#9949)) - Improve retry mechanisms on Windows for `copy_atomic` and `write_atomic` ([#​10026](astral-sh/uv#10026)) - Add nuance to prefetch logging ([#​9984](astral-sh/uv#9984)) - Update to [`python-build-standalone 20241219`](https://github.com/astral-sh/python-build-standalone/releases/tag/20241219) ##### Preview features - Build backend: Preserve executable bits for scripts in distributions ([#​10027](astral-sh/uv#10027)) - Build backend: Handle case where `metadata_directory` already contains `dist-info` directory ([#​10005](astral-sh/uv#10005)) ##### Performance - Batch resolver pre-fetches per fork ([#​10029](astral-sh/uv#10029)) ##### Bug fixes - Allow `--script` to be provided with `uv run -` ([#​10035](astral-sh/uv#10035)) - Allow `uv run` arguments when reading from `stdin` ([#​10034](astral-sh/uv#10034)) - Prefer higher Python lower-bounds when forking ([#​10007](astral-sh/uv#10007)) - Remove references to deprecated `first-match` ([#​10036](astral-sh/uv#10036)) ##### Documentation - Add `uv python install --preview` to the documentation ([#​10010](astral-sh/uv#10010)) - Fix `uv python install --default` note about multiple requests ([#​10011](astral-sh/uv#10011)) - Fix typo in Caching docs ([#​10032](astral-sh/uv#10032)) - Remove remaining references to deprecated `first-match` ([#​10038](astral-sh/uv#10038)) - Supplement missing separators for `UV_INSTALL_DIR` directions on Windows ([#​9507](astral-sh/uv#9507)) ### [`v0.5.10`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#0510) [Compare Source](astral-sh/uv@0.5.9...0.5.10) ##### Enhancements - Improve backtracking behavior when packages conflict repeatedly ([#​9843](astral-sh/uv#9843)) - Patch Python `sysconfig` values such as `AR` at `ar` install time ([#​9905](astral-sh/uv#9905)) - Patch Python `sysconfig` values such as `clang` to `cc` at install time ([#​9916](astral-sh/uv#9916)) - Skip `--native-tls` in `pip compile` header ([#​9913](astral-sh/uv#9913)) - Add resolver error hint for no-binary and no-build failures ([#​9948](astral-sh/uv#9948)) - Improve build error messages ([#​9660](astral-sh/uv#9660)) - Reduce redundant Python version incompatibilities in resolver error message ([#​9957](astral-sh/uv#9957)) - Reduce redundant enumeration of all package versions in some resolver errors ([#​9885](astral-sh/uv#9885)) - Improve display of ranges when pre-releases are not allowed ([#​9944](astral-sh/uv#9944)) - Improve error messages for `uv remove` ([#​9959](astral-sh/uv#9959)) - Improve phrasing for single term incompatibilities ([#​9953](astral-sh/uv#9953)) - Improve styling of `uv remove` dependency hints ([#​9960](astral-sh/uv#9960)) - Omit trailing zeros on Python requirements inferred from versions ([#​9952](astral-sh/uv#9952)) - Show a concise error message for missing `version` field ([#​9912](astral-sh/uv#9912)) - Use the build options value to improve hints for no wheel / source distribution errors ([#​9950](astral-sh/uv#9950)) ##### Bug fixes - Allow multiple disjoint URLs in overrides ([#​9893](astral-sh/uv#9893)) - Include explicit indexes in publish index choice ([#​9932](astral-sh/uv#9932)) - Fix Python interpreter detection for 32-bit operating systems on 64-bit hosts ([#​9970](astral-sh/uv#9970)) ##### Documentation - Fix typo "operation system" ([#​9971](astral-sh/uv#9971)) - Clarify uninstallation docs ([#​9938](astral-sh/uv#9938)) - Add a note to say that dependencies between workspace members are editable ([#​9363](astral-sh/uv#9363)) - Correctly document default value of `fork-strategy` setting ([#​9931](astral-sh/uv#9931)) - Use double quotes for Windows support in examples ([#​9946](astral-sh/uv#9946)) - Remove `pypy` from top-level pin example ([#​9896](astral-sh/uv#9896)) - Update references to `python-build-standalone` to reflect the transferred project ([#​9977](astral-sh/uv#9977)) - Use a different Ruff version in documentation ([#​9943](astral-sh/uv#9943)) - Change example so it works as-is on `powershell` and `cmd.exe` ([#​9903](astral-sh/uv#9903)) - Clarify best practice for Python matrix strategy in GitHub Actions ([#​9454](astral-sh/uv#9454)) - Add documentation for `uv-lock` and `uv-export` pre-commit hooks ([#​9872](astral-sh/uv#9872)) ##### Preview features - Build backend: Fix pre-PEP 639 license files ([#​9965](astral-sh/uv#9965)) ### [`v0.5.9`](https://github.com/astral-sh/uv/blob/HEAD/CHANGELOG.md#059) [Compare Source](astral-sh/uv@0.5.8...0.5.9) ##### Enhancements - Fork version selection based on `requires-python` requirements ([#​9827](astral-sh/uv#9827)) - Patch `sysconfig` data at install time ([#​9857](astral-sh/uv#9857)) - Remove `-isysroot` when patching sysconfig ([#​9860](astral-sh/uv#9860)) ##### Configuration - Introduce a `--fork-strategy` preference mode ([#​9868](astral-sh/uv#9868)) - Add support for `UV_OFFLINE` ([#​9795](astral-sh/uv#9795)) ##### Bug fixes - Avoid `panic!()` when current directory does not exist ([#​9876](astral-sh/uv#9876)) - Avoid reusing interpreter metadata when running under Rosetta ([#​9846](astral-sh/uv#9846)) - Avoid trailing slash when deserializing from lockfile ([#​9848](astral-sh/uv#9848)) - Fix bug in terms when collapsing unavailable versions in resolver errors ([#​9877](astral-sh/uv#9877)) - Fix suggestion to use `uv help python` on invalid install requests ([#​9820](astral-sh/uv#9820)) - Skip root when assessing prefix viability ([#​9823](astral-sh/uv#9823)) - Avoid spurious 'Upgraded tool environment' in `uv tool upgrade` ([#​9870](astral-sh/uv#9870)) ##### Rust API - Upgrade minimum Rust version to 1.83 ([#​9815](astral-sh/uv#9815)) ##### Documentation - Document the `--fork-strategy` setting ([#​9887](astral-sh/uv#9887)) ##### Preview features - Build backend: Allow underscores in entrypoints ([#​9825](astral-sh/uv#9825)) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOS42NC4wIiwidXBkYXRlZEluVmVyIjoiMzkuNzUuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90Il19-->
Background reading: #8157
Companion PR: astral-sh/pubgrub#36
Requires for test coverage: astral-sh/packse#230
When two packages A and B conflict, we have the option to choose a lower version of A, or a lower version of B. Currently, we determine this by the order we saw a package (assuming equal specificity of the requirement): If we saw A before B, we pin A until all versions of B are exhausted. This can lead to undesirable outcomes, from cases where it's just slow (sentry) to others cases without lower bounds where be backtrack to a very old version of B. This old version may fail to build (terminating the resolution), or it's a version so old that it doesn't depend on A (or the shared conflicting package) anymore - but also is too old for the user's application (fastapi). #8157 collects such cases, and the
wrong-backtracking
packse scenario contains a minimized example.We try to solve this by tracking which packages are "A"s, culprits, and "B"s, affected, and manually interfering with project selection and backtracking. Whenever a version we just chose is rejected, we give the current package a counter for being affected, and the package it conflicted with a counter for being a culprit. If a package accumulates more counts than a threshold, we reprioritize: Undecided after the culprits, after the affected, after packages that only have a single version (URLs,
==<version>
). We then ask pubgrub to backtrack just before the culprit. Due to the changed priorities, we now select package B, the affected, instead of package A, the culprit.To do this efficiently, we ask pubgrub for the incompatibility that caused backtracking, or just the last version to be discarded (due to its dependencies). For backtracking, we use the last incompatibility from unit propagation as a heuristic. When a version is discarded because one of its dependencies conflicts with the partial solution, the incompatibility tells us the package in the partial solution that conflicted.
We only backtrack once per package, on the first time it passes the threshold. This prevents backtracking loops in which we make the same decisions over and over again. But we also changed the priority, so that we shouldn't take the same path even after the one time we backtrack (it would defeat the purpose of this change).
There are some parameters that can be tweaked: Currently, the threshold is set to 5, which feels not too eager with so me of the conflicts that we want to tolerate but also changes strategies quickly. The relative order of the new priorities can also be changed, as for each (A, B) pair the priority of B is afterwards lower than that for A. Currently, culprits capture conflict for the whole package, but we could limit that to a specific version. We could discard conflict counters after backtracking instead of keeping them eternally as we do now. Note that we're always taking about pairs (A, B), but in practice we track individual packages, not pairs.
A case that we wouldn't capture is when B is only introduced to the dependency graph after A, but I think that would require cyclical dependency for A and B to conflict? There may also be cases where looking at the last incompatibility is insufficient.
Another example that we can't repair with prioritization is urllib3/boto3/botocore: We actually have to check all the newer versions of boto3 and botocore to identify the version that allows with the older urllib3, no shortcuts allowed.
All examples I tested were cases with two packages where we only had to switch the order, so I've abstracted them into a single packse case.
This PR changes the resolution for certain paths, and there is the risk for regressions.
Fixes #8157
All tested examples improved.
Input fastapi:
Input xarray:
Input sentry: The resolution is identical, but arrived at much faster: main tries 69 versions (sentry-kafka-schemas: 63), PR tries 12 versions (sentry-kafka-schemas: 6; 5 times conflicting, then once the right version).
Input apache-beam