Studio: split vision-cache exception test to match transient vs permanent (#5145)

`TestVisionCacheOnException::test_exception_result_cached` currently
patches `load_model_config` with `side_effect=OSError("network down")`
and asserts `assert_called_once()`. That assertion is impossible by
design: `_is_vision_model_uncached` in
`studio/backend/utils/models/model_config.py` intentionally returns
`None` for `OSError` so `is_vision_model` does not cache the fallback
and retries on the next call. The module docstring on
`_vision_detection_cache` itself spells this out:

    Only definitive results (True/False from successful detection) are
    cached; transient failures (network errors, timeouts) are NOT
    cached so they can be retried.

The test has been failing identically on every downstream review run
against `unslothai/unsloth` main (e.g. `unsloth#5115`, `unsloth#5080`),
but the failure is not introduced by any of those PRs and does not
gate correctness.

Fix the collision by splitting the class into the two contracts the
code actually implements:

1. `test_permanent_exception_result_cached` keeps the original
   intent ("exception falls back to False and that False is cached")
   but uses `ValueError`, which is one of the exception types
   `_is_vision_model_uncached` treats as permanent and caches. No
   `huggingface_hub` import needed.

2. `test_transient_exception_not_cached` pins the opposite contract
   with the original `OSError("network down")`: the call returns
   False but the second invocation re-runs detection
   (`call_count == 2`). This guards against a future regression
   where somebody caches transient failures and then users with a
   flaky network permanently see wrong detection for a model.

Both tests use `assert ... is False` on the public API and mock-count
assertions on `load_model_config`; no private helpers are touched.
This commit is contained in:
Daniel Han 2026-04-23 00:22:40 -07:00 committed by GitHub
commit 41a6cc8692
No known key found for this signature in database
GPG key ID: B5690EEEBB952194

View file

@ -124,23 +124,50 @@ class TestVisionCacheSubprocessPath:
class TestVisionCacheOnException:
"""When detection raises an exception, _is_vision_model_uncached catches
it and returns False. That False must be cached so subsequent calls don't
retry and fail again."""
"""When detection raises an exception, _is_vision_model_uncached
distinguishes permanent failures (cached as False) from transient
failures (returned as None, not cached so the next call can retry).
Verify both contracts."""
@patch(
"utils.models.model_config.load_model_config",
side_effect = ValueError("bad config"),
)
@patch("utils.transformers_version.needs_transformers_5", return_value = False)
def test_permanent_exception_result_cached(self, mock_needs_t5, mock_load_config):
"""A permanent failure (ValueError / RepositoryNotFoundError /
GatedRepoError / JSONDecodeError) should be caught, return False,
and that False should be cached so subsequent calls don't retry.
ValueError is used here because it's the simplest of the
code-path's cacheable exception types and does not require an
import of huggingface_hub errors (whose module path varies
across versions)."""
# First call: load_model_config raises -> except branch -> False.
assert is_vision_model("broken/model") is False
# Second call: cache hit, load_model_config not called again.
assert is_vision_model("broken/model") is False
mock_load_config.assert_called_once()
@patch(
"utils.models.model_config.load_model_config",
side_effect = OSError("network down"),
)
@patch("utils.transformers_version.needs_transformers_5", return_value = False)
def test_exception_result_cached(self, mock_needs_t5, mock_load_config):
"""A real exception inside _is_vision_model_uncached should be caught,
return False, and that False should be cached for subsequent calls."""
# First call: load_model_config raises → except branch → False
def test_transient_exception_not_cached(self, mock_needs_t5, mock_load_config):
"""A transient failure (OSError, timeouts) should return None from
_is_vision_model_uncached, surface as False to the caller, and
NOT be cached, so the next call retries detection. This matches
the documented behaviour on _vision_detection_cache:
'transient failures (network errors, timeouts) are NOT cached so
they can be retried.'"""
# First call: load_model_config raises OSError -> uncached None
# -> caller returns False without caching.
assert is_vision_model("broken/model") is False
# Second call: cache hit, load_model_config not called again
# Second call: cache miss again, load_model_config called a
# second time.
assert is_vision_model("broken/model") is False
mock_load_config.assert_called_once()
assert mock_load_config.call_count == 2
# ---------------------------------------------------------------------------