Return and track affected and culprit on conflicts #298
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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 ```
CodSpeed Performance ReportMerging #298 will degrade performances by 16.8%Comparing Summary
Benchmarks breakdown
|
konstin
added a commit
to astral-sh/pubgrub
that referenced
this pull request
Dec 16, 2024
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 ```
konstin
added a commit
to astral-sh/uv
that referenced
this pull request
Dec 16, 2024
Make our implementation compatible with pubgrub-rs/pubgrub#298
konstin
added a commit
to astral-sh/uv
that referenced
this pull request
Dec 16, 2024
Make our implementation compatible with pubgrub-rs/pubgrub#298
Eh2406
reviewed
Dec 16, 2024
src/internal/core.rs
Outdated
| pub(crate) fn unit_propagation( | ||
| &mut self, | ||
| package: Id<DP::P>, | ||
| ) -> Result<(), NoSolutionError<DP>> { | ||
| ) -> Result<Vec<(Id<DP::P>, IncompDpId<DP>)>, NoSolutionError<DP>> { | ||
| let mut root_causes = Vec::new(); |
There was a problem hiding this comment.
Nit: like unit_propagation_buffer I bet this Vec ends up expensive to allocate.
There was a problem hiding this comment.
Changed it for a smallvec (it's used a lot less than unit_propagation_buffer)
|
I do like the overall approach and particularly the code flexibility of this version. Unfortunately, I'm not able to reproduce the performance improvements. |
|
What performance numbers do you get? |
|
In my implementation I also get worse performance than #291: Pubgrub CPU time for |
x-hgg-x
reviewed
Dec 18, 2024
| unit_propagation_affected: u32, | ||
| unit_propagation_culprit: u32, | ||
| dependencies_affected: u32, | ||
| dependencies_culprit: u32, |
There was a problem hiding this comment.
dependencies_culprit is never set ?
|
Sorry I didn't get you numbers sooner. Eh2406/pubgrub-crates-benchmark@179f289 Eh2406/pubgrub-crates-benchmark@ec4830d Eh2406/pubgrub-crates-benchmark@65fea31 |
|
There were two big differences between my test of this branch and my branch. Obviously one of them was how the metrics were collected. The other big change was the removal of Eh2406/pubgrub-crates-benchmark@787462f Therefore, I feel comfortable merging this, as long as we follow it up fairly quickly by PR's adding |
Eh2406
approved these changes
Dec 18, 2024
|
In retrospect, the relevant number was "how much does this improve over dev" not "how much does this improve over my PR". Sorry for the distraction. Over dev this is huge! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 the main solver loop, we count those conflicts and pass their count to the user in
prioritize.In cargo, we use this information for prioritization, which speeds up resolution (
cargo run -r -- -m pub --with-solana --filter solana-archiver-lib -t 16goes from 90s to 20s on my machine).Configurations that are noticeably slower for the solana test case:
add_versionIn 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.
Internally, we track more fields that we expose externally. This allows experimenting with different prioritization schemes without exposing potentially breaking changes in the API.
Built on #291
Benchmarks
Main:
With #291:
This PR:
Surprisingly, after adding conflicting-ness to the prioritization in any form, the should cancel count and the wall time lose their correlation, i've seen this effect with the other prioritization schemes i tried, too.
The remaining time seems to be spent mostly in semver(-pubgrub).