Clean up utils.hub using the latest from hf_hub #18857
Conversation
|
The documentation is not available anymore as the PR was closed or merged. |
| _ = AutoConfig.from_pretrained(TINY_MODEL, force_download=True) | ||
| mock_tqdm.assert_called() | ||
| enable_progress_bar() | ||
| assert not are_progress_bars_disabled() |
There was a problem hiding this comment.
I did not manage to adapt this test with mock since:
- the tqdm from
huggingface_hub.file_downloadis always called - the calls to tqdm.auto.tqdm are not detected anymore
If someone has a genius idea, happy to hear!
There was a problem hiding this comment.
In huggingface_hub I test that by using the capsys fixture from pytest that catch the output while running the tests. Then you can check if the output stayed quiet or if you have progress bars in it.
You can have a look here for the setup and here for a test where tqdm is enabled.
However from transformers's point of view, this is a job done by an external library (huggingface_hub) so testing it here would be a duplicate of what is already tested. It's would not be wrong to test it but the current test you wrote is good enough I think 👍
There was a problem hiding this comment.
And in general, are transformers's _tqdm_active and _tqdm_cls used anywhere now ?
There was a problem hiding this comment.
No they're not used anywhere now (we do use tqdm in other places, so should switch to the one in logging, but probably for another PR).
src/transformers/utils/hub.py
Outdated
| except ValueError: | ||
| # We try to see if we have a cached version (not up to date): |
There was a problem hiding this comment.
Here to be more precise you can use except LocalEntryNotFoundError.
And since the newly introduced exception is defined as (EntryNotFoundError, FileNotFoundError, ValueError), you will never catch it because you catch EntryNotFoundError line 382 above.
except EntryNotFoundError:
if not _raise_exceptions_for_missing_entries:
return None
if revision is None:
revision = "main"
raise EnvironmentError(
f"{path_or_repo_id} does not appear to have a file named {full_filename}. Checkout "
f"'https://huggingface.co/{path_or_repo_id}/{revision}' for available files."
)In the end, I suggest to use except LocalEntryNotFoundError: instead of except ValueError: and to reorder the except (...).
There was a problem hiding this comment.
Good point! Will adapt.
| _ = AutoConfig.from_pretrained(TINY_MODEL, force_download=True) | ||
| mock_tqdm.assert_called() | ||
| enable_progress_bar() | ||
| assert not are_progress_bars_disabled() |
There was a problem hiding this comment.
In huggingface_hub I test that by using the capsys fixture from pytest that catch the output while running the tests. Then you can check if the output stayed quiet or if you have progress bars in it.
You can have a look here for the setup and here for a test where tqdm is enabled.
However from transformers's point of view, this is a job done by an external library (huggingface_hub) so testing it here would be a duplicate of what is already tested. It's would not be wrong to test it but the current test you wrote is good enough I think 👍
| _ = AutoConfig.from_pretrained(TINY_MODEL, force_download=True) | ||
| mock_tqdm.assert_called() | ||
| enable_progress_bar() | ||
| assert not are_progress_bars_disabled() |
There was a problem hiding this comment.
And in general, are transformers's _tqdm_active and _tqdm_cls used anywhere now ?
* Clean up utils.hub using the latest from hf_hub * Adapt test * Address review comment * Fix test
What does this PR do?
This PR uses the newly released version of
hugginface_hubto clean up a few things introduced in #18438 (points 1 and 2 in the description of this PR). For point 3 (load from cache) there is currently a difference between Transformers'try_to_load_from_cacheand the huggingface_hub's one, so this will a follow-up PR inhuggingface_hub(and then to wait for the next release).