-
Notifications
You must be signed in to change notification settings - Fork 27k
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
FIX [Gemma
/ CI
] Make sure our runners have access to the model
#29242
FIX [Gemma
/ CI
] Make sure our runners have access to the model
#29242
Conversation
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.
I think it's fine to just put token as a top module variable. Kind a bit strange to has a class attribute as token
.
Thanks for the quick fix.
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.
Should the token not work as long as it's set with export HF_HUB_READ_TOKEN=$GIT_SECRET
Anyone wants to run the corresponding tests have to do
For CI, it will work fine as the secret is stored on Github Actions. |
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 @Wauplin , after offline talks, seems that we should have something like:
def require_read_token(fn):
token = os.getenv("HF_HUB_READ_TOKEN")
@wraps(fn)
def _inner(*args, **kwargs):
with patch(huggingface_hub.utils._headers, "get_token", return_value=token):
return fn(*args, **kwargs)
return _inner
Users who want to test locally are usually logged in, so if they don't pass a token (False
) it should still be alright
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.
LGTM otherwise
What does this PR do?
As discussed offline cc @ydshieh @sanchit-gandhi @ArthurZucker