Consider installed packages during resolution by zanieb · Pull Request #2596 · astral-sh/uv

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

charliermarsh

I think this generally looks good!

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.

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

                    .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.

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

                    .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

+                    && 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

+                {
                    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

Comment thread crates/uv-resolver/src/candidate_selector.rs Outdated

zanieb commented Mar 21, 2024

View reviewed changes

Comment thread crates/uv-resolver/src/candidate_selector.rs Outdated

zanieb commented Mar 21, 2024

View reviewed changes

Comment thread crates/distribution-types/src/prioritized_distribution.rs Outdated

zanieb commented Mar 21, 2024

View reviewed changes

Comment thread crates/uv-resolver/src/resolver/mod.rs Outdated

charliermarsh reviewed Mar 22, 2024

View reviewed changes

Comment thread crates/distribution-types/src/lib.rs Outdated

charliermarsh reviewed Mar 22, 2024

View reviewed changes

Comment thread crates/uv-distribution/src/distribution_database.rs Outdated

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

Comment thread crates/distribution-types/src/resolved.rs Outdated

zanieb commented Mar 25, 2024

View reviewed changes

Comment thread crates/distribution-types/src/prioritized_distribution.rs

zanieb commented Mar 25, 2024

View reviewed changes

Comment thread crates/uv/tests/pip_install.rs Outdated

zanieb commented Mar 25, 2024

View reviewed changes

Comment thread crates/uv/tests/pip_install.rs Outdated

zanieb commented Mar 25, 2024

View reviewed changes

Comment thread crates/uv/tests/pip_install.rs Outdated

charliermarsh reviewed Mar 26, 2024

View reviewed changes

Comment thread crates/distribution-types/src/resolution.rs Outdated

Comment thread crates/distribution-types/src/resolved.rs Outdated

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

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