-
Notifications
You must be signed in to change notification settings - Fork 2.1k
CHORE: Ensure PEFT works with huggingface_hub 1.0.0 #2808
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
CHORE: Ensure PEFT works with huggingface_hub 1.0.0 #2808
Conversation
The reset_sessions function is removed but it's also no longer necessary to call it for the purpose we used it. Moreover, the deprecated use_auth_token argument is fully removed now, so everywhere we used to pass it, it is now removed, unless a user passes it explicitly.
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
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.
Thanks for handling this! I'm pre-approving as current code is correct but I'd recommend updating the use_auth_token
logic.
"cache_dir": kwargs.get("cache_dir", None), | ||
"token": kwargs.get("token", None), | ||
} | ||
if use_auth_token := kwargs.get("use_auth_token", None): |
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.
While is this correct, I would highly recommend to deprecate use_auth_token
explicitly and forward it as token
if passed. Something like this:
token = kwargs.get("token", None)
if use_auth_token := kwargs.get("use_auth_token", None) is not None:
warnings.warn("`use_auth_token` is deprecated and will soon be removed. Use `token` instead.")
token = use_auth_token
(warning message to adapt)
They both have the exact same behavior and use_auth_token
has been deprecated for ~2 years in transformers
(and will soon be removed anyway). So good if we can completely get rid of it in HF ecosystem mid-term.
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.
Also by doing so, no need for hf_kwargs
which makes things more readable IMO
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.
So my reasoning for this is to explicitly not handle it for users. If they pass use_auth_token
explicitly, they should have gotten the deprecation message for the last 2 years, as you mentioned. IIUC, should PEFT convert the argument to token
, we would have a split in the HF libraries, with some still supporting use_auth_token
and others not. Therefore, I thought that passing on the argument, even if it means it can cause an error if hfh 1.0 is installed, would be the preferred solution.
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.
Fine for me 👍
@Wauplin I saw another issue, namely that we use peft/tests/regression/test_regression.py Lines 101 to 102 in 815956b
What is the solution here? Can the argument be safely removed if hfh 1.0 is being used? |
Parameter |
The
reset_sessions
function is removed but it's also no longer necessary to call it for the purpose we used it.Moreover, the deprecated
use_auth_token
argument is fully removed now, so everywhere we used to pass it, it is now removed, unless a user passes it explicitly.