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

ENH Use provided token from HUGGING_FACE_HUB_TOKEN env variable if available #794

Merged

Conversation

FrancescoSaverioZuppichini
Copy link
Contributor

@FrancescoSaverioZuppichini FrancescoSaverioZuppichini commented Mar 23, 2022

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, create a folder, and store 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 lines in the tests

Thanks

Francesco

@FrancescoSaverioZuppichini
Copy link
Contributor Author

Some tests are red, from the outputs it doesn't look like they are related to the changes in this PR

@adrinjalali
Copy link
Contributor

I think it is related though, it's coming from this line:

>       with unittest.mock.patch.dict(os.environ, {"HUGGING_FACE_HUB_TOKEN": None}):

@FrancescoSaverioZuppichini
Copy link
Contributor Author

FrancescoSaverioZuppichini commented Mar 24, 2022

opsy, missed! Updated but still not all tests are passing

Copy link
Contributor

@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.

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved
"""
token: Optional[str] = os.environ.get("HUGGING_FACE_HUB_TOKEN")
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we want to use the token if provided?

Suggested change
token: Optional[str] = os.environ.get("HUGGING_FACE_HUB_TOKEN")
token: Optional[str] = os.environ.get("HUGGING_FACE_HUB_TOKEN")
if token:
return token

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed with a check if token is None to avoid early returns

@adrinjalali adrinjalali changed the title Token in env variables ENH Use provided oken from HUGGINGFACE_HUB_TOKEN env variable if available Mar 24, 2022
@adrinjalali adrinjalali changed the title ENH Use provided oken from HUGGINGFACE_HUB_TOKEN env variable if available ENH Use provided oken from HUGGING_FACE_HUB_TOKEN env variable if available Mar 24, 2022
@FrancescoSaverioZuppichini FrancescoSaverioZuppichini changed the title ENH Use provided oken from HUGGING_FACE_HUB_TOKEN env variable if available Enable token in os env Mar 24, 2022
@FrancescoSaverioZuppichini FrancescoSaverioZuppichini changed the title Enable token in os env ENH Use provided token from HUGGING_FACE_HUB_TOKEN env variable if available Mar 24, 2022
@FrancescoSaverioZuppichini
Copy link
Contributor Author

Updated the code following @adrinjalali feedbacks :)

Copy link
Contributor

@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.

LGTM, but I'm not sure why you prefer to avoid an early return, I'm probably missing something as why it's not good practice. It does look better to me that way, but no strong opinions there.

@adrinjalali
Copy link
Contributor

@merveenoyan or @osanseviero do you know why the test fails?

_____________ HubKerasSequentialTest.test_push_to_hub_tensorboard ______________

args = (<tests.test_keras_integration.HubKerasSequentialTest testMethod=test_push_to_hub_tensorboard>,)
kwargs = {}, retry_count = 2

    def decorator(*args, **kwargs):
        retry_count = 1
        while retry_count < number_of_tries:
            try:
                return function(*args, **kwargs)
            except HTTPError as e:
                if e.response.status_code == 504:
                    logger.info(
                        f"Attempt {retry_count} failed with a 504 error. Retrying new execution in {wait_time} second(s)..."
                    )
                    time.sleep(5)
                    retry_count += 1
            except OSError:
                logger.info(
                    f"Race condition met where we tried to `clone` before fully deleting a repository. Retrying new execution in {wait_time} second(s)..."
                )
                retry_count += 1
            # Preserve original traceback
>           return function(*args, **kwargs)

tests/testing_utils.py:209: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tests/test_keras_integration.py:389: in test_push_to_hub_tensorboard
    git_email="ci@dummy.com",
/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/huggingface_hub/keras_mixin.py:310: in push_to_hub_keras
    git_email=git_email,
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <huggingface_hub.repository.Repository object at 0x7faafc9f61d0>
local_dir = '/home/runner/work/huggingface_hub/huggingface_hub/tests/fixtures/working_repo_test_keras_integration/PUSH_TO_HUB_TB'
clone_from = None, repo_type = None
use_auth_token = 'hf_94wBhPGp6KrrTH3KDchhKpRxZwd6dmHWLL', git_user = 'ci'
git_email = 'ci@dummy.com', revision = None, private = False
skip_lfs_files = False

    def __init__(
        self,
        local_dir: str,
        clone_from: Optional[str] = None,
        repo_type: Optional[str] = None,
        use_auth_token: Union[bool, str] = True,
        git_user: Optional[str] = None,
        git_email: Optional[str] = None,
        revision: Optional[str] = None,
        private: bool = False,
        skip_lfs_files: bool = False,
    ):
        """
        Instantiate a local clone of a git repo.
    
        If specifying a `clone_from`:
        will clone an existing remote repository, for instance one
        that was previously created using ``HfApi().create_repo(repo_id=repo_name)``.
        ``Repository`` uses the local git credentials by default, but if required, the ``huggingface_token``
        as well as the git ``user`` and the ``email`` can be explicitly specified.
        If `clone_from` is used, and the repository is being instantiated into a non-empty directory,
        e.g. a directory with your trained model files, it will automatically merge them.
    
        Args:
            local_dir (``str``):
                path (e.g. ``'my_trained_model/'``) to the local directory, where the ``Repository`` will be initalized.
            clone_from (``str``, `optional`):
                repository url (e.g. ``'https://huggingface.co/philschmid/playground-tests'``).
            repo_type (``str``, `optional`):
                To set when creating a repo: et to "dataset" or "space" if creating a dataset or space, default is model.
            use_auth_token (``str`` or ``bool``, `optional`, defaults to ``True``):
                huggingface_token can be extract from ``HfApi().login(username, password)`` and is used to authenticate against the hub
                (useful from Google Colab for instance).
            git_user (``str``, `optional`):
                will override the ``git config user.name`` for committing and pushing files to the hub.
            git_email (``str``, `optional`):
                will override the ``git config user.email`` for committing and pushing files to the hub.
            revision (``str``, `optional`):
                Revision to checkout after initializing the repository. If the revision doesn't exist, a
                branch will be created with that revision name from the default branch's current HEAD.
            private (``bool``, `optional`, defaults to ``False``):
                whether the repository is private or not.
            skip_lfs_files (``bool``, `optional`, defaults to ``False``):
                whether to skip git-LFS files or not.
        """
    
        os.makedirs(local_dir, exist_ok=True)
        self.local_dir = os.path.join(os.getcwd(), local_dir)
        self.repo_type = repo_type
        self.command_queue = []
        self.private = private
        self.skip_lfs_files = skip_lfs_files
    
        self.check_git_versions()
    
        if isinstance(use_auth_token, str):
            self.huggingface_token = use_auth_token
        elif use_auth_token:
            self.huggingface_token = HfFolder.get_token()
        else:
            self.huggingface_token = None
    
        if clone_from is not None:
            self.clone_from(repo_url=clone_from)
        else:
            if is_git_repo(self.local_dir):
                logger.debug("[Repository] is a valid git repo")
            else:
                raise ValueError(
>                   "If not specifying `clone_from`, you need to pass Repository a valid git clone."
                )
E               ValueError: If not specifying `clone_from`, you need to pass Repository a valid git clone.

/opt/hostedtoolcache/Python/3.7.12/x64/lib/python3.7/site-packages/huggingface_hub/repository.py:428: ValueError

@FrancescoSaverioZuppichini
Copy link
Contributor Author

FrancescoSaverioZuppichini commented Mar 24, 2022

LGTM, but I'm not sure why you prefer to avoid an early return, I'm probably missing something as why it's not good practice. It does look better to me that way, but no strong opinions there.

Same to me, I usually avoid them but if it's inconsistent I am happy to change it :)

@osanseviero
Copy link
Contributor

Could this be some Repository flakiness? The test has been passing for some time, and it passed in 3.10.

@adrinjalali
Copy link
Contributor

I've seen this failure quite a few times in the past few days though. Regardless, I think we can merge this PR anyway.

@merveenoyan
Copy link
Contributor

@adrinjalali I remember fixing this, I wonder if it's a past version. I'll look into it asap.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Apr 4, 2022

The documentation is not available anymore as the PR was closed or merged.

@FrancescoSaverioZuppichini FrancescoSaverioZuppichini merged commit 19edd71 into huggingface:main Apr 6, 2022
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.

5 participants