-
Notifications
You must be signed in to change notification settings - Fork 570
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
MNT deprecate imports from snapshot_download #880
Conversation
The documentation is not available anymore as the PR was closed or merged. |
It seems |
That looks very nice |
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.
Looks great, thanks for working on it @adrinjalali! Only left a nit regarding testing.
tests/test_snapshot_download.py
Outdated
|
||
def test_snapshot_download_import(): | ||
with pytest.warns(FutureWarning, match="has been made private"): | ||
from huggingface_hub.snapshot_download import snapshot_download # noqa |
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.
Could we also check that the method is the same as the one imported from from huggingface_hub import snapshot_download
?
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.
Done
Perfect, thank you @adrinjalali! |
In #874 we had to rename the
snapshot_download.py
file which makesfrom huggingface_hub.snapshot_download import blah
fail.This PR deprecated such imports and warns the user instead of failing.
Ref: scientific-python/lazy-loader#9
cc @LysandreJik @osanseviero
P.S.: I won't be available to apply suggestions here this week, but I think we should have this in the release. If you think anything needs to be changed in this PR, please apply the suggestions and then merge :)