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

Wait for request stream to flush before returning resolution #2374

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

charliermarsh
Copy link
Member

@charliermarsh charliermarsh commented Mar 12, 2024

Summary

This is a more robust fix for #2300.

The basic issue is:

  • When we resolve, we attempt to pre-fetch the distribution metadata for candidate packages.
  • It's possible that the resolution completes without those pre-fetch responses. (In the linked issue, this was mainly because we were running with --no-deps, but the pre-fetch was causing us to attempt to build a package to get its dependencies. The resolution would then finish before the build completed.)
  • In that case, the Index will be marked as "waiting" for that response -- but it'll never come through.
  • If there's a subsequent call to the Index, to see if we should fetch or are waiting for that response, we'll end up waiting for it forever, since it looks like it's in-flight (but isn't). (In the linked issue, we had to build the source distribution for the install phase of pip install, but setuptools was in this bad state from the resolve phase.)

This PR modifies the resolver to ensure that we flush the stream of requests before returning. Specifically, we now join rather than select between the resolution and request-handling futures.

This could be wasteful, since we don't need those requests, but it at least ensures that every .wait is followed by .done. In practice, I expect this not to have any significant effect on performance, since we end up using the pre-fetched distributions almost every time.

Test Plan

I ran through the test plan from #2373, but ran the build 10 times and ensured it never crashed. (I reverted #2373, since that also fixes the issue in the proximate case, by never fetching setuptools during the resolve phase.)

I also added logging to verify that requests are being handled after the resolution completes, as expected.

I also introduced an arbitrary error in fetch to ensure that the error was immediately propagated.

@charliermarsh charliermarsh added the bug Something isn't working label Mar 12, 2024
@charliermarsh charliermarsh marked this pull request as ready for review March 12, 2024 04:04
@@ -197,42 +196,40 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
let requests_fut = self.fetch(request_stream).fuse();

// Run the solver.
let resolve_fut = self.solve(&request_sink).fuse();
let resolve_fut = self.solve(request_sink).fuse();
Copy link
Member Author

Choose a reason for hiding this comment

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

Taking ownership means that request_sink is dropped as soon as the future completes.

@charliermarsh
Copy link
Member Author

No meaningful change, as expected:

❯ python -m scripts.bench \
        --uv-path ./target/release/uv-main \
        --uv-path ./target/release/uv \
        scripts/requirements/trio.in --benchmark resolve-cold --min-runs 50
Benchmark 1: ./target/release/uv-main (resolve-cold)
  Time (mean ± σ):     245.7 ms ±  48.9 ms    [User: 64.4 ms, System: 62.9 ms]
  Range (min … max):   183.6 ms … 322.8 ms    50 runs

Benchmark 2: ./target/release/uv (resolve-cold)
  Time (mean ± σ):     244.8 ms ±  50.2 ms    [User: 64.5 ms, System: 61.7 ms]
  Range (min … max):   179.9 ms … 326.9 ms    50 runs

Summary
  './target/release/uv (resolve-cold)' ran
    1.00 ± 0.29 times faster than './target/release/uv-main (resolve-cold)'

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

Nice

@@ -27,7 +27,7 @@ pub enum ResolveError {
#[error(transparent)]
Client(#[from] uv_client::Error),

#[error("The channel is closed, was there a panic?")]
#[error("The channel closed unexpectedly")]
Copy link
Member

Choose a reason for hiding this comment

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

I added the panic message to all the broken channel cases because in my experience this error happens when there was a panic in another thread, so the error message you get is ~irrelevant, while scrolling up another thread spilled a first panic to stderr that has the actually relevant failure

@charliermarsh charliermarsh merged commit 79ac3a2 into main Mar 12, 2024
7 checks passed
@charliermarsh charliermarsh deleted the charlie/join branch March 12, 2024 14:13
@charliermarsh
Copy link
Member Author

I also benchmarked on home-assistant to be safe (this branch is "faster" but I think it's just noise):

❯ python -m scripts.bench \
        --uv-path ./target/release/uv-main \
        --uv-path ./target/release/uv \
        scripts/requirements/home-assistant.in --benchmark resolve-cold
Benchmark 1: ./target/release/uv-main (resolve-cold)
  Time (mean ± σ):     14.211 s ±  0.316 s    [User: 66.686 s, System: 29.350 s]
  Range (min … max):   13.691 s … 14.771 s    10 runs

Benchmark 2: ./target/release/uv (resolve-cold)
  Time (mean ± σ):     13.791 s ±  0.291 s    [User: 66.304 s, System: 28.658 s]
  Range (min … max):   13.290 s … 14.202 s    10 runs

Summary
  './target/release/uv (resolve-cold)' ran
    1.03 ± 0.03 times faster than './target/release/uv-main (resolve-cold)'

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 this pull request may close these issues.

4 participants