Skip to content

add retries on body read errors during download#331

Merged
assafvayner merged 3 commits intomainfrom
assaf/retries-download-stream-off-main
May 15, 2025
Merged

add retries on body read errors during download#331
assafvayner merged 3 commits intomainfrom
assaf/retries-download-stream-off-main

Conversation

@assafvayner
Copy link
Contributor

fix XET-567

This PR adds the retry mechanism on downloading a range from CF/S3 for when an error occurs reading or parsing the body.
The upload is handled by reverting a change to reintroduce retries on a clonable body PR #328.


impl tokio_retry::Condition<CasClientError> for ChunkRangeDeserializeFromBytesStreamRetryCondition {
fn should_retry(&mut self, err: &CasClientError) -> bool {
// we only care about retrying some error yielded by trying to deserialize the stream
Copy link
Collaborator

Choose a reason for hiding this comment

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

This can't handle this case: huggingface/huggingface_hub#3036 (comment), which comes from send()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that should be handled by the retry middleware on the client

Copy link
Collaborator

@seanses seanses May 15, 2025

Choose a reason for hiding this comment

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

Hmm.. The message says kind: Request and the implementation on Request kind is only to retry on hyper_error.is_incomplete_message() || hyper_error.is_canceled() or if the underlying I/O error is std::io::ErrorKind::ConnectionReset | std::io::ErrorKind::ConnectionAborted, but not including hyper::Error(ChannelClosed)

Copy link
Contributor

@jgodlew jgodlew left a comment

Choose a reason for hiding this comment

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

LGTM!

@assafvayner assafvayner merged commit 2bc433f into main May 15, 2025
4 checks passed
@assafvayner assafvayner deleted the assaf/retries-download-stream-off-main branch May 15, 2025 22:29
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