Skip to content

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

Closed
BenjaminBossan wants to merge 3 commits intoskops-dev:mainfrom
BenjaminBossan:test-hf-hub-0.12rc
Closed

Test huggingface_hub v0.12.0rc0, don't merge!#273
BenjaminBossan wants to merge 3 commits intoskops-dev:mainfrom
BenjaminBossan:test-hf-hub-0.12rc

Conversation

@BenjaminBossan
Copy link
Copy Markdown
Collaborator

Just for testing the release candidate

@BenjaminBossan
Copy link
Copy Markdown
Collaborator Author

@merveenoyan @adrinjalali Please take a look at the changes in this PR to fix errors with hf_hub v0.12. If you're fine with them, I'd create a separate PR (minus the dependency bump).

Copy link
Copy Markdown
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

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

cc @Wauplin on the token name change.


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

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

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

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

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

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

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

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

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants