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 environment variables #1786

Merged
merged 2 commits into from
Oct 31, 2023
Merged

Conversation

Wauplin
Copy link
Contributor

@Wauplin Wauplin commented Oct 30, 2023

From a discussion started by @thomwolf on slack (private link) cc @clefourrier @LysandreJik as well.

It's slightly error prone to have sometimes HF_ sometimes HUGGING_FACE_ and sometimes HUGGINGFACE_ in the env variables we use throuhout our libs (at least it tricked me a couple of times). Maybe some unification (with backward comp) to discuss across the libs as well at some point


This PR fixes this by making all environments prefixed either by HF_ or HF_HUB_.
IMO, this will be particularly impactful for HF_TOKEN and HF_CACHE that were previously not intuitive.

4 environment variables have been modified:

Deprecated Variable Replacement
HUGGINGFACE_HUB_CACHE HF_HUB_CACHE
HUGGINGFACE_ASSETS_CACHE HF_ASSETS_CACHE
HUGGING_FACE_HUB_TOKEN HF_TOKEN
HUGGINGFACE_HUB_VERBOSITY HF_HUB_VERBOSITY

Backward compatibility is of course kept. No warning triggered if a legacy variable is used. If both the legacy and replacement variables are set on the machine, the newest (replacement) variable is kept.


Once this PR is merged and released, my plan will be to harmonize environment variables in the HF-ecosystem as well, starting with the Python libraries (transformers, datasets, diffusers, evaluate,...). But the goal is also to harmonize with JS, Rust and C# libraries when applicable.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Oct 30, 2023

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

Copy link
Member

@stevhliu stevhliu left a comment

Choose a reason for hiding this comment

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

Love the little table, great job! 👍

@julien-c
Copy link
Member

so it's harmonizing to transformers, right? (i.e. where do those HF_ env variables currently exist?)

@Wauplin
Copy link
Contributor Author

Wauplin commented Oct 30, 2023

It's about harmonizing between themselves. To avoid having HUGGINGFACE_HUB_CACHE, HUGGING_FACE_HUB_TOKEN and HF_HOME with three ways to spell huggingface. It is annoying as you always need to go back to the docs to check which one is the good one.

@julien-c
Copy link
Member

HF_CACHE => i would rename this one to HF_HUB_CACHE (and nest HF_HUB_ASSETS_CACHE if it's under hub/)

Other than that lgtm

@Wauplin
Copy link
Contributor Author

Wauplin commented Oct 31, 2023

I'm fine with HF_HUB_CACHE instead of HF_CACHE. Made the change in 71b8880.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Great! Thanks for doing the work here @Wauplin

Looks good to me. Let's make sure we update in transformers and others

@Wauplin
Copy link
Contributor Author

Wauplin commented Oct 31, 2023

Thanks for the review! I'll merge now.

Let's make sure we update in transformers and others

Yes definitely! I'll take care of it once released.

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