Fix custom tqdm_class silently broken in non-TTY environments#4056
Fix custom tqdm_class silently broken in non-TTY environments#4056hanouticelina wants to merge 4 commits intomainfrom
tqdm_class silently broken in non-TTY environments#4056Conversation
| pbar.close() | ||
|
|
||
|
|
||
| def _create_progress_bar(*, cls: type[old_tqdm], log_level: int, name: str | None = None, **kwargs) -> old_tqdm: |
There was a problem hiding this comment.
added this private helper to centralize how we initialize the tqdm progress bar. is_tqdm_disabled is no longer needed internally but kept it since it's exposed in huggingface_hub.utils module
|
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. |
|
Thanks for the fix @hanouticelina! There's a related but distinct tqdm bug that this PR's tqdm crashes with multiprocessing lock errors when used in threaded contexts such as Textual TUI worker threads, some web server setups, etc. The error looks like: or This happens because tqdm internally initializes multiprocessing locks during def _create_progress_bar(*, cls, log_level, name=None, **kwargs):
# ... existing logic ...
try:
return cls(disable=disable, name=name, **kwargs)
except (OSError, ValueError):
# tqdm multiprocessing lock init can fail in threaded contexts
# (Textual workers, some web servers). Fall back to disabled bar.
logger.warning(
"Progress bar could not be initialized in this environment. "
"Download will continue without progress reporting. "
"Set HF_HUB_DISABLE_PROGRESS_BARS=1 to silence this warning."
)
return cls(disable=True, **kwargs)Without this centralized entry point, there's no clean place to catch the error. tqdm creation was previously scattered across multiple call sites. This PR creates the right abstraction to handle it in one place. Right now users can work around this by setting This crash was initial root motivation behind #4051 and I feel it might as well be addressed here. The non-TTY silencing was a secondary symptom, but the primary crash was tqdm's multiprocessing lock failing in worker threads. My suggestion is to add this try/except and add a test case before merging. This way a separate fix/pr can be avoided and this is a great defensive measure. |
| disable = False | ||
| else: | ||
| disable = None | ||
| return cls(disable=disable, name=name, **kwargs) # type: ignore[return-value] |
There was a problem hiding this comment.
_create_progress_bar should wrap instantiation in try/except (OSError, ValueError), log a warning, and fall back to a no-op. A progress bar failing to init shouldn't crash the download.
See #4056 (comment)
Fixes #4050.
when passing a custom
tqdm_classtohf_hub_download()orsnapshot_download(), progress tracking is broken. this is because all tqdm classes are instantiated identically. When a user passes a customtqdm_class, they take full control of progress bar behavior. The library should not inject HF-specific kwargs (name) or override display logic (disable). the custom class is responsible for its own configuration.Here is a small reproducible:
this causes two bugs depending on the environment:
In TTY:
disable=None->isatty()returnsTrue-> tqdm continues__init__-> hits the unknown kwargs check →TqdmKeyErroronname:In non-TTY:
is_tqdm_disabled()returnsNone. Vanilla tqdm interpretsdisable=Noneas "checksys.stderr.isatty()" →Falsein non-TTY → setsself.disable = True→update()becomes a no-op. No crash, but progress is silently lost:Note
Medium Risk
Touches shared progress-bar creation used by downloads; behavior changes could affect progress visibility/TTY handling for some callers, but has no security or data-impacting logic.
Overview
Fixes custom
tqdm_classhandling so non-HF progress bar implementations aren’t silently disabled in non-TTY environments or crashed by HF-specific kwargs.Adds
_create_progress_barinutils/tqdm.pyand routes_get_progress_bar_contextandsnapshot_download’s aggregated bytes bar through it, only applying HF disable/namebehavior when the class is the Hub’stqdmsubclass (or a subclass). Includes new regression tests covering vanilla/custom tqdm behavior and ensuringHF_HUB_DISABLE_PROGRESS_BARS/namedon’t leak into custom classes.Reviewed by Cursor Bugbot for commit 5d059bf. Bugbot is set up for automated code reviews on this repo. Configure here.