Skip to content
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

uv pip install fails for dependencies on already-installed local packages #1661

Closed
charlesnicholson opened this issue Feb 18, 2024 · 14 comments · Fixed by #2596
Closed

uv pip install fails for dependencies on already-installed local packages #1661

charlesnicholson opened this issue Feb 18, 2024 · 14 comments · Fixed by #2596
Assignees
Labels
bug Something isn't working

Comments

@charlesnicholson
Copy link

When a package depends on an editable-installed namespaced package, uv pip install fails saying that the already-installed package can't be found. Possibly related to string hygiene and converting ns1.package1 to ns1-package1?

Full cloneable repro case here: https://github.com/charlesnicholson/uv-pep420-bug/tree/main
(Runs at least on macOS and maybe Linux, let me know if it's insufficient and I'll fix it up)

Screenshot 2024-02-18 at 12 58 16 PM
@charlesnicholson
Copy link
Author

Ah, sorry:

❯ ./uv --version
uv 0.1.3

@charlesnicholson
Copy link
Author

Confirmed still happens on 0.1.4 (albeit with a much more soothing color scheme!):

Screenshot 2024-02-18 at 1 43 43 PM

@zanieb zanieb added the bug Something isn't working label Feb 18, 2024
@charliermarsh
Copy link
Member

So, I think cargo run pip install -e ns2.package2 -e ns1.package1 works, but installing the packages one-by-one does not. I need to think about what the right solution is here. When we go to install the second package (-e ns2.package2), we start by performing a resolution, to figure out the required dependencies. We don't consider the "current virtual environment" to be a valid package source, so we have no way of finding ns1-package1.

@charlesnicholson
Copy link
Author

Ah, interesting, thanks for taking a look and sharing your thoughts!

For context, if it's useful, the reason we incrementally pip install -e our various packages in order is because they're part of a larger build system that understands the dependencies between our packages and other tools. Each package has an "editable install" target, and for our build system to satisfy our version of ns2.package2's dependencies, it needs to bring the "editable-install ns1.package1" target up-to-date first, which it does via a separate invocation of pip install -e.

@charlesnicholson
Copy link
Author

charlesnicholson commented Feb 20, 2024

OTOH maybe since uv is so hilariously fast, there isn't really a build-time performance penalty for just always re-editable-installing our dependent packages along with the new package :-D (Transitive package dependencies make this complicated though)

Doing the same work with pip definitely incurs a performance penalty.

@charlesnicholson
Copy link
Author

I am curious, though- the venv seems like a pretty reasonable source of packages given that it holds strongly-versioned already-installed dependencies, and that's more or less its reason to exist. To my mind, it seems reasonable that uv would behave identically with both a unified and multiple separate invocations. I am a brand new user, though, and not the author, so I certainly don't have the context that you do.

@charliermarsh
Copy link
Member

I think it's probably correct for us to fix this. It just changes a lot of assumptions -- right now, we perform a resolution that's independent of the virtual environment.

@charliermarsh charliermarsh changed the title uv pip install fails with PEP420 namespace package dependencies uv pip install fails when packages rely on installed, locally-derived packages Feb 22, 2024
@charliermarsh charliermarsh changed the title uv pip install fails when packages rely on installed, locally-derived packages uv pip install fails for dependencies on already-installed, locally-derived packages Feb 22, 2024
@charliermarsh charliermarsh changed the title uv pip install fails for dependencies on already-installed, locally-derived packages uv pip install fails for dependencies on already-installed local packages Feb 22, 2024
@kkpattern
Copy link

Installing all the editable packages at once works for us.

@charliermarsh
Copy link
Member

Yeah, the problem is that the second install doesn't "know" where to find the existing editable. We need to make the resolver aware of packages that already exist in the virtual environment.

@tylerw
Copy link

tylerw commented Feb 25, 2024

Possibly related (if not, I can file a separate issue—or none at all if this is a misunderstanding on my part), when I uv pip compile a requirements.in or uv pip install a requirements.txt that has local, editable-installed modules (e.g, specified with -e path/to/module) they are all rebuilt and I assume cached, even if they haven't changed and don't need it. Is this avoidable?

For the record, this is a large monorepo with dozens of modules. The commands complete really fast and therefore it's not a huge problem, just wondering if there is a way (or possibly a different work flow) to prevent this.

@charliermarsh
Copy link
Member

@tylerw -- I think we try to avoid rebuilding with uv pip compile, but we always rebuild right now in uv pip install. Building editables is typically pretty cheap, but I'm looking into removing that step.

@charliermarsh
Copy link
Member

(I intend to fix this.)

@zanieb zanieb self-assigned this Mar 21, 2024
zanieb added a commit that referenced this issue Mar 28, 2024
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:

- #1476
- #1856
- #2093
- #2282
- #2383
- #2560

## Test plan

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

zanieb commented Apr 1, 2024

Hi! Charlie tricked me into taking on this hard task instead :)

It should be addressed in the latest release (0.1.27) via #2596.

Let us know if you have any problems.

@charlesnicholson
Copy link
Author

So far it's great, thanks! (I'm integrating it as I type this :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants