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

Consider Retry-After header for delay #2393

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

bmuskalla
Copy link
Contributor

Adds support for Retry-After header to be considered for delay. This header is used to indicate how long the user agent should wait before making a follow-up request.

Implemented as part of the existing backoff strategy as it nicely integrated there instead of adding it as a separate adapter as suggested.

Discussion item

Should we include handling HTTP 429 by default in the backoff strategy? Or should we require callers to explicitly opt-in using retry_on_status_codes? Right now, I decided to opt-in by default as its how HTTP spec suggest to handle this whereas retry_on_status_codes should only be used for the "I know the server and I know that we want to retry in those special cases".

Fixes #2360

@Wauplin
Copy link
Contributor

Wauplin commented Jul 17, 2024

Hi @bmuskalla, thanks for opening this PR! As suggested in #2360 (comment), I would prefer to see the retry mechanism directly built in the requests adapter. Doing so, we would retry on HTTP 429 on every requests made to the Hub, not only when using http_backoff. The backoff mechanism is there to ensure robustness when dealing with 500, 503, etc. errors only in some cases but if a 429 can be retried with an explicit Retry-After, then it's fine to always respect it. Could you move the current logic to ./src/huggingface_hub/utils/_http.py? In theory the mocked tests will stay the same (very nice tests btw!)

Test uses a real http server instead of mocking as the retry handling
is implemented on the connection level and mocking that
became quite messy.
@bmuskalla
Copy link
Contributor Author

@Wauplin Thanks for the kudos on the test, unfortunately, the test had to change quite a bit as mocking out the right parts while keeping the retry logic intact became too brittle. Instead of mocking out parts of the connection pool, I went with an integration-style test, hope that works for you.

@Wauplin
Copy link
Contributor

Wauplin commented Aug 13, 2024

Hey @bmuskalla, thanks for the extensive changes and sorry for the response delay. I dived into the Retry class and it's great to automatically follow Retry-After in a clean way! However, do you know if it'd be possible to have this Retry mechanism only if the header is set but not otherwise. If server returns a 503 without a Retry-After header, I'd prefer to delegate the retry mechanism to the higher level logic depending on the user case (which already exist for file download for instance).

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.

http_backoff does not respect Retry-After headers on 429 requests
2 participants