Skip to content

Recursively resolve direct URL references upfront#2684

Merged
charliermarsh merged 2 commits intomainfrom
charlie/recursive-locals
Apr 1, 2024
Merged

Recursively resolve direct URL references upfront#2684
charliermarsh merged 2 commits intomainfrom
charlie/recursive-locals

Conversation

@charliermarsh
Copy link
Copy Markdown
Member

@charliermarsh charliermarsh commented Mar 27, 2024 •
edited
Loading

Summary

This PR would enable us to support transitive URL requirements. The key idea is to leverage the fact that...

  • URL requirements can only come from URL requirements.
  • URL requirements identify a specific version, and so don't require backtracking.

Prior to running the "real" resolver, we recursively resolve any URL requirements, and collect all the known URLs upfront, then pass those to the resolver as "lookahead" requirements. This means the resolver knows upfront that if a given package is included, it must use the provided URL.

Closes #1808.

@charliermarsh charliermarsh force-pushed the charlie/recursive-locals branch from 943ad05 to d1c557e Compare March 27, 2024 03:03
@charliermarsh charliermarsh changed the base branch from main to charlie/pre-local March 27, 2024 03:50
@charliermarsh charliermarsh force-pushed the charlie/recursive-locals branch 2 times, most recently from aa6470e to 5a07fda Compare March 27, 2024 04:03
charliermarsh
charliermarsh commented Mar 27, 2024
View reviewed changes
Comment thread crates/uv-resolver/src/prerelease_mode.rs Outdated
charliermarsh
charliermarsh commented Mar 27, 2024
View reviewed changes
Comment thread crates/uv-requirements/src/lookahead.rs Outdated
@charliermarsh charliermarsh force-pushed the charlie/recursive-locals branch from 5a07fda to 89a18f7 Compare March 27, 2024 04:04
charliermarsh
charliermarsh commented Mar 27, 2024
View reviewed changes
Comment thread crates/uv-requirements/src/lookahead.rs Outdated
@charliermarsh charliermarsh force-pushed the charlie/pre-local branch 2 times, most recently from fd40f0b to d5c5a09 Compare March 27, 2024 22:07
Base automatically changed from charlie/pre-local to main March 27, 2024 22:17
@charliermarsh charliermarsh mentioned this pull request Mar 28, 2024
Merged
@zanieb
Copy link
Copy Markdown
Member

zanieb commented Mar 28, 2024 •
edited
Loading

Sweet! I tested this on an old version of packse which has git dependencies.

❯ cargo run -- pip install git+https://github.com/zanieb/packse@14c55ed460d24cc3e247dd3e09506a8e75e35adcpackse@14c55ed460d24cc3e247dd3e09506a8e75e35adc
 Updated https://github.com/zanieb/packse (14c55ed)                                                                           
 Updated https://github.com/zanieb/devpi (22f71ac)
 Updated https://github.com/zanieb/waitress (d6d764b)                                                                        
Resolved 80 packages in 10.49s
   Built pyramid-chameleon==0.3
   Built waitress @ git+https://github.com/zanieb/waitress@d6d764bcc970e1e50486153588eda8a92cf5b5e4
   Built packse @ git+https://github.com/zanieb/packse@14c55ed460d24cc3e247dd3e09506a8e75e35adc
   Built devpi-server @ git+https://github.com/zanieb/devpi@22f71acb8f08a59a098e7ad434cf388a1193fc24#subdirectory=server
   Built devpi==2.2.0                                                                                                        Downloaded 79 packages in 4.48s
Installed 80 packages in 87ms
...

charliermarsh added a commit that referenced this pull request Mar 28, 2024
@charliermarsh
Recursively allow URL requirements for local dependencies (#2702)
1f31350 
## Summary

This is a trimmed-down version of
#2684 that only applies to local
source trees for now, which enables workspace-like workflows (whereby
local packages can depend on other local packages at arbitrary depth).

Closes #2699.

## Test Plan

Added new tests.

Also cloned this MRE that was shared with me
(https://github.com/timothyjlaurent/uv-poetry-monorepo-mre), and
verified that it was installed without error:

```
❯ cargo run pip install ./uv-poetry-monorepo-mre/app --no-cache
    Finished dev [unoptimized + debuginfo] target(s) in 0.15s
     Running `target/debug/uv pip install ./uv-poetry-monorepo-mre/app --no-cache`
Resolved 4 packages in 1.28s
   Built app @ file:///Users/crmarsh/workspace/uv/uv-poetry-monorepo-mre/app
   Built lib1 @ file:///Users/crmarsh/workspace/uv/uv-poetry-monorepo-mre/lib1
   Built lib2 @ file:///Users/crmarsh/workspace/uv/uv-poetry-monorepo-mre/lib2                                        Downloaded 4 packages in 457ms
Installed 4 packages in 2ms
 + app==0.1.0 (from file:///Users/crmarsh/workspace/uv/uv-poetry-monorepo-mre/app)
 + lib1==0.1.0 (from file:///Users/crmarsh/workspace/uv/uv-poetry-monorepo-mre/lib1)
 + lib2==0.1.0 (from file:///Users/crmarsh/workspace/uv/uv-poetry-monorepo-mre/lib2)
 + ruff==0.3.4
```
@charliermarsh charliermarsh mentioned this pull request Mar 28, 2024
Closed
@charliermarsh charliermarsh force-pushed the charlie/recursive-locals branch 8 times, most recently from 26bfa87 to c62c6cc Compare April 1, 2024 15:22
@charliermarsh charliermarsh marked this pull request as ready for review April 1, 2024 15:37
@charliermarsh charliermarsh added the enhancement New feature or improvement to existing functionality label Apr 1, 2024
@charliermarsh
Copy link
Copy Markdown
Member Author

charliermarsh commented Apr 1, 2024

This PR lifts a significant limitation (transitive URL dependencies) at the cost of some parallelism (we have to build and resolve them all upfront).

@charliermarsh charliermarsh requested a review from zanieb April 1, 2024 16:00
@zanieb
Copy link
Copy Markdown
Member

zanieb commented Apr 1, 2024

It's only going to be a performance degradation for cases where the user previously had to enumerate all the URLs, correct?

zanieb
zanieb approved these changes Apr 1, 2024
View reviewed changes
Copy link
Copy Markdown
Member

@zanieb zanieb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sweet

@charliermarsh
Copy link
Copy Markdown
Member Author

charliermarsh commented Apr 1, 2024

That seems fair, yes. There are two issues:

  1. In general, the use of the lookahead resolver is slower than the theoretically-optimal" behavior of resolving as we go within the resolver, because (e.g.) we're spending time resolving source distributions when we could (in parallel) be fetching metadata from PyPI for other dependencies... So we're probably not saturating the network. By doing more work in the lookahead resolver, we make that a bit worse.
  2. Now, we're resolving at multiple levels rather than requiring users to enumerate all the URL requirements upfront, so what was previously parallel is now sequential (which is more directly-related to your comment).

@zanieb
Copy link
Copy Markdown
Member

zanieb commented Apr 1, 2024

Did you do any benches? Worth it? The behavior seems worth enabling regardless.

@charliermarsh
Copy link
Copy Markdown
Member Author

charliermarsh commented Apr 1, 2024

Yeah my guess is that it's pretty rare to have nested URL dependencies anyway, and in the cases that you do, users would definitely prefer that they work even if there's a performance hit.

@charliermarsh
Copy link
Copy Markdown
Member Author

charliermarsh commented Apr 1, 2024

I think I need to update docs before merging though.

@charliermarsh charliermarsh force-pushed the charlie/recursive-locals branch from c62c6cc to be3735c Compare April 1, 2024 20:52
@charliermarsh charliermarsh force-pushed the charlie/recursive-locals branch from be3735c to 3b2728a Compare April 1, 2024 21:07
@charliermarsh charliermarsh enabled auto-merge (squash) April 1, 2024 21:14
@charliermarsh charliermarsh merged commit a48bcae into main Apr 1, 2024
@charliermarsh charliermarsh deleted the charlie/recursive-locals branch April 1, 2024 21:16
@BrewTestBot BrewTestBot mentioned this pull request Apr 2, 2024
Merged
@ringohoffman ringohoffman mentioned this pull request Jun 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@zanieb zanieb zanieb approved these changes

Assignees

No one assigned

Labels

enhancement New feature or improvement to existing functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support URL requirements in transitive dependencies

2 participants

@charliermarsh @zanieb