Ensure authentication is passed from the index url to distribution files #1886
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Url::parse(&file.url) | ||
| .map_err(|err| FileConversionError::Url(file.url.clone(), err))?, | ||
| ); | ||
| FileLocation::AbsoluteUrl(url.to_string()) |
There was a problem hiding this comment.
I think we intentionally changed this to use a string rather than a URL because we saw a huge regression due to excessive URL parsing, serialization, and deserialization. We should at least benchmark this since we're now doing an extra URL parse and serialization each time.
| @@ -51,7 +53,12 @@ impl File { | |||
| size: file.size, | |||
| upload_time_utc_ms: file.upload_time.map(|dt| dt.timestamp_millis()), | |||
| url: if file.url.contains("://") { | |||
| FileLocation::AbsoluteUrl(file.url) | |||
| let url = safe_copy_url_auth( | |||
There was a problem hiding this comment.
Is it possible to skip this work if there's no username or password on the base?
There was a problem hiding this comment.
Yeah we should be able to avoid it in that case
There was a problem hiding this comment.
Skipped in eb1d6a6
No changes to integration benchmarks
|
Benches show no change $ python -m scripts.bench \
--uv-path ./target/release/uv \
--uv-path ./target/release/baseline \
./scripts/requirements/trio.in --benchmark resolve-warm
Benchmark 1: ./target/release/uv (resolve-warm)
Time (mean ± σ): 144.8 ms ± 1.5 ms [User: 95.8 ms, System: 37.8 ms]
Range (min … max): 143.4 ms … 149.8 ms 20 runs
Benchmark 2: ./target/release/baseline (resolve-warm)
Time (mean ± σ): 145.0 ms ± 2.6 ms [User: 95.9 ms, System: 38.1 ms]
Range (min … max): 142.0 ms … 151.3 ms 20 runs
Summary
./target/release/uv (resolve-warm) ran
1.00 ± 0.02 times faster than ./target/release/baseline (resolve-warm)
$ python -m scripts.bench \
--uv-path ./target/release/uv \
--uv-path ./target/release/baseline \
./scripts/requirements/home-assistant.in --benchmark resolve-warm
Benchmark 1: ./target/release/uv (resolve-warm)
Time (mean ± σ): 302.7 ms ± 10.8 ms [User: 233.3 ms, System: 326.0 ms]
Range (min … max): 292.1 ms … 322.9 ms 10 runs
Benchmark 2: ./target/release/baseline (resolve-warm)
Time (mean ± σ): 295.1 ms ± 7.7 ms [User: 234.0 ms, System: 336.4 ms]
Range (min … max): 285.1 ms … 312.8 ms 10 runs
Summary
./target/release/baseline (resolve-warm) ran
1.03 ± 0.05 times faster than ./target/release/uv (resolve-warm)
$ python -m scripts.bench \
--uv-path ./target/release/uv \
--uv-path ./target/release/baseline \
./scripts/requirements/all-kinds.in --benchmark resolve-warm
Benchmark 1: ./target/release/uv (resolve-warm)
Time (mean ± σ): 798.0 ms ± 24.9 ms [User: 306.8 ms, System: 81.8 ms]
Range (min … max): 776.3 ms … 859.3 ms 10 runs
Benchmark 2: ./target/release/baseline (resolve-warm)
Time (mean ± σ): 807.3 ms ± 55.0 ms [User: 305.5 ms, System: 84.0 ms]
Range (min … max): 744.5 ms … 941.5 ms 10 runs
Summary
./target/release/uv (resolve-warm) ran
1.01 ± 0.08 times faster than ./target/release/baseline (resolve-warm)
$ python -m scripts.bench \
--uv-path ./target/release/uv \
--uv-path ./target/release/baseline \
./scripts/requirements/all-kinds.in --benchmark resolve-warm
Benchmark 1: ./target/release/uv (resolve-warm)
Time (mean ± σ): 798.0 ms ± 24.9 ms [User: 306.8 ms, System: 81.8 ms]
Range (min … max): 776.3 ms … 859.3 ms 10 runs
Benchmark 2: ./target/release/baseline (resolve-warm)
Time (mean ± σ): 807.3 ms ± 55.0 ms [User: 305.5 ms, System: 84.0 ms]
Range (min … max): 744.5 ms … 941.5 ms 10 runs
Summary
./target/release/uv (resolve-warm) ran
1.01 ± 0.08 times faster than ./target/release/baseline (resolve-warm)
$ python -m scripts.bench \
--uv-path ./target/release/uv \
--uv-path ./target/release/baseline \
./scripts/requirements/all-kinds.in --benchmark resolve-cold
Benchmark 1: ./target/release/uv (resolve-cold)
Time (mean ± σ): 1.472 s ± 0.033 s [User: 1.107 s, System: 0.572 s]
Range (min … max): 1.427 s … 1.526 s 10 runs
Benchmark 2: ./target/release/baseline (resolve-cold)
Time (mean ± σ): 1.492 s ± 0.124 s [User: 1.075 s, System: 0.578 s]
Range (min … max): 1.395 s … 1.838 s 10 runs
Summary
./target/release/uv (resolve-cold) ran
1.01 ± 0.09 times faster than ./target/release/baseline (resolve-cold) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #1709
Closes #1371
Tested with the reproduction provided in #1709 which gets past the HTTP 401.
Reuses the same copying logic we introduced in #1874 to ensure authentication is attached to file URLs with a realm that matches that of the index. I had to move the authentication logic into a new crate so it could be used in
distribution-types.We will want to something more robust in the future, like track all realms with authentication in a central store and perform lookups there. That's what
pipdoes and it allows consolidation of logic like netrc lookups. That refactor feels significant though, and I'd like to get this fixed ASAP so this is a minimal fix.