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

feat: improved msg for network timeouts #1961

Merged
merged 5 commits into from
Feb 25, 2024

Conversation

samypr100
Copy link
Collaborator

@samypr100 samypr100 commented Feb 25, 2024

Summary

Closes #1922

When a timeout occurs, it hints to the user to configure the UV_HTTP_TIMEOUT env var.

Before

error: Failed to download distributions
  Caused by: Failed to fetch wheel: torch==2.2.0 
  Caused by: Failed to extract source distribution
  Caused by: request or response body error: operation timed out
  Caused by: operation timed out

After

error: Failed to download distributions
  Caused by: Failed to fetch wheel: torch==2.2.0 
  Caused by: Failed to extract source distribution
  Caused by: Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT.

Test Plan

Wasn't sure if we'd want a test. If we do, is there a existing mechanism or preferred approach to force a timeout to occur in tests? Maybe set the timeout to 1 and add torch as an install check (although it's possible that could become flaky)?

@@ -45,6 +45,14 @@ pub struct DistributionDatabase<'a, Context: BuildContext + Send + Sync> {
builder: SourceDistCachedBuilder<'a, Context>,
}

fn handle_response_errors(err: reqwest::Error) -> io::Error {
if err.is_timeout() {
io::Error::new(io::ErrorKind::TimedOut, "Failed to download distribution due to network timeout. Try increasing UV_HTTP_TIMEOUT.")
Copy link
Member

Choose a reason for hiding this comment

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

It looks like Zanie suggested HTTP_TIMEOUT. Do you mind changing to HTTP_TIMEOUT, and displaying the current value too? This is great!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, I decided to use UV_HTTP_TIMEOUT for consitency with registry_client.rs first check (which seems to prefer UV_HTTP_TIMEOUT and HTTP_TIMEOUT last). I could also update that warn_user_once as part of this PR if we want to be fully consistent.

            let timeout = env::var("UV_HTTP_TIMEOUT").or_else(|_| env::var("UV_REQUEST_TIMEOUT")).or_else(|_| env::var("HTTP_TIMEOUT")).and_then(|value| {
                value.parse::<u64>()
                    .or_else(|_| {
                        // On parse error, warn and  use the default timeout
                        warn_user_once!("Ignoring invalid value from environment for UV_REQUEST_TIMEOUT. Expected integer number of seconds, got \"{value}\".");
                        Ok(default_timeout)
                    })
            }).unwrap_or(default_timeout);

Copy link
Member

@zanieb zanieb Feb 25, 2024

Choose a reason for hiding this comment

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

I think UV_HTTP_TIMEOUT is good, it seems better not to recommend changing a value that other clients will respect unless you need to.

Copy link
Member

Choose a reason for hiding this comment

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

Can we say how long the timeout is currently though?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added in 65a5c35 and 783a419. I didn't see reqwest allowing us to look at config.timeout, so I ended up exposing our timeout to the RegistryClient struct instead.

@zanieb
Copy link
Member

zanieb commented Feb 25, 2024

Thanks for contributing!

I'd really like to explore addressing this at its root too, it seems like our timeout is not correctly configured.

Regarding testing, maybe we'd spin up a temporary webserver that just doesn't respond? That feels beyond the scope of this change though. A manual test should suffice for now.

@charliermarsh
Copy link
Member

Thanks!

@charliermarsh charliermarsh enabled auto-merge (squash) February 25, 2024 21:06
@charliermarsh charliermarsh merged commit 757f8e2 into astral-sh:main Feb 25, 2024
7 checks passed
@samypr100 samypr100 deleted the hint-http-timeout branch March 1, 2024 01:54
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.

Display HTTP timeout setting on timeout
3 participants