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

Reset _nb_retries if data has been received from the server #1784

Merged
merged 2 commits into from
Oct 31, 2023

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Oct 30, 2023

In #1766, I have introduced a retry mechanism in the download process if a ConnectionError or ReadTimeout error happens. The mechanism is quite simple: retry up to 5 times, wait 1s in between.

This PR modifies this mechanism to reset the number of retries each time new data is received from the server. This is because (IMO) we should not assume before-hand how many retries will be needed to download a huge file on a slow connection. The only thing we want to avoid is to end up in an infinite loop, which cannot be the case here. Worst case is we retry 5 times between each chunk of data (very unlikely 😄)

(Failing tests are unrelated. I'm trying to fix them separately)

@Wauplin Wauplin requested a review from LysandreJik October 30, 2023 09:20
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 30, 2023

The documentation is not available anymore as the PR was closed or merged.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Aha, indeed! Smart. LGTM!

@Wauplin
Copy link
Contributor Author

Wauplin commented Oct 31, 2023

Great, merging now then :)

@Wauplin Wauplin merged commit 7b50c1b into main Oct 31, 2023
11 of 15 checks passed
@Wauplin Wauplin deleted the more-robust-file-download branch October 31, 2023 14:13
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

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.

3 participants