Consider installed packages during resolution #2596

zanieb · 2024-03-21T18:47:07Z

Previously, we did not consider installed distributions as candidates while performing resolution. Here, we update the resolver to use installed distributions that satisfy requirements instead of pulling new distributions from the registry.

The implementation details are as follows:

We now provide SitePackages to the CandidateSelector
If an installed distribution satisfies the requirement, we prefer it over remote distributions
We do not want to allow installed distributions in some cases, i.e., upgrade and reinstall
- We address this by introducing an Exclusions type which tracks installed packages to ignore during selection
There's a new ResolvedDist wrapper with Installed(InstalledDist) and Installable(Dist) variants
- This lets us pass already installed distributions throughout the resolver

The user-facing behavior is thoroughly covered in the tests, but briefly:

Installing a package that depends on an already-installed package prefers the local version over the index
Installing a package with a name that matches an already-installed URL package does not reinstall from the index
Reinstalling (--reinstall) a package by name will pull from the index even if an already-installed URL package is present
- To reinstall the URL package, you must specify the URL in the request

Closes #1661

Addresses:

Test plan

Reproduction at charlesnicholson/uv-pep420-bug passes
Unit test for editable package (#1476)
Unit test for previously installed package with empty registry
Unit test for local non-editable package
Unit test for new version available locally but not in registry (#2093)
~~[ ] Unit test for wheel not available in registry but already installed locally (#2282)~~ (seems complicated and not worthwhile)
Unit test for install from URL dependency then with matching version (#2383)
Unit test for install of new package that depends on installed package does not change version (#2560)
Unit test that pip compile does not consider installed packages

crates/uv-resolver/src/candidate_selector.rs

crates/distribution-types/src/prioritized_distribution.rs

crates/uv-resolver/src/resolver/mod.rs

crates/distribution-types/src/lib.rs

crates/uv-distribution/src/distribution_database.rs

crates/distribution-types/src/resolved.rs

crates/distribution-types/src/prioritized_distribution.rs

crates/uv/tests/pip_install.rs

charliermarsh

I think this generally looks good!

crates/distribution-types/src/resolution.rs

crates/distribution-types/src/resolved.rs

crates/uv-resolver/src/resolver/mod.rs

This is driving me a little crazy and is becoming a larger problem in #2596 where I need to move more types (like `Upgrade` and `Reinstall`) into this crate. Anything that's shared across our core resolver, install, and build crates needs to be defined in this crate to avoid cyclic dependencies. We've outgrown it being a single file with some shared traits. There are no behavioral changes here.

crates/distribution-types/src/installed.rs

crates/uv-dispatch/src/lib.rs

crates/uv-resolver/src/resolver/mod.rs

zanieb · 2024-03-27T16:57:24Z

crates/uv-resolver/src/resolver/mod.rs

@@ -613,26 +634,23 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
                    .ok_or(ResolveError::Unregistered)?;
                self.visited.insert(package_name.clone());

+                let empty_version_map = VersionMap::default();


We need to initialize this for lifetimes since version_map is a reference.

Alternatively we could have CandidateSelector.select take an Option<&VersionMap> but I opted for an empty iterable to avoid nesting in select instead. I'd consider revisiting this in the future as well.

I think you can do let empty_version_map; here and then empty_version_map = VersionMap::default() in each branch if you want.

Oh that could be nicer.

Actually idk if that makes sense we're trying to return &VersionMap::default() in the match which is the problem?

## Summary  - Displays missing packages as single-line warnings. - Adds support for `Editable project location` and `Required-by` fields in `pip show`. Part of #2526. # Conflicts: # crates/uv-dispatch/src/lib.rs # crates/uv-resolver/src/resolver/mod.rs # crates/uv-resolver/tests/resolver.rs # crates/uv-traits/src/lib.rs # crates/uv/src/commands/pip_compile.rs # Conflicts: # crates/uv/src/commands/pip_sync.rs

charliermarsh

This generally looks great to me.

crates/uv-dispatch/src/lib.rs

crates/uv-installer/src/plan.rs

crates/uv-resolver/src/candidate_selector.rs

crates/uv-resolver/src/exclusions.rs

crates/uv-resolver/src/version_map.rs

crates/uv-resolver/src/resolver/mod.rs

charliermarsh · 2024-03-27T21:27:46Z

crates/uv-resolver/src/resolver/mod.rs

@@ -613,26 +634,23 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
                    .ok_or(ResolveError::Unregistered)?;
                self.visited.insert(package_name.clone());

+                let empty_version_map = VersionMap::default();


I think you can do let empty_version_map; here and then empty_version_map = VersionMap::default() in each branch if you want.

charliermarsh · 2024-03-27T21:31:03Z

crates/uv-resolver/src/resolver/mod.rs

+                    && self
+                        .installed_packages
+                        .get_packages(package_name)
+                        .is_empty()


I believe so but want to confirm: do you have any tests in which a package is already installed, but doesn't exist on the registry? I.e., exercising true for self.unavailable_packages.get(package_name).is_some() and false for the latter condition.

Yes definitely.

charliermarsh · 2024-03-27T21:31:58Z

crates/uv-resolver/src/resolver/mod.rs

+                        .installed_packages
+                        .get_packages(package_name)
+                        .is_empty()
+                {
                    debug_assert!(
                        false,
                        "Dependencies were requested for a package that is not available"


How do we even get here btw?

Maybe a dumb question since there's a debug_assert! here anyway.

Ah if a version map cannot be retrieved for a package we track the package in unavailable_packages but that's no longer the sole way to have a candidate for a package so this is kind of a lazy way of allowing installed packages dependencies to be retrieved even if they were not available in the index.

# Conflicts: # crates/distribution-types/src/lib.rs # crates/uv-dev/src/install_many.rs # crates/uv-resolver/src/finder.rs # crates/uv-resolver/src/lib.rs # crates/uv-resolver/src/manifest.rs # crates/uv-resolver/tests/resolver.rs # crates/uv/src/commands/pip_compile.rs # crates/uv/src/commands/pip_install.rs # crates/uv/src/commands/pip_sync.rs # crates/uv/src/commands/reporters.rs # crates/uv/tests/pip_sync.rs

…ences

zanieb · 2024-03-28T17:10:16Z

I've addressed all the feedback from the review.

…tion in the resolver (#2779) Addresses panic introduced in #2596 and reported in #2763 (comment) When there are multiple versions of a package available, we remove the existing packages before installing the resolved version to "fix" the environment. We must remove all of the package versions and reinstall because removing _any_ of the package versions could break the others. Since reinstalls require a pull from the remote, this broke a contract between the resolver and the planner which must always agree on which packages should come from the remote. This further demonstrates that we should be constructing the install plan with more concrete knowledge from the resolver (i.e. `ResolvedDist` instead of `Requirement`) to avoid having to manually ensure logic matches. ## Test plan Fails on `main` with panic succeeds on branch ``` uv venv --seed source .venv/bin/activate pip install anyio==3.7.0 --ignore-installed pip install anyio==4.0.0 --ignore-installed cargo run -- pip install anyio black -v ```

zanieb commented Mar 21, 2024

View reviewed changes

crates/uv-resolver/src/candidate_selector.rs Outdated Show resolved Hide resolved

zanieb commented Mar 21, 2024

View reviewed changes

crates/uv-resolver/src/candidate_selector.rs Outdated Show resolved Hide resolved

zanieb commented Mar 21, 2024

View reviewed changes

crates/distribution-types/src/prioritized_distribution.rs Outdated Show resolved Hide resolved

zanieb commented Mar 21, 2024

View reviewed changes

crates/uv-resolver/src/resolver/mod.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Mar 22, 2024

View reviewed changes

crates/distribution-types/src/lib.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Mar 22, 2024

View reviewed changes

crates/uv-distribution/src/distribution_database.rs Outdated Show resolved Hide resolved

zanieb force-pushed the zb/installed-resolve branch 2 times, most recently from 4bd3f16 to dbe5cf9 Compare March 25, 2024 20:43

zanieb commented Mar 25, 2024

View reviewed changes

crates/distribution-types/src/resolved.rs Outdated Show resolved Hide resolved

zanieb commented Mar 25, 2024

View reviewed changes

crates/distribution-types/src/prioritized_distribution.rs Show resolved Hide resolved

zanieb commented Mar 25, 2024

View reviewed changes

crates/uv/tests/pip_install.rs Outdated Show resolved Hide resolved

zanieb commented Mar 25, 2024

View reviewed changes

crates/uv/tests/pip_install.rs Outdated Show resolved Hide resolved

zanieb commented Mar 25, 2024

View reviewed changes

crates/uv/tests/pip_install.rs Outdated Show resolved Hide resolved

charliermarsh reviewed Mar 26, 2024

View reviewed changes

crates/distribution-types/src/resolution.rs Outdated Show resolved Hide resolved

crates/distribution-types/src/resolved.rs Outdated Show resolved Hide resolved

crates/uv-resolver/src/resolver/mod.rs Show resolved Hide resolved

zanieb mentioned this pull request Mar 26, 2024

Rename uv-traits and split into separate modules #2674

Merged

zanieb force-pushed the zb/installed-resolve branch 5 times, most recently from 48e4bf0 to 18f36d6 Compare March 27, 2024 16:02

zanieb marked this pull request as ready for review March 27, 2024 16:38

zanieb commented Mar 27, 2024

View reviewed changes

crates/distribution-types/src/installed.rs Show resolved Hide resolved

zanieb commented Mar 27, 2024

View reviewed changes

crates/uv-dispatch/src/lib.rs Outdated Show resolved Hide resolved

zanieb commented Mar 27, 2024

View reviewed changes

crates/uv-resolver/src/resolver/mod.rs Outdated Show resolved Hide resolved

zanieb commented Mar 27, 2024

View reviewed changes

crates/uv-resolver/src/resolver/mod.rs Show resolved Hide resolved

zanieb commented Mar 27, 2024

View reviewed changes

zanieb force-pushed the zb/installed-resolve branch from e53df4c to d9cf413 Compare March 27, 2024 17:04

zanieb requested a review from charliermarsh March 27, 2024 17:05

zanieb force-pushed the zb/installed-resolve branch from 5bca934 to e2fbf2e Compare March 27, 2024 19:54

Fix test

77caaf8

zanieb mentioned this pull request Mar 27, 2024

Allow prereleases, locals, and URLs in non-editable path requirements #2671

Merged

charliermarsh approved these changes Mar 27, 2024

View reviewed changes

zanieb force-pushed the zb/installed-resolve branch from 5188ca6 to 3815e3e Compare March 27, 2024 23:58

zanieb added 3 commits March 27, 2024 19:10

Add ReadInstalled error variant

a8ba64f

Always prefer local distributions over remote ones — including prefer…

76fe2f4

…ences

Improve commentary

59b58dc

zanieb force-pushed the zb/installed-resolve branch from 9217532 to 59b58dc Compare March 28, 2024 03:21

zanieb added 2 commits March 28, 2024 09:29

Rename local to installed

d0b4705

Implement Default for version map

03fef6c

zanieb added the enhancement New feature or request label Mar 28, 2024

zanieb merged commit e1878c8 into main Mar 28, 2024
31 checks passed

zanieb deleted the zb/installed-resolve branch March 28, 2024 18:49

BrewTestBot mentioned this pull request Apr 1, 2024

uv 0.1.27 Homebrew/homebrew-core#167733

Merged

This was referenced Apr 1, 2024

Panic: Resolution should contain all packages #2763

Closed

Exclude installed distributions with multiple versions from consideration in the resolver #2779

Merged

phalanx-hk mentioned this pull request Apr 11, 2024

feat: use uv instead of pip phalanx-hk/kaggle_pipeline#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider installed packages during resolution #2596

Consider installed packages during resolution #2596

zanieb commented Mar 21, 2024 •

edited

Loading

charliermarsh left a comment

zanieb Mar 27, 2024 •

edited

Loading

zanieb Mar 27, 2024

charliermarsh Mar 27, 2024

zanieb Mar 27, 2024

zanieb Mar 28, 2024

charliermarsh left a comment

charliermarsh Mar 27, 2024

charliermarsh Mar 27, 2024

zanieb Mar 28, 2024

charliermarsh Mar 27, 2024

charliermarsh Mar 28, 2024

zanieb Mar 28, 2024

zanieb commented Mar 28, 2024

Consider installed packages during resolution #2596

Consider installed packages during resolution #2596

Conversation

zanieb commented Mar 21, 2024 • edited Loading

Test plan

charliermarsh left a comment

Choose a reason for hiding this comment

zanieb Mar 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charliermarsh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zanieb commented Mar 28, 2024

zanieb commented Mar 21, 2024 •

edited

Loading

zanieb Mar 27, 2024 •

edited

Loading