mirror of
https://github.com/jamiepine/voicebox.git
synced 2026-09-12 19:30:38 -07:00
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]>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
de15d8fdc6
commit
79b70b8970
@@ -5,6 +5,11 @@ Verifies that the helper mutates the cached module constants in
|
||||
``huggingface_hub.constants`` and ``transformers.utils.hub`` — not just
|
||||
``os.environ`` — and that concurrent users are refcount-coordinated so
|
||||
one thread's exit can't strip another thread's offline protection.
|
||||
|
||||
NOTE: These tests mutate process-global state in ``huggingface_hub.constants``
|
||||
and ``transformers.utils.hub``. They are not safe under cross-process
|
||||
parallelism (e.g. ``pytest-xdist`` with ``--dist=loadfile``/``loadscope``);
|
||||
run this file serially.
|
||||
"""
|
||||
|
||||
import os
|
||||
@@ -36,27 +41,27 @@ def test_mutates_cached_huggingface_hub_constant():
|
||||
original = _hf_const().HF_HUB_OFFLINE
|
||||
with force_offline_if_cached(True, "t"):
|
||||
assert _hf_const().HF_HUB_OFFLINE is True
|
||||
assert _hf_const().HF_HUB_OFFLINE == original
|
||||
assert original == _hf_const().HF_HUB_OFFLINE
|
||||
|
||||
|
||||
def test_mutates_cached_transformers_constant():
|
||||
original = _tf_hub()._is_offline_mode
|
||||
with force_offline_if_cached(True, "t"):
|
||||
assert _tf_hub()._is_offline_mode is True
|
||||
assert _tf_hub()._is_offline_mode == original
|
||||
assert original == _tf_hub()._is_offline_mode
|
||||
|
||||
|
||||
def test_sets_env_variable():
|
||||
original = os.environ.get("HF_HUB_OFFLINE")
|
||||
with force_offline_if_cached(True, "t"):
|
||||
assert os.environ.get("HF_HUB_OFFLINE") == "1"
|
||||
assert os.environ.get("HF_HUB_OFFLINE") == original
|
||||
assert "1" == os.environ.get("HF_HUB_OFFLINE")
|
||||
assert original == os.environ.get("HF_HUB_OFFLINE")
|
||||
|
||||
|
||||
def test_noop_when_not_cached():
|
||||
before = _hf_const().HF_HUB_OFFLINE
|
||||
with force_offline_if_cached(False, "t"):
|
||||
assert _hf_const().HF_HUB_OFFLINE == before
|
||||
assert before == _hf_const().HF_HUB_OFFLINE
|
||||
|
||||
|
||||
def test_nested_contexts_respect_refcount():
|
||||
@@ -67,7 +72,7 @@ def test_nested_contexts_respect_refcount():
|
||||
assert _hf_const().HF_HUB_OFFLINE is True
|
||||
# inner exit must not restore while outer is still active
|
||||
assert _hf_const().HF_HUB_OFFLINE is True
|
||||
assert _hf_const().HF_HUB_OFFLINE == original
|
||||
assert original == _hf_const().HF_HUB_OFFLINE
|
||||
|
||||
|
||||
def test_concurrent_threads_share_offline_window():
|
||||
@@ -101,8 +106,8 @@ def test_concurrent_threads_share_offline_window():
|
||||
t_fast.join(timeout=5)
|
||||
|
||||
assert not errors, errors
|
||||
assert observations == [True], "slow thread lost offline protection"
|
||||
assert _hf_const().HF_HUB_OFFLINE == original
|
||||
assert [True] == observations, "slow thread lost offline protection"
|
||||
assert original == _hf_const().HF_HUB_OFFLINE
|
||||
|
||||
|
||||
if __name__ == "__main__":
|
||||
|
||||
@@ -54,23 +54,59 @@ def force_offline_if_cached(is_cached: bool, model_label: str = ""):
|
||||
|
||||
with _offline_lock:
|
||||
if _offline_refcount == 0:
|
||||
_saved_env = os.environ.get("HF_HUB_OFFLINE")
|
||||
# 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:
|
||||
import huggingface_hub.constants as hf_const
|
||||
try:
|
||||
import huggingface_hub.constants as hf_const
|
||||
|
||||
_saved_hf_const = hf_const.HF_HUB_OFFLINE
|
||||
hf_const.HF_HUB_OFFLINE = True
|
||||
except ImportError:
|
||||
_saved_hf_const = None
|
||||
try:
|
||||
import transformers.utils.hub as tf_hub
|
||||
prev_hf = hf_const.HF_HUB_OFFLINE
|
||||
hf_const.HF_HUB_OFFLINE = True
|
||||
except ImportError:
|
||||
prev_hf = None
|
||||
|
||||
_saved_transformers_const = tf_hub._is_offline_mode
|
||||
tf_hub._is_offline_mode = True
|
||||
except ImportError:
|
||||
_saved_transformers_const = None
|
||||
try:
|
||||
import transformers.utils.hub as tf_hub
|
||||
|
||||
os.environ["HF_HUB_OFFLINE"] = "1"
|
||||
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",
|
||||
|
||||
Reference in New Issue
Block a user