From b6222deae7b959079493e7e8e26994f032520860 Mon Sep 17 00:00:00 2001 From: Lucain Pouget Date: Thu, 24 Oct 2024 16:12:46 +0200 Subject: [PATCH 1/2] Deprecate is_write_action and write_permission=True when login --- src/huggingface_hub/_login.py | 92 +++++++++++---------------- src/huggingface_hub/hf_api.py | 2 - src/huggingface_hub/utils/_headers.py | 34 +++------- tests/test_auth.py | 16 ----- tests/test_utils_headers.py | 13 ---- 5 files changed, 47 insertions(+), 110 deletions(-) diff --git a/src/huggingface_hub/_login.py b/src/huggingface_hub/_login.py index 2901437752..b14702201d 100644 --- a/src/huggingface_hub/_login.py +++ b/src/huggingface_hub/_login.py @@ -15,7 +15,6 @@ import os import subprocess -from functools import partial from getpass import getpass from pathlib import Path from typing import Optional @@ -42,6 +41,7 @@ _save_token, get_stored_tokens, ) +from .utils._deprecation import _deprecate_arguments, _deprecate_positional_args logger = logging.get_logger(__name__) @@ -55,8 +55,15 @@ """ +@_deprecate_arguments( + version="1.0", + deprecated_args="write_permission", + custom_message="Fine-grained tokens added complexity to the permissions, making it irrelevant to check if a token has 'write' access.", +) +@_deprecate_positional_args(version="1.0") def login( token: Optional[str] = None, + *, add_to_git_credential: bool = False, new_session: bool = True, write_permission: bool = False, @@ -97,8 +104,8 @@ def login( to the end user. new_session (`bool`, defaults to `True`): If `True`, will request a token even if one is already saved on the machine. - write_permission (`bool`, defaults to `False`): - If `True`, requires a token with write permission. + write_permission (`bool`): + Ignored and deprecated argument. Raises: [`ValueError`](https://docs.python.org/3/library/exceptions.html#ValueError) If an organization token is passed. Only personal account tokens are valid @@ -116,11 +123,11 @@ def login( "`--add-to-git-credential` if using via `huggingface-cli` if " "you want to set the git credential as well." ) - _login(token, add_to_git_credential=add_to_git_credential, write_permission=write_permission) + _login(token, add_to_git_credential=add_to_git_credential) elif is_notebook(): - notebook_login(new_session=new_session, write_permission=write_permission) + notebook_login(new_session=new_session) else: - interpreter_login(new_session=new_session, write_permission=write_permission) + interpreter_login(new_session=new_session) def logout(token_name: Optional[str] = None) -> None: @@ -235,7 +242,13 @@ def auth_list() -> None: ### -def interpreter_login(new_session: bool = True, write_permission: bool = False) -> None: +@_deprecate_arguments( + version="1.0", + deprecated_args="write_permission", + custom_message="Fine-grained tokens added complexity to the permissions, making it irrelevant to check if a token has 'write' access.", +) +@_deprecate_positional_args(version="1.0") +def interpreter_login(*, new_session: bool = True, write_permission: bool = False) -> None: """ Displays a prompt to log in to the HF website and store the token. @@ -248,11 +261,10 @@ def interpreter_login(new_session: bool = True, write_permission: bool = False) Args: new_session (`bool`, defaults to `True`): If `True`, will request a token even if one is already saved on the machine. - write_permission (`bool`, defaults to `False`): - If `True`, requires a token with write permission. - + write_permission (`bool`): + Ignored and deprecated argument. """ - if not new_session and _current_token_okay(write_permission=write_permission): + if not new_session and get_token() is not None: logger.info("User is already logged in.") return @@ -275,11 +287,7 @@ def interpreter_login(new_session: bool = True, write_permission: bool = False) token = getpass("Enter your token (input will not be visible): ") add_to_git_credential = _ask_for_confirmation_no_tui("Add token as git credential?") - _login( - token=token, - add_to_git_credential=add_to_git_credential, - write_permission=write_permission, - ) + _login(token=token, add_to_git_credential=add_to_git_credential) ### @@ -306,7 +314,13 @@ def interpreter_login(new_session: bool = True, write_permission: bool = False) notebooks. """ -def notebook_login(new_session: bool = True, write_permission: bool = False) -> None: +@_deprecate_arguments( + version="1.0", + deprecated_args="write_permission", + custom_message="Fine-grained tokens added complexity to the permissions, making it irrelevant to check if a token has 'write' access.", +) +@_deprecate_positional_args(version="1.0") +def notebook_login(*, new_session: bool = True, write_permission: bool = False) -> None: """ Displays a widget to log in to the HF website and store the token. @@ -319,8 +333,8 @@ def notebook_login(new_session: bool = True, write_permission: bool = False) -> Args: new_session (`bool`, defaults to `True`): If `True`, will request a token even if one is already saved on the machine. - write_permission (`bool`, defaults to `False`): - If `True`, requires a token with write permission. + write_permission (`bool`): + Ignored and deprecated argument. """ try: import ipywidgets.widgets as widgets # type: ignore @@ -330,7 +344,7 @@ def notebook_login(new_session: bool = True, write_permission: bool = False) -> "The `notebook_login` function can only be used in a notebook (Jupyter or" " Colab) and you need the `ipywidgets` module: `pip install ipywidgets`." ) - if not new_session and _current_token_okay(write_permission=write_permission): + if not new_session and get_token() is not None: logger.info("User is already logged in.") return @@ -353,14 +367,8 @@ def notebook_login(new_session: bool = True, write_permission: bool = False) -> display(login_token_widget) # On click events - def login_token_event(t, write_permission: bool = False): - """ - Event handler for the login button. - - Args: - write_permission (`bool`, defaults to `False`): - If `True`, requires a token with write permission. - """ + def login_token_event(t): + """Event handler for the login button.""" token = token_widget.value add_to_git_credential = git_checkbox_widget.value # Erase token and clear value to make sure it's not saved in the notebook. @@ -369,14 +377,14 @@ def login_token_event(t, write_permission: bool = False): login_token_widget.children = [widgets.Label("Connecting...")] try: with capture_output() as captured: - _login(token, add_to_git_credential=add_to_git_credential, write_permission=write_permission) + _login(token, add_to_git_credential=add_to_git_credential) message = captured.getvalue() except Exception as error: message = str(error) # Print result (success message or error) login_token_widget.children = [widgets.Label(line) for line in message.split("\n") if line.strip()] - token_finish_button.on_click(partial(login_token_event, write_permission=write_permission)) + token_finish_button.on_click(login_token_event) ### @@ -387,7 +395,6 @@ def login_token_event(t, write_permission: bool = False): def _login( token: str, add_to_git_credential: bool, - write_permission: bool = False, ) -> None: from .hf_api import whoami # avoid circular import @@ -396,11 +403,6 @@ def _login( token_info = whoami(token) permission = token_info["auth"]["accessToken"]["role"] - if write_permission and permission != "write": - raise ValueError( - "Token is valid but is 'read-only' and a 'write' token is required.\nPlease provide a new token with" - " correct permission." - ) logger.info(f"Token is valid (permission: {permission}).") token_name = token_info["auth"]["accessToken"]["displayName"] @@ -469,24 +471,6 @@ def _set_active_token( logger.info(f"Your token has been saved to {constants.HF_TOKEN_PATH}") -def _current_token_okay(write_permission: bool = False): - """Check if the current token is valid. - - Args: - write_permission (`bool`, defaults to `False`): - If `True`, requires a token with write permission. - - Returns: - `bool`: `True` if the current token is valid, `False` otherwise. - """ - from .hf_api import get_token_permission # avoid circular import - - permission = get_token_permission() - if permission is None or (write_permission and permission != "write"): - return False - return True - - def _is_git_credential_helper_configured() -> bool: """Check if a git credential helper is configured. diff --git a/src/huggingface_hub/hf_api.py b/src/huggingface_hub/hf_api.py index 5ab1aa0ebb..0b53fc877d 100644 --- a/src/huggingface_hub/hf_api.py +++ b/src/huggingface_hub/hf_api.py @@ -9082,7 +9082,6 @@ def delete_webhook(self, webhook_id: str, *, token: Union[bool, str, None] = Non def _build_hf_headers( self, token: Union[bool, str, None] = None, - is_write_action: bool = False, library_name: Optional[str] = None, library_version: Optional[str] = None, user_agent: Union[Dict, str, None] = None, @@ -9096,7 +9095,6 @@ def _build_hf_headers( token = self.token return build_hf_headers( token=token, - is_write_action=is_write_action, library_name=library_name or self.library_name, library_version=library_version or self.library_version, user_agent=user_agent or self.user_agent, diff --git a/src/huggingface_hub/utils/_headers.py b/src/huggingface_hub/utils/_headers.py index 8b05e939db..300e6b4e9c 100644 --- a/src/huggingface_hub/utils/_headers.py +++ b/src/huggingface_hub/utils/_headers.py @@ -20,6 +20,7 @@ from .. import constants from ._auth import get_token +from ._deprecation import _deprecate_arguments from ._runtime import ( get_fastai_version, get_fastcore_version, @@ -35,15 +36,20 @@ from ._validators import validate_hf_hub_args +@_deprecate_arguments( + version="1.0", + deprecated_args="is_write_action", + custom_message="This argument is ignored and we let the server handle the permission error instead (if any).", +) @validate_hf_hub_args def build_hf_headers( *, token: Optional[Union[bool, str]] = None, - is_write_action: bool = False, library_name: Optional[str] = None, library_version: Optional[str] = None, user_agent: Union[Dict, str, None] = None, headers: Optional[Dict[str, str]] = None, + is_write_action: bool = False, ) -> Dict[str, str]: """ Build headers dictionary to send in a HF Hub call. @@ -68,9 +74,6 @@ def build_hf_headers( - if `False`, authorization header is not set - if `None`, the token is read from the machine only except if `HF_HUB_DISABLE_IMPLICIT_TOKEN` env variable is set. - is_write_action (`bool`, default to `False`): - Set to True if the API call requires a write access. If `True`, the token - will be validated (cannot be `None`, cannot start by `"api_org***"`). library_name (`str`, *optional*): The name of the library that is making the HTTP request. Will be added to the user-agent header. @@ -83,6 +86,8 @@ def build_hf_headers( headers (`dict`, *optional*): Additional headers to include in the request. Those headers take precedence over the ones generated by this function. + is_write_action (`bool`): + Ignored and deprecated argument. Returns: A `Dict` of headers to pass in your API call. @@ -105,9 +110,6 @@ def build_hf_headers( >>> build_hf_headers() # token is not sent {"user-agent": ...} - >>> build_hf_headers(token="api_org_***", is_write_action=True) - ValueError: You must use your personal account token for write-access methods. - >>> build_hf_headers(library_name="transformers", library_version="1.2.3") {"authorization": ..., "user-agent": "transformers/1.2.3; hf_hub/0.10.2; python/3.10.4; tensorflow/1.55"} ``` @@ -122,7 +124,6 @@ def build_hf_headers( """ # Get auth token to send token_to_send = get_token_to_send(token) - _validate_token_to_send(token_to_send, is_write_action=is_write_action) # Combine headers hf_headers = { @@ -171,23 +172,6 @@ def get_token_to_send(token: Optional[Union[bool, str]]) -> Optional[str]: return cached_token -def _validate_token_to_send(token: Optional[str], is_write_action: bool) -> None: - if is_write_action: - if token is None: - raise ValueError( - "Token is required (write-access action) but no token found. You need" - " to provide a token or be logged in to Hugging Face with" - " `huggingface-cli login` or `huggingface_hub.login`. See" - " https://huggingface.co/settings/tokens." - ) - if token.startswith("api_org"): - raise ValueError( - "You must use your personal account token for write-access methods. To" - " generate a write-access token, go to" - " https://huggingface.co/settings/tokens" - ) - - def _http_user_agent( *, library_name: Optional[str] = None, diff --git a/tests/test_auth.py b/tests/test_auth.py index fd1a18f641..457b9342ef 100644 --- a/tests/test_auth.py +++ b/tests/test_auth.py @@ -88,22 +88,6 @@ def test_login_success(self, mock_whoami): assert _get_token_by_name("test_token") == TOKEN assert _get_token_from_file() == TOKEN - @patch( - "huggingface_hub.hf_api.whoami", - return_value={ - "auth": { - "accessToken": { - "displayName": "test_token", - "role": "read", - "createdAt": "2024-01-01T00:00:00.000Z", - } - } - }, - ) - def test_login_errors(self, mock_whoami): - with pytest.raises(ValueError, match=r"Token is valid but is 'read-only' and a 'write' token is required.*"): - _login(TOKEN, add_to_git_credential=False, write_permission=True) - class TestLogout: def test_logout_deletes_files(self): diff --git a/tests/test_utils_headers.py b/tests/test_utils_headers.py index 89cce741c3..202f4283b0 100644 --- a/tests/test_utils_headers.py +++ b/tests/test_utils_headers.py @@ -46,19 +46,6 @@ def test_use_auth_token_none_no_cached_token(self, mock_get_token: Mock) -> None def test_use_auth_token_none_has_cached_token(self, mock_get_token: Mock) -> None: self.assertEqual(build_hf_headers(), FAKE_TOKEN_HEADER) - def test_write_action_org_token(self) -> None: - with self.assertRaises(ValueError): - build_hf_headers(use_auth_token=FAKE_TOKEN_ORG, is_write_action=True) - - @patch("huggingface_hub.utils._headers.get_token", return_value=None) - def test_write_action_none_token(self, mock_get_token: Mock) -> None: - with self.assertRaises(ValueError): - build_hf_headers(is_write_action=True) - - def test_write_action_use_auth_token_false(self) -> None: - with self.assertRaises(ValueError): - build_hf_headers(use_auth_token=False, is_write_action=True) - @patch("huggingface_hub.utils._headers.get_token", return_value=FAKE_TOKEN) def test_implicit_use_disabled(self, mock_get_token: Mock) -> None: with patch( # not as decorator to avoid friction with @handle_injection From 9a7fcf4917478c25c7c2403d5d435915a783c271 Mon Sep 17 00:00:00 2001 From: Lucain Pouget Date: Fri, 25 Oct 2024 11:07:21 +0200 Subject: [PATCH 2/2] fix mypy --- src/huggingface_hub/file_download.py | 12 ++++++------ src/huggingface_hub/inference/_client.py | 4 +++- .../inference/_generated/_async_client.py | 4 +++- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/src/huggingface_hub/file_download.py b/src/huggingface_hub/file_download.py index def44b3a34..46d5ccaead 100644 --- a/src/huggingface_hub/file_download.py +++ b/src/huggingface_hub/file_download.py @@ -821,7 +821,7 @@ def hf_hub_download( if repo_type not in constants.REPO_TYPES: raise ValueError(f"Invalid repo type: {repo_type}. Accepted repo types are: {str(constants.REPO_TYPES)}") - headers = build_hf_headers( + hf_headers = build_hf_headers( token=token, library_name=library_name, library_version=library_version, @@ -850,7 +850,7 @@ def hf_hub_download( # HTTP info endpoint=endpoint, etag_timeout=etag_timeout, - headers=headers, + headers=hf_headers, proxies=proxies, token=token, # Additional options @@ -870,7 +870,7 @@ def hf_hub_download( # HTTP info endpoint=endpoint, etag_timeout=etag_timeout, - headers=headers, + headers=hf_headers, proxies=proxies, token=token, # Additional options @@ -1283,20 +1283,20 @@ def get_hf_file_metadata( A [`HfFileMetadata`] object containing metadata such as location, etag, size and commit_hash. """ - headers = build_hf_headers( + hf_headers = build_hf_headers( token=token, library_name=library_name, library_version=library_version, user_agent=user_agent, headers=headers, ) - headers["Accept-Encoding"] = "identity" # prevent any compression => we want to know the real size of the file + hf_headers["Accept-Encoding"] = "identity" # prevent any compression => we want to know the real size of the file # Retrieve metadata r = _request_wrapper( method="HEAD", url=url, - headers=headers, + headers=hf_headers, allow_redirects=False, follow_relative_redirects=True, proxies=proxies, diff --git a/src/huggingface_hub/inference/_client.py b/src/huggingface_hub/inference/_client.py index 8d5a8e6a38..ed473e6d11 100644 --- a/src/huggingface_hub/inference/_client.py +++ b/src/huggingface_hub/inference/_client.py @@ -178,7 +178,9 @@ def __init__( self.model: Optional[str] = model self.token: Union[str, bool, None] = token if token is not None else api_key - self.headers = CaseInsensitiveDict(build_hf_headers(token=self.token)) # 'authorization' + 'user-agent' + self.headers: CaseInsensitiveDict[str] = CaseInsensitiveDict( + build_hf_headers(token=self.token) # 'authorization' + 'user-agent' + ) if headers is not None: self.headers.update(headers) self.cookies = cookies diff --git a/src/huggingface_hub/inference/_generated/_async_client.py b/src/huggingface_hub/inference/_generated/_async_client.py index 5c3a8044fc..74888bc0b8 100644 --- a/src/huggingface_hub/inference/_generated/_async_client.py +++ b/src/huggingface_hub/inference/_generated/_async_client.py @@ -170,7 +170,9 @@ def __init__( self.model: Optional[str] = model self.token: Union[str, bool, None] = token if token is not None else api_key - self.headers = CaseInsensitiveDict(build_hf_headers(token=self.token)) # 'authorization' + 'user-agent' + self.headers: CaseInsensitiveDict[str] = CaseInsensitiveDict( + build_hf_headers(token=self.token) # 'authorization' + 'user-agent' + ) if headers is not None: self.headers.update(headers) self.cookies = cookies