implemented `auth_check` #2497

cjfghk5697 · 2024-08-30T17:57:35Z

Related to #2466

I have implemented auth_check related to the issue #2466.

Implemented auth_check
Added docstring for auth_check
Wrote test code for auth_check

Please review the implementation and let me know if any further changes are required.

hanouticelina

Hello @cjfghk5697, thank you for this PR 🤗 I left few comments, otherwise everything looks good to me. The indentation fix should resolve both the documentation build and the code quality check in the CI pipelines

hanouticelina · 2024-09-03T09:15:38Z

src/huggingface_hub/hf_api.py

+        If the repository is gated or does not exist, the respective error is raised.
+    """
+        headers = self._build_hf_headers(token=token)
+        path = f"{self.endpoint}/api/datasets/{repo_id}/auth-check"


The current implementation only handles the datasets repo type. It should handle all repo types supported by the Hugging Face Hub (i.e. datasets, models and spaces). You can add a repo_type argument to the method.

Suggested change

path = f"{self.endpoint}/api/datasets/{repo_id}/auth-check"

if repo_type is None:

repo_type = constants.REPO_TYPE_MODEL

if repo_type not in constants.REPO_TYPES:

raise ValueError(

f"Invalid repo type, must be one of {constants.REPO_TYPES}"

)

path = f"{self.endpoint}/api/{repo_type}s/{repo_id}/auth-check"

hanouticelina · 2024-09-03T09:18:22Z

src/huggingface_hub/hf_api.py

@@ -9525,6 +9529,51 @@ def list_user_following(self, username: str) -> Iterable[User]:
        for followed_user in r.json():
            yield User(**followed_user)

+    def auth_check(self, repo_id: str, token: Union[bool, str, None] = None) -> None:
+    """


Small indentation fix needed on this line

hanouticelina · 2024-09-03T09:33:35Z

src/huggingface_hub/hf_api.py

@@ -9525,6 +9529,51 @@ def list_user_following(self, username: str) -> Iterable[User]:
        for followed_user in r.json():
            yield User(**followed_user)

+    def auth_check(self, repo_id: str, token: Union[bool, str, None] = None) -> None:


I think it would be more convenient if the method returns True when the user has access. This change would allow users to use the boolean return in conditional statements

While a boolean value is convenient to use in a if/else statement, it will nonetheless be hiding some information to the end user. For now there are 3 cases:

passing correctly

RepositoryNotFoundError error if private or not existing

GatedRepoError error if exists but wrong permission

The end user will eventually want to deal with these errors differently depending on their use cases. Letting the correct error be raised makes it flexible.

However what would be nice is to document how to use the method:

from huggingface_hub import auth_check from huggingface_hub.utils import GatedRepoError, RepositoryNotFoundError try: auth_check(repo_id, repo_type=repo_type) except GatedRepoError: # Handle gated repo ... except RepositoryNotFoundError: # Handle missing repo ...

This snippet can be added as an example in the docstring.

To be honest, auth_check is for more advanced usage of the lib'. For power users it's fine to have slightly more verbose boilerplate code.

hanouticelina · 2024-09-03T09:34:38Z

tests/test_hf_api.py

+
+    @patch.object(HfApi, "auth_check", return_value=None)
+    def test_auth_check_success(self, mock_auth_check):
+        try:


If auth_check raises any exception, the test will automatically fail without needing self.fail(), The try/except block with self.fail() is redundant in this case.

Wauplin

Thanks a lot for the PR @cjfghk5697! Agree with most comments @hanouticelina above :) I've added a few comment especially related to how we should test this feature. Let us know if you have any question!

Wauplin · 2024-09-03T12:05:21Z

src/huggingface_hub/hf_api.py

+    Example:
+        >>> api = HfApi(token="your_token")
+        >>> api.auth_check(repo_id="user/my-cool-model")


Suggested change

Example:

>>> api = HfApi(token="your_token")

>>> api.auth_check(repo_id="user/my-cool-model")

Example:

```py

>>> from huggingface_hub import HfApi

>>> api = HfApi(token="your_token")

>>> api.auth_check(repo_id="user/my-cool-model")

```

src/huggingface_hub/hf_api.py

Wauplin · 2024-09-03T12:15:31Z

src/huggingface_hub/hf_api.py

@@ -9525,6 +9529,51 @@ def list_user_following(self, username: str) -> Iterable[User]:
        for followed_user in r.json():
            yield User(**followed_user)

+    def auth_check(self, repo_id: str, token: Union[bool, str, None] = None) -> None:


While a boolean value is convenient to use in a if/else statement, it will nonetheless be hiding some information to the end user. For now there are 3 cases:

passing correctly

RepositoryNotFoundError error if private or not existing

GatedRepoError error if exists but wrong permission

The end user will eventually want to deal with these errors differently depending on their use cases. Letting the correct error be raised makes it flexible.

However what would be nice is to document how to use the method:

from huggingface_hub import auth_check from huggingface_hub.utils import GatedRepoError, RepositoryNotFoundError try: auth_check(repo_id, repo_type=repo_type) except GatedRepoError: # Handle gated repo ... except RepositoryNotFoundError: # Handle missing repo ...

This snippet can be added as an example in the docstring.

Wauplin · 2024-09-03T12:16:53Z

src/huggingface_hub/hf_api.py

@@ -9525,6 +9529,51 @@ def list_user_following(self, username: str) -> Iterable[User]:
        for followed_user in r.json():
            yield User(**followed_user)

+    def auth_check(self, repo_id: str, token: Union[bool, str, None] = None) -> None:


To be honest, auth_check is for more advanced usage of the lib'. For power users it's fine to have slightly more verbose boilerplate code.

Wauplin · 2024-09-03T12:24:28Z

tests/test_hf_api.py

@@ -4239,3 +4231,27 @@ def test_upload_large_folder(self, repo_url: RepoUrl) -> None:
            for j in range(N_FILES_PER_FOLDER):
                assert f"subfolder_{i}/file_lfs_{i}_{j}.bin" in uploaded_files
                assert f"subfolder_{i}/file_regular_{i}_{j}.txt" in uploaded_files
+
+
+class TestHfApiAuthCheck(HfApiCommonTest):


Thanks for adding tests as well. Mocked tests are useful in many cases but for the core HfApi features, we want to test the behavior "for real" on the staging environment. Here is how you can do that!

class TestHfApiAuthCheck(HfApiCommonTest): @use_tmp_repo(repo_type="dataset") def test_auth_check_success(self, repo_url: RepoUrl) -> None: self._api.auth_check(repo_id=repo_url.repo_id, repo_type=repo_url.repo_type) def test_auth_check_repo_missing(self) -> None: with self.assertRaises(RepositoryNotFoundError): self._api.auth_check(repo_id="username/missing_repo_id")

In the first case, you test what happens if the repo exists (using @use_tmp_repo decorator). In the second case, you test what happens if the repo is missing (with a fake url). Finally, you'll need a last case where you create a new repo + set it as gated (as done here) + try auth_check with a different token. Since a user has always access to its own repo, no matter if it's gated, you need to create a repo with user A and then test access with user B. You can use TOKEN and OTHER_TOKEN for that.

cjfghk5697 · 2024-09-04T06:18:28Z

@Wauplin @hanouticelina Thank you so much for your reviews! I've refactored the code based on your feedback. Could you please review it again?

I really appreciate your work. Thanks to you, I feel like I've learned how to write better code😊🤗

hanouticelina

Hello @cjfghk5697, thanks for the refactoring :) I've left a comment related to the return type of auth_check, otherwise everything else looks good to me

hanouticelina · 2024-09-04T09:01:33Z

src/huggingface_hub/hf_api.py

@@ -9525,6 +9529,83 @@ def list_user_following(self, username: str) -> Iterable[User]:
        for followed_user in r.json():
            yield User(**followed_user)

+    def auth_check(self, repo_id: str, repo_type: Optional[str] = None, token: Union[bool, str, None] = None) -> bool:


As discussed by @Wauplin here, using a boolean for auth_check combines different error types into a single 'False' result. Instead, allowing specific exceptions (GatedRepoError, RepositoryNotFoundError) to propagate provides users with more detailed information and enables more precise error handling in their code.

tests/test_hf_api.py

src/huggingface_hub/hf_api.py

cjfghk5697 · 2024-09-05T01:03:05Z

@Wauplin @hanouticelina
I've completed the updates according to review. The auth_check function now works as expected, and the test case properly verifies access denial for gated repositories. Thank you for guidance and support!

Wauplin · 2024-09-10T15:13:33Z

Hi @cjfghk5697, thanks for the update. As discussed in #2497 (comment) and #2497 (comment) we would prefer for auth_check to raise an error instead of returning a boolean to check authorization. Could you take care of it (see more details in above comments). Thanks!

cjfghk5697 · 2024-09-11T06:44:22Z

Hi @Wauplin,

I apologize for the mistake, and thank you for pointing it out. I've made the necessary adjustments as per your feedback. Could you please review it again when you have a moment?

Thank you!

HuggingFaceDocBuilderDev · 2024-09-11T07:47:12Z

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.

hanouticelina

Hi @cjfghk5697, thanks a lot for the iterations! seems good to me 👍
cc @Wauplin

Wauplin

Thanks a lot @cjfghk5697! I have left final comments regarding code styling and docstrings that I will merge right away. Other than that, we are good to merge! 🎉

src/huggingface_hub/hf_api.py

Wauplin · 2024-09-11T09:50:51Z

tests/test_hf_api.py

+        with self.assertRaises(RepositoryNotFoundError):
+            self._api.auth_check(repo_id="username/missing_repo_id")
+
+    def test_auth_check_gated_repo(self) -> None:


Test looks good!

src/huggingface_hub/hf_api.py

Wauplin · 2024-09-11T10:13:35Z

All green! Thanks a lot @cjfghk5697 🤗 🎉

cjfghk5697 · 2024-09-11T10:22:10Z

@Wauplin @hanouticelina Thank you so much for all your help. If there's anything else that needs to be handled, please let me know!

cjfghk5697 added 2 commits August 30, 2024 17:41

check auth

7b15b1f

doc string

cc42280

cjfghk5697 changed the title ~~check auth~~ implemented auth_check Aug 30, 2024

hanouticelina self-requested a review September 3, 2024 08:51

hanouticelina requested changes Sep 3, 2024

View reviewed changes

Wauplin reviewed Sep 3, 2024

View reviewed changes

cjfghk5697 added 3 commits September 4, 2024 05:16

Refactor auth_check

c699a44

make style & quality

9829958

Docstring

900e670

Merge branch 'main' into auth_check

20c05a9

hanouticelina requested changes Sep 4, 2024

View reviewed changes

Wauplin reviewed Sep 4, 2024

View reviewed changes

tests/test_hf_api.py Outdated Show resolved Hide resolved

Wauplin reviewed Sep 4, 2024

View reviewed changes

src/huggingface_hub/hf_api.py Outdated Show resolved Hide resolved

cjfghk5697 added 3 commits September 5, 2024 01:05

change gate test, expect value and delete duplicate code

266e7e2

Merge branch 'main' into auth_check

6a3a10e

delete duplicate code, change gate test, auth api return value

834efc1

raise error

a151058

Merge branch 'main' into auth_check

b7d5fee

hanouticelina approved these changes Sep 11, 2024

View reviewed changes

Wauplin approved these changes Sep 11, 2024

View reviewed changes

Wauplin added 2 commits September 11, 2024 11:58

Apply suggestions from code review

6703c84

style

0e96458

Wauplin merged commit 855755b into huggingface:main Sep 11, 2024
16 checks passed

cjfghk5697 deleted the auth_check branch September 12, 2024 05:43

Wauplin mentioned this pull request Sep 13, 2024

Add support for /auth-check #2466

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implemented `auth_check` #2497

implemented `auth_check` #2497

cjfghk5697 commented Aug 30, 2024 •

edited

Loading

hanouticelina left a comment

hanouticelina Sep 3, 2024

hanouticelina Sep 3, 2024

hanouticelina Sep 3, 2024

Wauplin Sep 3, 2024

Wauplin Sep 3, 2024

hanouticelina Sep 3, 2024

hanouticelina Sep 3, 2024

Wauplin left a comment

Wauplin Sep 3, 2024

Wauplin Sep 3, 2024

Wauplin Sep 3, 2024

Wauplin Sep 3, 2024

cjfghk5697 commented Sep 4, 2024 •

edited

Loading

hanouticelina left a comment

hanouticelina Sep 4, 2024

cjfghk5697 commented Sep 5, 2024

Wauplin commented Sep 10, 2024

cjfghk5697 commented Sep 11, 2024

HuggingFaceDocBuilderDev commented Sep 11, 2024

hanouticelina left a comment

Wauplin left a comment

Wauplin Sep 11, 2024

Wauplin commented Sep 11, 2024

cjfghk5697 commented Sep 11, 2024

-        path = f"{self.endpoint}/api/datasets/{repo_id}/auth-check"
+        if repo_type is None:
+            repo_type = constants.REPO_TYPE_MODEL
+        if repo_type not in constants.REPO_TYPES:
+            raise ValueError(
+                f"Invalid repo type, must be one of {constants.REPO_TYPES}"
+            )
+        path = f"{self.endpoint}/api/{repo_type}s/{repo_id}/auth-check"

implemented auth_check #2497

implemented auth_check #2497

Conversation

cjfghk5697 commented Aug 30, 2024 • edited Loading

hanouticelina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wauplin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjfghk5697 commented Sep 4, 2024 • edited Loading

hanouticelina left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjfghk5697 commented Sep 5, 2024

Wauplin commented Sep 10, 2024

cjfghk5697 commented Sep 11, 2024

HuggingFaceDocBuilderDev commented Sep 11, 2024

hanouticelina left a comment

Choose a reason for hiding this comment

Wauplin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wauplin commented Sep 11, 2024

cjfghk5697 commented Sep 11, 2024

implemented `auth_check` #2497

implemented `auth_check` #2497

cjfghk5697 commented Aug 30, 2024 •

edited

Loading

cjfghk5697 commented Sep 4, 2024 •

edited

Loading