Skip to content

Test huggingface_hub v0.12.0rc0, don't merge! #273

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.

Sign up for GitHub

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

Closed
BenjaminBossan wants to merge 3 commits into from
Closed

Test huggingface_hub v0.12.0rc0, don't merge! #273

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dc460df
Test huggingface_hub v0.12.0rc0, don't merge!
BenjaminBossan Jan 20, 2023
08913c8
[CI inference] Adjust error message
BenjaminBossan Jan 20, 2023
ecbbd72
[CI inference] 'token' instead of 'use_auth_token'
BenjaminBossan Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion skops/_min_dependencies.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
# "tomli": ("1.1.0", "install", "python_full_version < '3.11.0a7'"),
dependent_packages = {
"scikit-learn": ("0.24", "install", None),
"huggingface_hub": ("0.10.1", "install", None),
"huggingface_hub": ("0.12.0rc0", "install", None),
"tabulate": ("0.8.8", "install", None),
"pytest": (PYTEST_MIN_VERSION, "tests", None),
"pytest-cov": ("2.9.0", "tests", None),
Expand Down
2 changes: 1 addition & 1 deletion skops/hub_utils/_hf_hub.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -672,7 +672,7 @@ def download(
dst.rmdir()

cached_folder = snapshot_download(
repo_id=repo_id, revision=revision, use_auth_token=token, **kwargs
repo_id=repo_id, revision=revision, token=token, **kwargs
Copy link
Copy Markdown
Member

@adrinjalali adrinjalali Jan 20, 2023

Choose a reason for hiding this comment

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

does this make us have to depend on the latest hf lib?

@Wauplin how can we make this compatible with multiple hub versions? it's very annoying that we can't easily keep our dependency to an older version.

Copy link
Copy Markdown
Collaborator Author

@BenjaminBossan BenjaminBossan Jan 20, 2023

Choose a reason for hiding this comment

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

I tested locally (with 0.11.1) and it worked.

Copy link
Copy Markdown

@Wauplin Wauplin Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@adrinjalali You can keep use_auth_token for the time being.

By default and for future integrations, it's best to use token everywhere. But if you want to ensure backward compatibility with previous versions, you can still use use_auth_token. If used, it will be treated as token thanks to the @validate_hf_hub_args validator. The idea is to slowly get rid of every use_auth_token mention over time. It will not throw a warning message (yet) as it is still too widely used.

Copy link
Copy Markdown

@Wauplin Wauplin Jan 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

And yes as @BenjaminBossan wrote, it's not specific to hfh 0.12. The switch has been introduced in hfh 0.11.

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali Jan 20, 2023

Choose a reason for hiding this comment

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

If that's the case we should be able to keep it as is, but our CI fails I guess?

I tested locally (with 0.11.1) and it worked.

But it doesn't work with 0.10, does it?

Copy link
Copy Markdown
Collaborator Author

@BenjaminBossan BenjaminBossan Jan 20, 2023

Choose a reason for hiding this comment

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

The problem is that mypy doesn't understand that use_auth_token works because it's not in the signature (I assume the validate_hf_hub_args decorator does the actual handling). I'll test with 0.10.

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali Jan 20, 2023

Choose a reason for hiding this comment

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

then I guess we can make mypy ignore this line for now.

Copy link
Copy Markdown
Collaborator Author

@BenjaminBossan BenjaminBossan Jan 20, 2023

Choose a reason for hiding this comment

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

Doesn't work with 0.10.1. So we could revert to using use_auth_token and add a # type: ignore I guess.

I wonder why hf_hub uses this unconventional way of dealing with deprecated parameters, using a traditional approach would not confuse mypy.

Copy link
Copy Markdown
Collaborator Author

@BenjaminBossan BenjaminBossan Jan 20, 2023

Choose a reason for hiding this comment

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

I'll create a new PR with the changes.

Copy link
Copy Markdown

@Wauplin Wauplin Jan 20, 2023

Choose a reason for hiding this comment

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

Using token with v0.10 will unfortunately not work no.
About the mypy check, that's true that @validate_hf_hub_args does not change the signature. I'm not sure we can easily do that in Python 😕 So the workarounds I see are either:

  1. put a # type! ignore on snapshot_download calls
  2. bump hfh to have minimal version 0.11
  3. have some logic in skops to pass either token or use_auth_token based on the hfh version (:confused:)

I let you see what you prefer for skops. What's weird is that mypy should have already complained when migrating to hfh 0.11.

)
shutil.copytree(cached_folder, dst)
if not keep_cache:
Expand Down
13 changes: 9 additions & 4 deletions skops/hub_utils/tests/test_hf_hub.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -399,10 +399,15 @@ def test_push_download(
private=True,
)

with pytest.raises(
RepositoryNotFoundError,
match="If the repo is private, make sure you are authenticated.",
):
# TODO: remove 1st message when huggingface_hub < v0.12 is dropped
# message changes in huggingface_hub v0.12, test both
match = (
"If the repo is private, make sure you are authenticated"
"|"
"If you are trying to access a private or gated repo, "
"make sure you are authenticated"
)
with pytest.raises(RepositoryNotFoundError, match=match):
download(repo_id=repo_id, dst="/tmp/test")

with pytest.raises(OSError, match="None-empty dst path already exists!"):
Expand Down