mirror of
https://github.com/jamiepine/voicebox.git
synced 2026-09-18 06:10:43 -07:00
fix(offline): guard inference paths with HF_HUB_OFFLINE (#503)
* fix(offline): guard inference paths with HF_HUB_OFFLINE (#462) PR #443 wrapped the model *load* path with `force_offline_if_cached` so cached models don't phone home at startup. The context manager restores `HF_HUB_OFFLINE` on exit, which left inference paths (generate, transcribe, voice-prompt creation) unguarded — and `qwen_tts`, `mlx_audio`, and `transformers` perform lazy tokenizer/processor/config lookups during inference. With internet on, those lookups are near-instant and invisible; with internet off, `requests` hangs on DNS or connect until the network returns. This is exactly what users in #462 describe: model shows "Loaded", internet drops, generation "thinks" forever, internet comes back, generation completes. Chatterbox and LuxTTS don't exhibit this because their engine libs resolve everything through already-cached paths at load time. Fix: wrap each inference-sync body with `force_offline_if_cached(True, ...)`. Since inference only runs after a successful load, weights are known to be on disk, so `is_cached=True` is unconditional. Also adds the load-time guard that was missing from `qwen_custom_voice_backend.py` — CustomVoice previously had no offline protection at all. Paths patched: - PyTorchTTSBackend.create_voice_prompt (create_voice_clone_prompt) - PyTorchTTSBackend.generate (generate_voice_clone) - PyTorchSTTBackend.transcribe (Whisper generate + decoder-prompt-ids) - MLXTTSBackend.generate (mlx_audio generate, all branches) - MLXSTTBackend.transcribe (mlx_audio whisper generate) - QwenCustomVoiceBackend._load_model_sync + generate Does not address the secondary `check_model_inputs() missing 'func'` error reported in the same issue — that's a `transformers` 5.x version-skew bug on the install path, separate concern. Fixes #462. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(offline): mutate cached HF constants + threadsafe refcount Review feedback on the initial fix surfaced two real issues: 1. ``os.environ`` toggles alone don't flip offline mode. ``huggingface_hub.constants.HF_HUB_OFFLINE`` is read once at import time into a module-level bool; ``transformers.utils.hub._is_offline_mode`` mirrors that bool at its own import time. The hot paths (``_http._default_backend_factory`` in huggingface_hub, ``is_offline_mode`` in transformers) read the cached bools — not the env — so mutating only ``os.environ`` was a no-op. 2. Race condition on concurrent inference. Two threads running inside ``force_offline_if_cached`` via ``asyncio.to_thread`` could have thread A's ``finally`` strip thread B's offline protection mid-run. Rewrite the helper to: - mutate ``huggingface_hub.constants.HF_HUB_OFFLINE`` and ``transformers.utils.hub._is_offline_mode`` directly - refcount concurrent users under a single ``threading.RLock`` so a shared offline window is restored only when the last caller exits - still write ``os.environ`` for anything that reads it dynamically Also addresses the unused-variable ruff flag on the Whisper transcribe path (``audio, sr`` → ``audio, _sr``). New unit tests cover the cached-constant mutation, env propagation, no-op on ``is_cached=False``, nested contexts, and a threaded race where a slow thread must retain offline mode after a peer exits. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * fix(offline): atomic entry rollback + tidy test assertions Review follow-up: - Wrap the `_offline_refcount == 0` setup in a try/except so any failure during the cached-constant mutation (including unexpected non-ImportError like RuntimeError or AttributeError from a half-initialized module) rolls back *all* partial state before re-raising. Without this, a mid-setup crash could leave `huggingface_hub.constants.HF_HUB_OFFLINE` mutated but the refcount at 0 — a persistent offline flag outliving the process. - Swap ruff-flagged Yoda comparisons in the new test file (SIM300) and add a module-level note warning that these tests mutate global state and are not safe under cross-process parallelism. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> * test(offline): make concurrency test deterministic and bounded Replace the `sleep(0.15)` ordering hack with an explicit `threading.Event` the fast thread sets in `finally`. The slow thread waits on that event (bounded), then observes the flag — so we deterministically verify the slow thread still sees offline mode after the fast thread has exited. Also add timeouts to `barrier.wait()` and assert `not thread.is_alive()` after the joins so the test can't hang on an unexpected failure path. Co-Authored-By: Claude Opus 4.7 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.7 (1M context) <[email protected]>
This commit is contained in:
Jamie Pine
co-authored by
Claude Opus 4.7
Claude Opus 4.7
parent
5964af5dea
commit
5aa1677a25
@@ -6,6 +6,7 @@ are already downloaded. Must be imported BEFORE mlx_audio.
|
||||
|
||||
import logging
|
||||
import os
|
||||
import threading
|
||||
from contextlib import contextmanager
|
||||
from pathlib import Path
|
||||
from typing import Optional, Union
|
||||
@@ -13,13 +14,33 @@ from typing import Optional, Union
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
|
||||
# huggingface_hub reads ``HF_HUB_OFFLINE`` once at import time into
|
||||
# ``huggingface_hub.constants.HF_HUB_OFFLINE``; transformers mirrors that into
|
||||
# ``transformers.utils.hub._is_offline_mode`` at *its* import time. Toggling
|
||||
# ``os.environ`` after either module is imported does not flip those cached
|
||||
# bools, and the hot paths (``_http._default_backend_factory``,
|
||||
# ``transformers.utils.hub.is_offline_mode``) read the bools — not the env.
|
||||
# We mutate the cached constants directly, guarded by a refcount so
|
||||
# concurrent inference threads share a single offline window safely.
|
||||
|
||||
_offline_lock = threading.RLock()
|
||||
_offline_refcount = 0
|
||||
_saved_env: Optional[str] = None
|
||||
_saved_hf_const: Optional[bool] = None
|
||||
_saved_transformers_const: Optional[bool] = None
|
||||
|
||||
|
||||
@contextmanager
|
||||
def force_offline_if_cached(is_cached: bool, model_label: str = ""):
|
||||
"""Context manager that sets ``HF_HUB_OFFLINE=1`` while loading a cached model.
|
||||
"""Force offline mode for the duration of a cached-model operation.
|
||||
|
||||
Flips ``HF_HUB_OFFLINE`` in the process env **and** in the cached bools
|
||||
inside ``huggingface_hub.constants`` / ``transformers.utils.hub`` so HTTP
|
||||
adapters and offline-mode checks actually see the change. Uses a refcount
|
||||
so multiple concurrent inference threads share a single offline window
|
||||
and the last one to exit restores state.
|
||||
|
||||
If *is_cached* is ``False`` the block runs normally (network allowed).
|
||||
If the offline load raises an error containing "offline" we automatically
|
||||
retry with network access so a partially-cached model still works.
|
||||
|
||||
Args:
|
||||
is_cached: Whether the model weights are already on disk.
|
||||
@@ -29,34 +50,96 @@ def force_offline_if_cached(is_cached: bool, model_label: str = ""):
|
||||
yield
|
||||
return
|
||||
|
||||
original_value = os.environ.get("HF_HUB_OFFLINE")
|
||||
os.environ["HF_HUB_OFFLINE"] = "1"
|
||||
logger.info(
|
||||
"[offline-guard] %s is cached — forcing HF_HUB_OFFLINE=1",
|
||||
model_label or "model",
|
||||
)
|
||||
global _offline_refcount, _saved_env, _saved_hf_const, _saved_transformers_const
|
||||
|
||||
with _offline_lock:
|
||||
if _offline_refcount == 0:
|
||||
# Snapshot prior state, apply new state, roll back on *any*
|
||||
# failure. Catching only ImportError here would let a partially
|
||||
# broken install (RuntimeError, AttributeError from a half-init
|
||||
# module, etc.) leave the cached HF constants mutated without
|
||||
# bumping the refcount — a persistent offline leak that outlives
|
||||
# the process and is miserable to debug.
|
||||
prev_env = os.environ.get("HF_HUB_OFFLINE")
|
||||
prev_hf: Optional[bool] = None
|
||||
prev_tf: Optional[bool] = None
|
||||
try:
|
||||
try:
|
||||
import huggingface_hub.constants as hf_const
|
||||
|
||||
prev_hf = hf_const.HF_HUB_OFFLINE
|
||||
hf_const.HF_HUB_OFFLINE = True
|
||||
except ImportError:
|
||||
prev_hf = None
|
||||
|
||||
try:
|
||||
import transformers.utils.hub as tf_hub
|
||||
|
||||
prev_tf = tf_hub._is_offline_mode
|
||||
tf_hub._is_offline_mode = True
|
||||
except ImportError:
|
||||
prev_tf = None
|
||||
|
||||
os.environ["HF_HUB_OFFLINE"] = "1"
|
||||
except BaseException:
|
||||
# Roll back whatever we already changed, then re-raise so
|
||||
# the caller sees the real failure.
|
||||
if prev_hf is not None:
|
||||
try:
|
||||
import huggingface_hub.constants as hf_const
|
||||
|
||||
hf_const.HF_HUB_OFFLINE = prev_hf
|
||||
except ImportError:
|
||||
pass
|
||||
if prev_tf is not None:
|
||||
try:
|
||||
import transformers.utils.hub as tf_hub
|
||||
|
||||
tf_hub._is_offline_mode = prev_tf
|
||||
except ImportError:
|
||||
pass
|
||||
if prev_env is not None:
|
||||
os.environ["HF_HUB_OFFLINE"] = prev_env
|
||||
else:
|
||||
os.environ.pop("HF_HUB_OFFLINE", None)
|
||||
raise
|
||||
|
||||
_saved_env = prev_env
|
||||
_saved_hf_const = prev_hf
|
||||
_saved_transformers_const = prev_tf
|
||||
logger.info(
|
||||
"[offline-guard] %s is cached — forcing offline mode",
|
||||
model_label or "model",
|
||||
)
|
||||
_offline_refcount += 1
|
||||
|
||||
try:
|
||||
yield
|
||||
except Exception as exc:
|
||||
if "offline" in str(exc).lower():
|
||||
logger.warning(
|
||||
"[offline-guard] Offline load failed for %s, retrying with network: %s",
|
||||
model_label or "model",
|
||||
exc,
|
||||
)
|
||||
# Restore original env and retry — caller must wrap the load
|
||||
# inside force_offline_if_cached so retrying here isn't possible.
|
||||
# Instead, propagate a flag via the exception so the caller can
|
||||
# decide. For simplicity we just let it fall through to the
|
||||
# finally block and re-raise.
|
||||
raise
|
||||
raise
|
||||
finally:
|
||||
if original_value is not None:
|
||||
os.environ["HF_HUB_OFFLINE"] = original_value
|
||||
else:
|
||||
os.environ.pop("HF_HUB_OFFLINE", None)
|
||||
with _offline_lock:
|
||||
_offline_refcount -= 1
|
||||
if _offline_refcount == 0:
|
||||
if _saved_env is not None:
|
||||
os.environ["HF_HUB_OFFLINE"] = _saved_env
|
||||
else:
|
||||
os.environ.pop("HF_HUB_OFFLINE", None)
|
||||
if _saved_hf_const is not None:
|
||||
try:
|
||||
import huggingface_hub.constants as hf_const
|
||||
|
||||
hf_const.HF_HUB_OFFLINE = _saved_hf_const
|
||||
except ImportError:
|
||||
pass
|
||||
if _saved_transformers_const is not None:
|
||||
try:
|
||||
import transformers.utils.hub as tf_hub
|
||||
|
||||
tf_hub._is_offline_mode = _saved_transformers_const
|
||||
except ImportError:
|
||||
pass
|
||||
_saved_env = None
|
||||
_saved_hf_const = None
|
||||
_saved_transformers_const = None
|
||||
|
||||
|
||||
def patch_huggingface_hub_offline():
|
||||
|
||||
Reference in New Issue
Block a user