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

Harmonization of token Parameter Descriptions and Type Definitions in hf_api.py #2251

Closed
lappemic opened this issue Apr 25, 2024 · 8 comments

Comments

@lappemic
Copy link
Contributor

While working on issue #2209, I noticed inconsistencies in the type definitions and descriptions of the token parameter across different method signatures and their associated docstrings in huggingface_hub.hf_api.py. This could confuse external as well as internal developers.

Variations in Type Definitions:

  • token: Optional[str] = None
  • token: Union[bool, str, None] = None
  • token: Optional[Union[str, bool]] = None

Variations in Docstring Descriptions:

  1. "Hugging Face token. Will default to the locally saved token if not provided."
  2. "A valid authentication token (see https://huggingface.co/settings/token)."
  3. "Hugging Face token. Will default to the locally saved token if not provided. Pass token=False if you don't want to send your token to the server."
  4. "A valid authentication token (see https://huggingface.co/settings/token). If None or True and machine is logged in (through huggingface-cli login or [~huggingface_hub.login]), token will be retrieved from the cache. If False, token is not sent in the request header."
  5. "A valid authentication token (see https://huggingface.co/settings/token). Used only if user is not passed to implicitly determine the current user name."

Proposal for Standardization:

I propose a unified type definition and description for the token parameter. Here are the options to consider for the type definition:

  • Option 1: token: Optional[str] = None
  • Option 2: token: Union[bool, str, None] = None

And for the description, a comprehensive version could be:

"A valid authentication token (see https://huggingface.co/settings/token). If None or True and the machine is logged in (via huggingface-cli login or huggingface_hub.login), the token will be retrieved from the cache. Pass False if you do not want to send your token to the server."

Would harmonizing these descriptions and type definitions be helpful? If so, which options do you think should we standardise?

@Wauplin
Copy link
Contributor

Wauplin commented Apr 25, 2024

Thanks for looking in this @lappemic! Totally agree with you this would benefit from some harmonization.

For the record, ~2y ago tokens where not used by default, hence the boolean value. Now, tokens are always sent when the user is logged in (either via env variable or huggingface-cli login). So the only boolean value that is relevant is token=False to explicitly refuse authentication.

So if there is harmonization, it should be towards

Option 2: token: Union[bool, str, None] = None

About the attribute description. I would update it to mention that the recommended way to login is not by using this arg but rather login the machine itself. What do you think of:

"A valid user access token (string). If not set, will default to the locally saved one. Pass token=False to explicitly disable authentication. Note that the recommended way to authenticate is usually with a locally saved token (see https://huggingface.co/docs/huggingface_hub/quick-start#authentication)."

?

@lappemic
Copy link
Contributor Author

The description sounds reasonable and is clear 👍

I will open a PR for this the next days.

@Wauplin
Copy link
Contributor

Wauplin commented Apr 25, 2024

Second version? (less verbose)

"A valid user access token (string). If not set, will default to the locally saved one which is the recommended way to authenticate (see https://huggingface.co/docs/huggingface_hub/quick-start#authentication). Finally, pass False to explicitly disable authentication."

@Wauplin
Copy link
Contributor

Wauplin commented Apr 25, 2024

And thanks for raising this topic + propose your help on this one! It's something that always bothered me but never really addressed 😄

@lappemic
Copy link
Contributor Author

The second version is definitely less verbose. I just stumble across the "Finally" in the last sentence which is in my understanding like the last step in an "actionchain" (regardless the actions before). I do not want to make it picky, but what would you think about:

A valid user access token (string). Defaults to the locally saved token, which is the recommended method for authentication (see Hugging Face Hub Quick Start). To disable authentication, pass False.

@lappemic
Copy link
Contributor Author

And thanks for raising this topic + propose your help on this one! It's something that always bothered me but never really addressed 😄

Haha, addressing every itch would just be too overwhelming 😅 Glad i can help out here!

@Wauplin
Copy link
Contributor

Wauplin commented Apr 25, 2024

Thanks for the suggestion, let's use that! I'd just update it to set the url directly in the docstring. I think it's better for users reading the srouce code directly (so not rendered),

A valid user access token (string). Defaults to the locally saved token, which is the recommended method for authentication (see https://huggingface.co/docs/huggingface_hub/quick-start#authentication). To disable authentication, pass False.

@Wauplin
Copy link
Contributor

Wauplin commented Apr 29, 2024

Close by @lappemic in #2252. Thanks!

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

No branches or pull requests

2 participants