Adding the ability to configure etag timeout as env property

[Shahafgo]

Hello HuggingFace Team!
I noticed that the etag_timeout in the snapshot_download request supports passing customized values but for some reason it was not in the pipeline and tokenizers of transformers and the timeout is set by default to 10 seconds without the ability to override and perhaps in another libraries as well.

This ability is important to me as I want to give the user the ability to configure the etag_timeout and because there is not an options to do so in tokenizer I have thought maybe it is a good idea to override the etag timeout for all the libraries (like in huggingface-cli) that use huggingface_hub.

[LysandreJik]

Hey @Shahafgo, thanks for your report! I think this makes sense!

@Wauplin should we move this report to transformers or would you like to keep it here and think of a library-agnostic way to handle this?

[Wauplin]

Hey there 👋 I think an environment variable makes sense to overwrite the default value here. It feels to me that it's a quite low-level setting that a user would want to set once and for all for all libraries depending on huggingface_hub. We can add support for the etag_timeout parameter in transformers, tokenizers and diffusers but I don't think there is much added value in implementing in each library rather than setting globally (also, even within huggingface_hub, it's not a parameter of huggingface-cli download for instance).

So yeah, definitely in favor of the suggested env variable. I would call it $HF_HUB_ETAG_TIMEOUT. WDYT?
@Shahafgo would you like to open a PR for it? It would require to:

• parse the env value in constants.py and check that it's a positive integer value. Default value should be 10.
• update the parameter to etag_timeout: Optional[float] = None, in file_download.py, _snapshot_download.py and hf_api.py
• use the constant value when etag_timeout is None
• update the documentation for environment variables

[Shahafgo]

Hi @Wauplin , I will do it shortly and I will open a new pr.
I will update the issue once I finish, thank you for the quick response!

[Wauplin]

Great, looking forward for a PR! Thank you for raising the topic!

[Shahafgo]

@Wauplin @LysandreJik Hi team,
As part of my investigation, I can see that there is a timeout both for the head and the get requests of a file.
I think maybe it's a good idea to give the option to control both the timeout params.
In my opinion it's make sense, get requests maybe slow until getting the stream.
What do you think?

[Wauplin]

@Shahafgo those 2 timeouts are indeed pretty different. I think it's already the case the the timeouts have different values (10s for the HEAD and 60s or the GET if I remember correctly). However I would not add another parameter for the GET call. I think it's fine enough with the default value. Here is my reasoning:

The "why etag_timeout is more important to configure" is that in the case the model weights are already cached, we are still doing a HEAD call to the Hub to check the model has not being updated since we cached it. This HEAD call is important to stay up to date but not mandatory: if it fails we default back to the cached files. So reducing the HEAD call timeout would definitely help on slow connections when the files are already cached and we don't want to wait before loading them. On the contrary if the GET request is triggered, we need this call to work. If it doesn't, there is no fallback and the script fails with a TimeoutError. So setting a high value (60s) for the timeout should be good. Also this timeout period is in any case expected to be faster than the download time itself.

What I'm afraid of as well is if we have a $HF_HUB_GET_TIMEOUT one would expect it to be a timeout on any GET requests we make to the Hub (download weights but also listing models, getting model files,...). I would prefer just not introducing it so that we don't have to handle it ^^

What do you think? Would also be interested in @LysandreJik's opinion here in case I missed something with these timeout settings.

[LysandreJik]

Your analysis sounds good to me @Wauplin

[Wauplin]

Ok thanks, then let's keep only a $HF_HUB_ETAG_TIMEOUT as described in #1703 (comment). @Shahafgo please let me know if you want to open a PR for that change :)

[Shahafgo]

@Wauplin @LysandreJik, guys I don׳t know how to thank you for the explanation, I appreciate it.

@Wauplin as a quote:
"
On the contrary if the GET request is triggered, we need this call to work. If it doesn't, there is no fallback and the script fails with a TimeoutError. So setting a high value (60s) for the timeout should be good. Also this timeout period is in any case expected to be faster than the download time itself".

That is why I want to have configurable timout, which user can set higher timeout than 60 seconds if needed.

Also I took a look on the code and it seems that the http_get function has 10 seconds timeout by default.

[Wauplin]

Also I took a look on the code and it seems that the http_get function has 10 seconds timeout by default.

Correct. My apologies, I thought it was way higher than this. Then yes it makes sense for the user to be able to increase this value. What do you think of $HF_HUB_DOWNLOAD_TIMEOUT? That way it would not be confused with the other GET methods in HfApi.

EDIT: for both $HF_HUB_ETAG_TIMEOUT and $HF_HUB_DOWNLOAD_TIMEOUT, let's say that if they are set then they overwrite any value provided by the user. Also requests allow for unlimited timeout with value None. Since we are setting them via environment variable, this is not really possible so let's not support unlimited timeouts.

[Shahafgo]

Ok thank you, I will make the recent wanted changes and I will open the pr.

[Shahafgo]

@Wauplin @LysandreJik This is the pr : #1720
Please let me know if something should be changed or should be added.

[Wauplin]

Reviewed the PR! Thanks for the good job 🎉

[Shahafgo]

@Wauplin Thank you for your review!
I am a bit confused and I would to discuss about it.
First, It seems like http_get uses the as well _request_wrapper.
So according to your suggestion above and at the bottom.
The timeout in http_get should be set according HF_HUB_DOWNLOAD_TIMEOUT
The timeout in _request_wrapper should be set according HF_HUB_ETAG_TIMEOUT
So if both will be set the HF_HUB_ETAG_TIMEOUT in _request_wrapper will override the HF_HUB_DOWNLOAD_TIMEOUT.
Maybe we should use only one variable in _request_wrapper, lets say HF_HUB_REQUEST_TIMEOUT and thats all?

[Wauplin]

First, It seems like http_get uses the as well _request_wrapper.
So according to your suggestion above and at the bottom.
The timeout in http_get should be set according HF_HUB_DOWNLOAD_TIMEOUT
The timeout in _request_wrapper should be set according HF_HUB_ETAG_TIMEOUT
So if both will be set the HF_HUB_ETAG_TIMEOUT in _request_wrapper will override the HF_HUB_DOWNLOAD_TIMEOUT.
Maybe we should use only one variable in _request_wrapper, lets say HF_HUB_REQUEST_TIMEOUT and thats all?

Sorry for the confusion!

What I wanted to say is that for each value (etag timeout and download timeout) we should check the set value against the default value only in 1 place which will be:

• http_get for HF_HUB_DOWNLOAD_TIMEOUT (as done here)
• file_download.hf_hub_download for HF_HUB_ETAG_TIMEOUT (as done here)

And nothing to check in _request_wrapper, that was my mistake!
So it seems all good to me. I'll make a last review and we're good to go :)

[Wauplin]

And... merged! (#1720) 🚀 Nice contribution @Shahafgo 🤗