-
Notifications
You must be signed in to change notification settings - Fork 570
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
Add helpers to disable progress bars globally + tests #987
Conversation
The documentation is not available anymore as the PR was closed or merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for working on this!
I think the env variable should get priority over programmatic functions. My reasoning being that if the user goes out of their way to set it on the env level, it's really clear they want that thing all the time. But it's just my 2 cents and going the other way around also works.
Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>
Makes sense. That's what I also started to think when writing the PR text actually but didn't want to change the code before having a second opinion :D |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Solve #978 (see API discussion in issue).
@sgugger Since both
HF_HUB_DISABLE_PROGRESS_BARS
env variable anddisable_progress_bars()
/enable_progress_bars()
helpers are defined, there might be a conflict at some point if not correctly handled. For example a library, could calldisable_progress_bars()
even though the user has setHF_HUB_DISABLE_PROGRESS_BARS=0
. Current solution is thatdisable_progress_bars()
has higher priority but I'm not 100% sure it's clever.The other solution would be the opposite: if
HF_HUB_DISABLE_PROGRESS_BARS
is set (either 0 or 1), thendisable_progress_bars
/enable_progress_bars
are ignored in the code with a warning ("cannot programmatical disable progress bars as verbosity has been set by environment variable"
).What is your opinion about it ? I feel that both are correct, it's more a usage preference.
EDIT: after discussion below, we went for solution 2 (environment variable has priority).