Skip to content

Use Resolver in pip sync#2696

Merged
charliermarsh merged 1 commit intomainfrom
charlie/finder
Mar 27, 2024
Merged

Use Resolver in pip sync#2696
charliermarsh merged 1 commit intomainfrom
charlie/finder

Conversation

@charliermarsh
Copy link
Copy Markdown
Member

@charliermarsh charliermarsh commented Mar 27, 2024

Summary

This PR removes the custom DistFinder that we use in pip sync. This originally existed because VersionMap wasn't lazy, and so we saved a lot of time in DistFinder by reading distribution data lazily. But now, AFAICT, there's really no benefit. Maintaining DistFinder means we effectively have to maintain two resolvers, and end up fixing bugs in DistFinder that don't exist in the Resolver (like #2688.

Closes #2694.

Closes #2443.

Test Plan

I ran this benchmark a bunch. It's basically a wash. Sometimes one is faster than the other.

❯ python -m scripts.bench \
        --uv-path ./target/release/main \
        --uv-path ./target/release/uv \
        scripts/requirements/compiled/trio.txt --min-runs 50 --benchmark install-warm --warmup 25
Benchmark 1: ./target/release/main (install-warm)
  Time (mean ± σ):      54.0 ms ±  10.6 ms    [User: 8.7 ms, System: 98.1 ms]
  Range (min … max):    45.5 ms …  94.3 ms    50 runs

  Warning: Statistical outliers were detected. Consider re-running this benchmark on a quiet PC without any interferences from other programs. It might help to use the '--warmup' or '--prepare' options.

Benchmark 2: ./target/release/uv (install-warm)
  Time (mean ± σ):      50.7 ms ±   9.2 ms    [User: 8.7 ms, System: 98.6 ms]
  Range (min … max):    44.0 ms …  98.6 ms    50 runs

  Warning: The first benchmarking run for this command was significantly slower than the rest (77.6 ms). This could be caused by (filesystem) caches that were not filled until after the first run. You should consider using the '--warmup' option to fill those caches before the actual benchmark. Alternatively, use the '--prepare' option to clear the caches before each timing run.

Summary
  './target/release/uv (install-warm)' ran
    1.06 ± 0.29 times faster than './target/release/main (install-warm)'

Comment thread crates/uv-dev/src/main.rs
/// cargo run --bin uv-dev -- resolve-many scripts/popular_packages/pypi_10k_most_dependents.txt
/// ```
ResolveMany(ResolveManyArgs),
InstallMany(InstallManyArgs),
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Does anyone care about preserving this? I can migrate it but I wasn't sure if it was worth it. \cc @konstin

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I've been annoyed maintaining it since I don't use it.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It was nice to have but honestly not if its effort to maintain, i don't run it anymore

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Haha. If I had my way I would remove them all.

@charliermarsh charliermarsh marked this pull request as ready for review March 27, 2024 17:24
error: Network connectivity is disabled, but the requested data wasn't found in the cache for: `black`
error: Because black==23.10.1 was not found in the cache and you require black==23.10.1, we can conclude that the requirements are unsatisfiable.

hint: Packages were unavailable because the network was disabled
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is an improvement, I think.

error: markupsafe isn't available locally, but making network requests to registries was banned.
error: Because markupsafe==2.1.3 was not found in the provided package locations and you require markupsafe==2.1.3, we can conclude that the requirements are unsatisfiable.

hint: Packages were unavailable because index lookups were disabled and no additional package locations were provided (try: `--find-links <uri>`)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is an improvement, I think.

Downloaded 1 package in [TIME]
Installed 1 package in [TIME]
+ werkzeug==2.0.0 (from git+https://github.com/pallets/werkzeug.git@2.0.0)
+ werkzeug==2.0.0 (from git+https://github.com/pallets/werkzeug.git@af160e0b6b7ddd81c22f1652c728ff5ac72d5c74)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is an improvement, I think.


----- stderr -----
Removed 2 files for tqdm ([SIZE])
Removed 3 files for tqdm ([SIZE])
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I don't know why this changed. Will look into it.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We have an additional .msgpack file from the cache, because we fetch the Requires-Python for the URL.

///
/// TODO(charlie): This currently passes, but should fail. `sync` does not currently validate the
/// `Requires-Python` constraint for direct URL dependencies. (It _does_ respect `Requires-Python`
/// for registry-based dependencies.)
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This was #2443.

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.

This makes sense to me. Will have conflicts with #2596, we'll need to add support for local site packages to pip sync.

Copy link
Copy Markdown
Member

@BurntSushi BurntSushi left a comment

Choose a reason for hiding this comment

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

Wow.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace custom Finder with Resolver in pip sync pip sync does not enforce Requires-Python requirements for direct URLs

4 participants