Skip to content
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

Token in env variables #522

Closed
wants to merge 13 commits into from
Closed

Token in env variables #522

wants to merge 13 commits into from

Conversation

FrancescoSaverioZuppichini
Copy link
Contributor

@FrancescoSaverioZuppichini FrancescoSaverioZuppichini commented Dec 6, 2021

Why

Sometimes (tests, docker, ... etc) it's easier to pass the token required to use the hub in a env variable, something like:

export HUGGING_FACE_HUB_TOKEN=<YOUR_TOKEN>

This avoids the need of getting the token, creating a folder, and storing it inside

How

I've edited the logic inside hf_api.HFolder.get_token. Pasted below for convenience

    @classmethod
    def get_token(cls) -> Optional[str]:
        """
        Get token or None if not existent. A token can be also provided using env variables: `export HUGGING_FACE_HUB_TOKEN=<YOUR_TOKEN>`.
        """
        token: Optional[str] = os.environ.get("HUGGING_FACE_HUB_TOKEN")
        try:
            with open(cls.path_token, "r") as f:
                token = f.read()
        except FileNotFoundError:
            pass

        return token

Test it

I've added a few new line in the tests but even if I've mocked the os.environ dict the token is still None, I can use a little help 🙏

Thanks

Francesco

@LysandreJik
Copy link
Member

Hey @FrancescoSaverioZuppichini, sorry for getting back so late on this - the issue with this approach is that it won't configure git properly as it won't put the token in the git-credential store, so it's only a partial fix.

Could you let me know of an issue you encountered with docker, so that we may take a look together and see the best resolution?

@FrancescoSaverioZuppichini
Copy link
Contributor Author

Hi @LysandreJik , thank you for your reply. Apologies for the noob question, how is the token placed in the git-credential store?

To use the hub in docker, you have to create the folder and paste the token inside it. Something like:

mkdir <HF_TOKEN_FOLDER>
echo $HF_TOKEN > <HF_TOKEN_FOLDER>/token

This has to be done at build time you also (probably) need to define the token variable with the ARG keyword.

In my opinion, it would be cool to let the code directly set everything up if it sees the token in the correct env variable. What do you think?

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Sorry for taking so long to get back to this. I agree with you regarding the need, thanks for offering a PR! Only left a comment, but once that is taken care of, you can feel free to merge. I'm looking at how to enable that from git's perspective and will get back to you.

Thanks, @FrancescoSaverioZuppichini!

Comment on lines +619 to +654
with mock.patch.dict(os.environ, {"HUGGING_FACE_HUB_TOKEN": token}):
self.assertEqual(HfFolder.get_token(), token)
with mock.patch.dict(os.environ, {"HUGGING_FACE_HUB_TOKEN": None}):
self.assertEqual(HfFolder.get_token(), None)
Copy link
Member

Choose a reason for hiding this comment

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

Cool :)

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
@LysandreJik
Copy link
Member

Actually, before merging, let me take a quick look at how we can handle that from git's side.

@LysandreJik
Copy link
Member

LysandreJik commented Feb 1, 2022

Your proposition of toggling it in the credential store could be done by calling the following method: write_to_credential_store.

I think the environment variable will not change during the runtime, so setting it at the beginning would be the best. Would you like to try your hand at it?

Edit: one issue with this approach is that this will impact the global state of git. Even if the env variable is set once, every subsequent git operation that requires auth on the hf hub will use the info stored into the git credentials. I'm not sure if this is a big deal or not.

@adrinjalali
Copy link
Contributor

I'm not sure if this should have much to do with the git credential storage @LysandreJik . I think it makes sense for people to be able to set the token as an environment variable taken from their secrets, and the code itself wouldn't have anything to do with the token and it would just read the env variable instead.

Am I missing something?

@LysandreJik
Copy link
Member

Thinking more about it, you're correct @adrinjalali. Having it as an environment variable without any impact on the git-credentials logging system sounds good to me.

Apart from the try/except statement, should be good to go. Thanks!

Co-authored-by: Julien Chaumond <julien@huggingface.co>
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
Co-authored-by: Zachary Mueller <muellerzr@gmail.com>
@FrancescoSaverioZuppichini
Copy link
Contributor Author

Tried to rebase it, something didn't work. Gonna open a new PR

This pull request was closed.
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