Skip prefetching when --no-deps is specified#2373
Merged
charliermarsh merged 1 commit intomainfrom Mar 12, 2024
Merged
Conversation
Member
Author
|
There is a more complete fix for #2300 that I will put up separately, but this does fix the proximate cause. |
charliermarsh
force-pushed
the
charlie/deps
branch
from
08b20dc to
4553cfa
Compare
charliermarsh
added a commit
that referenced
this pull request
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.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary
When running under
--no-deps, we don't need to pre-fetch, because pre-fetching fetches the distribution metadata. But with--no-deps, we only need the package metadata for the top-level requirements. We never need distribution metadata.Incidentally, this will fix #2300.
Test Plan
cargo test./target/debug/uv pip install --verbose --no-cache-dir --no-deps --reinstall ddtrace==2.6.2 debugpy==1.8.1 ecdsa==0.18.0 editorconfig==0.12.4 --verbosein a Python 3.10 Docker contain repeatedly.