Fix FutureWarnings for huggingface_hub>=0.10 #152
Conversation
For that Hub version, the use of the "token" argument produces a FutureWarning, leading our CI to fail. This PR fixes the warnings while leaving everything the same on the user side (i.e. they don't need to change the "token" argument). Implementation As discussed, we will not change the argument name on the skops, preferring token to use_auth_token. For the version checking, I went with the private Version class from scipy, which follows PEP440. This follows the approach taken in skorch (see this issue: skorch-dev/skorch#887), though we import it here instead of vendoring. Let me know if another approach is preferred.
- Since version is currently RC, it is not in fact >=0.10 - There is a somewhat confusing mixture of token and use_auth_token
|
@skops-dev/maintainers ready for review. Until this or another fix is merged, all our CI runs will fail, so this PR should have priority over the others. CI only checks |
| # TODO: remove once we drop support for Hub<0.10 | ||
| if Version(huggingface_hub.__version__) <= Version("0.9.1"): | ||
| extra_kwargs = {"token": token} | ||
| else: | ||
| extra_kwargs = {"use_auth_token": token} |
There was a problem hiding this comment.
rather have this in fixes.py as well.
There was a problem hiding this comment.
and we don't need to check for version, we try with use_auth_token, if it fails we do token.
There was a problem hiding this comment.
Hmm, I understand that try...except allows us to skirt having version checking code. But having a bunch of
try:
client.foo(repo_id=repo_id, repo_type="model", token=token)
except TypeError:
client.foo(repo_id=repo_id, repo_type="model", use_auth_token=token)looks suboptimal to me. It litters our code in multiple places, instead of dealing with the issue once. And, just as a stupid example, what if Hub re-introduces the token argument but with a different functionality, it could lead to unwanted side-effects.
There was a problem hiding this comment.
We can have a Client class in fixes.py which does this, and import that instead, and that class exposes the methods we need in skops, and handles different versions there. The rest of the codebase would stay clean this way.
There was a problem hiding this comment.
That might be a solution, though a bit of a bigger undertaking with some open questions:
- private or public?
- automatically expose all methods or only those we use?
- handle versions with try...expect or version checks?
- also handle inference api?
- testing will probably not be quite trivial
As a quick fix for this issue here, we could also pin the huggingface_hub version and close this PR. WDYT?
There was a problem hiding this comment.
Since we don't have many users yet, I'm also okay with closing this, and then moving along with using modelcards from the new release, which would mean depending on the latest one anyway, and we won't have this issue. So happy to close (I'd leave that to you).
Agree. We also merged #158 for now, so this PR is not strictly necessary. |
For that Hub version, the use of the
tokenargument produces aFutureWarning, leading our CI to fail. This PR fixes the warnings while leaving everything the same on the user side (i.e. they don't need to change thetokenargument).Implementation
As discussed, we will not change the argument name on the skops, preferring token to
use_auth_token.For the version checking, I went with the private
Versionclass from scipy, which follows PEP440. This follows the approach taken in skorch (see this issue: skorch-dev/skorch#887), though we import it here instead of vendoring. Let me know if another approach is preferred.