From fda378fd29655febfbc8264a8402b381d9db0f36 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 19 Apr 2024 19:30:08 -0400 Subject: [PATCH] Avoid preferring constrained over unconstrained packages (#3148) ## Summary pip prefers somewhat-constrained over unconstrained packages... but only if they're at equal depths in the tree. We don't have a way to track the latter property yet (I've added a TODO), so for now, we should remove this constraint -- it seems to be counter-productive. I've filed https://github.com/astral-sh/uv/issues/3149 as a follow-up. Closes https://github.com/astral-sh/uv/issues/3143 ## Test Plan - `git clone https://github.com/drivendataorg/zamba.git` - `cat "-e .[tests]" > req.in` - `cargo run venv && cargo run pip compile req.in --refresh -n --python-platform linux --python-version 3.8` --- crates/uv-resolver/src/pubgrub/priority.rs | 22 +++++------- crates/uv/tests/pip_install_scenarios.rs | 42 ++++++---------------- 2 files changed, 18 insertions(+), 46 deletions(-) diff --git a/crates/uv-resolver/src/pubgrub/priority.rs b/crates/uv-resolver/src/pubgrub/priority.rs index 3f4753bd7..388e35a27 100644 --- a/crates/uv-resolver/src/pubgrub/priority.rs +++ b/crates/uv-resolver/src/pubgrub/priority.rs @@ -33,9 +33,8 @@ impl PubGrubPriorities { std::collections::hash_map::Entry::Occupied(mut entry) => { // Preserve the original index. let index = match entry.get() { + PubGrubPriority::Unspecified(Reverse(index)) => *index, PubGrubPriority::Singleton(Reverse(index)) => *index, - PubGrubPriority::Unconstrained(Reverse(index)) => *index, - PubGrubPriority::Constrained(Reverse(index)) => *index, PubGrubPriority::DirectUrl(Reverse(index)) => *index, PubGrubPriority::Root => next, }; @@ -43,10 +42,8 @@ impl PubGrubPriorities { // Compute the priority. let priority = if version.as_singleton().is_some() { PubGrubPriority::Singleton(Reverse(index)) - } else if version == &Range::full() { - PubGrubPriority::Unconstrained(Reverse(index)) } else { - PubGrubPriority::Constrained(Reverse(index)) + PubGrubPriority::Unspecified(Reverse(index)) }; // Take the maximum of the new and existing priorities. @@ -58,10 +55,8 @@ impl PubGrubPriorities { // Insert the priority. entry.insert(if version.as_singleton().is_some() { PubGrubPriority::Singleton(Reverse(next)) - } else if version == &Range::full() { - PubGrubPriority::Unconstrained(Reverse(next)) } else { - PubGrubPriority::Constrained(Reverse(next)) + PubGrubPriority::Unspecified(Reverse(next)) }); } } @@ -71,9 +66,8 @@ impl PubGrubPriorities { std::collections::hash_map::Entry::Occupied(mut entry) => { // Preserve the original index. let index = match entry.get() { + PubGrubPriority::Unspecified(Reverse(index)) => *index, PubGrubPriority::Singleton(Reverse(index)) => *index, - PubGrubPriority::Unconstrained(Reverse(index)) => *index, - PubGrubPriority::Constrained(Reverse(index)) => *index, PubGrubPriority::DirectUrl(Reverse(index)) => *index, PubGrubPriority::Root => next, }; @@ -112,10 +106,10 @@ pub(crate) enum PubGrubPriority { /// /// As such, its priority is based on the order in which the packages were added (FIFO), such /// that the first package we visit is prioritized over subsequent packages. - Unconstrained(Reverse), - - /// The version range is constrained in some way (e.g., with a `<=` or `>` operator). - Constrained(Reverse), + /// + /// TODO(charlie): Prefer constrained over unconstrained packages, if they're at the same depth + /// in the dependency graph. + Unspecified(Reverse), /// The version range is constrained to a single version (e.g., with the `==` operator). Singleton(Reverse), diff --git a/crates/uv/tests/pip_install_scenarios.rs b/crates/uv/tests/pip_install_scenarios.rs index 2d1ac24e4..ea9cd3f97 100644 --- a/crates/uv/tests/pip_install_scenarios.rs +++ b/crates/uv/tests/pip_install_scenarios.rs @@ -460,14 +460,7 @@ fn dependency_excludes_range_of_compatible_versions() { ╰─▶ Because package-a==1.0.0 depends on package-b==1.0.0 and only the following versions of package-a are available: package-a==1.0.0 package-a>=2.0.0,<=3.0.0 - we can conclude that package-a<2.0.0 depends on package-b==1.0.0. - And because package-a==3.0.0 depends on package-b==3.0.0, we can conclude that any of: - package-a<2.0.0 - package-a>=3.0.0 - depends on one of: - package-b<=1.0.0 - package-b>=3.0.0 - (1) + we can conclude that package-a<2.0.0 depends on package-b==1.0.0. (1) Because only the following versions of package-c are available: package-c==1.0.0 @@ -477,13 +470,8 @@ fn dependency_excludes_range_of_compatible_versions() { package-a<2.0.0 package-a>=3.0.0 - And because we know from (1) that any of: - package-a<2.0.0 - package-a>=3.0.0 - depends on one of: - package-b<=1.0.0 - package-b>=3.0.0 - we can conclude that all versions of package-c depend on one of: + And because we know from (1) that package-a<2.0.0 depends on package-b==1.0.0, we can conclude that package-a!=3.0.0, all versions of package-c, package-b!=1.0.0 are incompatible. + And because package-a==3.0.0 depends on package-b==3.0.0, we can conclude that all versions of package-c depend on one of: package-b<=1.0.0 package-b>=3.0.0 @@ -597,14 +585,7 @@ fn dependency_excludes_non_contiguous_range_of_compatible_versions() { ╰─▶ Because package-a==1.0.0 depends on package-b==1.0.0 and only the following versions of package-a are available: package-a==1.0.0 package-a>=2.0.0,<=3.0.0 - we can conclude that package-a<2.0.0 depends on package-b==1.0.0. - And because package-a==3.0.0 depends on package-b==3.0.0, we can conclude that any of: - package-a<2.0.0 - package-a>=3.0.0 - depends on one of: - package-b<=1.0.0 - package-b>=3.0.0 - (1) + we can conclude that package-a<2.0.0 depends on package-b==1.0.0. (1) Because only the following versions of package-c are available: package-c==1.0.0 @@ -614,13 +595,8 @@ fn dependency_excludes_non_contiguous_range_of_compatible_versions() { package-a<2.0.0 package-a>=3.0.0 - And because we know from (1) that any of: - package-a<2.0.0 - package-a>=3.0.0 - depends on one of: - package-b<=1.0.0 - package-b>=3.0.0 - we can conclude that all versions of package-c depend on one of: + And because we know from (1) that package-a<2.0.0 depends on package-b==1.0.0, we can conclude that all versions of package-c, package-a!=3.0.0, package-b!=1.0.0 are incompatible. + And because package-a==3.0.0 depends on package-b==3.0.0, we can conclude that all versions of package-c depend on one of: package-b<=1.0.0 package-b>=3.0.0 @@ -3355,8 +3331,10 @@ fn transitive_prerelease_and_stable_dependency_many_versions() { ----- stderr ----- × No solution found when resolving dependencies: - ╰─▶ Because only package-c<2.0.0b1 is available and package-a==1.0.0 depends on package-c>=2.0.0b1, we can conclude that package-a==1.0.0 cannot be used. - And because only package-a==1.0.0 is available and you require package-a, we can conclude that the requirements are unsatisfiable. + ╰─▶ Because only package-a==1.0.0 is available and package-a==1.0.0 depends on package-c>=2.0.0b1, we can conclude that all versions of package-a depend on package-c>=2.0.0b1. + And because only package-c<2.0.0b1 is available, we can conclude that all versions of package-a depend on package-c>3.0.0. + And because package-b==1.0.0 depends on package-c and only package-b==1.0.0 is available, we can conclude that all versions of package-b and all versions of package-a are incompatible. + And because you require package-a and you require package-b, we can conclude that the requirements are unsatisfiable. hint: package-c was requested with a pre-release marker (e.g., package-c>=2.0.0b1), but pre-releases weren't enabled (try: `--prerelease=allow`) "###);