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

Harmonize HF environment variables + deprecate use_auth_token #6066

Merged
merged 3 commits into from
Dec 6, 2023

Conversation

Wauplin
Copy link
Collaborator

@Wauplin Wauplin commented Dec 5, 2023

(Similar to huggingface/transformers#27564. Sorry in advance for the size of the diff, I hope it'll be ok :) )

This PR aims at harmonizing environment variables in the HF ecosystem (following up on huggingface/huggingface_hub#1786). I also took the time to review the integration of huggingface_hub into diffusers to update to latest "standard". I tried to break nothing -staying at iso-feature- but the review will be much appreciated 😄. The overall goal is simply to have less duplication in the codebase.

List of changes:

  1. Remove usage of use_auth_token in favor of token everywhere and in a backward-compatible way. To do do I used the @validate_hf_hub_args decorator that checks which of use_auth_token and/or token are passed. For now using use_auth_token do not raise a warning but it will in the future. The goal is to slowly adapt scripts before deprecating officially. And the deprecation will only have to happen in huggingface_hub thanks to the decorator.
  2. Remove references to use_auth_token=True. This is not necessary anymore since the token is always passed to the server by default (already the case since ~1y ago).
  3. Do not set local_files_only to HF_HUB_OFFLINE by default. This is already done in huggingface_hub (less duplicated code)
  4. Do not set cache_dir to DIFFUSERS_CACHE by default. This is already done in huggingface_hub (less duplicated code)
  5. Remove DIFFUSERS_CACHE which is strictly equal to HF_HUB_CACHE (shared with other libraries)
  6. Do not parse constant values HF_HUB_OFFLINE and DISABLE_TELEMETRY from environment variables but use huggingface_hub values instead
  7. Use huggingface_hub.constants.HF_HOME instead of huggingface_hub.constants.hf_cache_home
  8. Use huggingface_hub.get_full_repo_name (instead of defining it in diffusers)
  9. Add static typing to _get_model_file (that one was for myself - it looks like a heavily used method)

And that's it. Mostly cleaning some stuff in a backward-compatible way to remove duplicated work. Sorry in advance for the tedious review 🙈

@HuggingFaceDocBuilderDev

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.

@kopyl
Copy link
Contributor

kopyl commented Dec 5, 2023

Why did you think that removing this arg is a good idea?

Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

That looks great - thanks for all the effort!

@patrickvonplaten patrickvonplaten merged commit 75ada25 into main Dec 6, 2023
23 of 24 checks passed
@kopyl
Copy link
Contributor

kopyl commented Dec 7, 2023

@patrickvonplaten Why do you think that removing use_auth_token arg is a good idea?

@kopyl
Copy link
Contributor

kopyl commented Dec 7, 2023

@patrickvonplaten also why do you approve this commit with not all checks passed?

@Wauplin
Copy link
Collaborator Author

Wauplin commented Dec 7, 2023

@kopyl Argument use_auth_token will still work normally on all diffusers methods and will have the exact same behavior as before. So don't expect any breaking changes in the short-term. On the long term it will be deprecated and then completely removed in favor of token that has the exact same behavior. The goal is to harmonize parameter names across our libraries which should benefit users in the long run.

Regarding CI checks I guess the failing error was unrelated to this PR.

@kopyl
Copy link
Contributor

kopyl commented Dec 7, 2023

@patrickvonplaten thank you very much. I either did not read properly the description, or there was no description originally.

donhardman pushed a commit to donhardman/diffusers that referenced this pull request Dec 18, 2023
…gface#6066)

* Harmonize HF environment variables + deprecate use_auth_token

* fix import

* fix
yoonseokjin pushed a commit to yoonseokjin/diffusers that referenced this pull request Dec 25, 2023
…gface#6066)

* Harmonize HF environment variables + deprecate use_auth_token

* fix import

* fix
@Sanster Sanster mentioned this pull request Jan 16, 2024
AmericanPresidentJimmyCarter pushed a commit to AmericanPresidentJimmyCarter/diffusers that referenced this pull request Apr 26, 2024
…gface#6066)

* Harmonize HF environment variables + deprecate use_auth_token

* fix import

* fix
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.

4 participants