-
Notifications
You must be signed in to change notification settings - Fork 470
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
Add HF_HUB_OFFLINE env var #22
Conversation
CI error looks transient |
|
||
def test_offline_with_timeout(): | ||
with offline(OfflineSimulationMode.CONNECTION_TIMES_OUT): | ||
with pytest.raises(RequestWouldHangIndefinitelyError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe use with self.assertRaises
to be consistent with other tests here @lhoestq (not a big deal if we want to stay consistent with other implems in our librairies)
timeout: float = 10.0, | ||
**params, | ||
) -> requests.Response: | ||
"""Wrapper around requests to retry in case it fails with a ConnectTimeout, with exponential backoff. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _request_with_retry( | ||
method: str, | ||
url: str, | ||
max_retries: int = 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default should maybe be finite no? like 5?
src/huggingface_hub/file_download.py
Outdated
if isinstance(exc, requests.exceptions.SSLError) or isinstance( | ||
exc, requests.exceptions.ProxyError | ||
): | ||
if isinstance(exc, requests.exceptions.ProxyError): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this intended to remove this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes otherwise the test with the bogus url test fails (raises SSLError while ValueError is expected)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one remark is that in transformers we should probably document HF_HUB_OFFLINE
instead of TRANSFORMERS_OFFLINE
given that we want to switch to huggingface_hub there in the codebase. Thoughts?
also related to huggingface/transformers#10235 @lhoestq |
logger.info( | ||
f"{method} request to {url} timed out, retrying... [{tries/max_retries}]" | ||
) | ||
sleep_time = max( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be min
?
This adds the
HF_HUB_OFFLINE
env var to tellhuggingface_hub
(same as in hf/transformers and hf/datasets).This allows to tell the library to look directly at the cache instead of trying network calls.
Moreover I fixed/changed a few things:
force_download=True
doesn't check the cache if there's no internet connection, but raises an error instead.offline
that you can use to test the library's behavior under different offline modes (same as in hf/datasets)I'm not sure why the CI fails for python 3.9 though