fix(backends): surface real cause on HF model load failures

[shingonoide]

Summary

Replaces the single catch-all except Exception in every backend's load() method with a shared context manager mapping HuggingFace Hub failures to targeted, actionable error messages. Applies uniformly across object_detection, image_classification, grounding_dino, and owlvit.

Exception branches (in catch order)

1. ImportError → _MISSING_DEP_HINTS allowlist (timm, sentencepiece, detectron2, torchvision) with dep-specific install hints; unknown deps re-raised unchanged.
2. GatedRepoError → "accept license with an authenticated token (set HF_TOKEN)" (caught before RepositoryNotFoundError, its parent class).
3. RepositoryNotFoundError → model ID / HF_TOKEN hint.
4. RevisionNotFoundError → revision (commit SHA, tag, or branch) hint.
5. LocalEntryNotFoundError / EntryNotFoundError → offline or shared-cache race hint (pre-download model, or retry after the first worker finishes).
6. HfHubHTTPError → download failure. If response.status_code == 401 a dedicated gated-repo / HF_TOKEN hint fires (older hub versions return 401 without raising GatedRepoError). Other codes keep the generic download message.
7. ValueError → generic "could not be loaded" with repr(e), no architecture-head assumption.
8. Exception (fallback) → generic message always including repr(e).

All raises use from e to preserve the original traceback.

Shared plumbing

• filter_huggingface_vision/backends/_hf_load_errors.py hosts the hf_load_error_handler(model_id, revision, task) context manager so future hub error types only need to be added in one place.
• Imports from huggingface_hub.utils (stable across huggingface-hub>=0.23) rather than the huggingface_hub.errors submodule (which only exposes these attributes from ~0.30+).
• tests/_hf_test_utils.py exposes make_hf_error(cls, message, status_code=404) — a single helper both backends' test suites share.
• pyproject.toml declares [tool.pytest.ini_options] pythonpath = ["tests"] so the bare from _hf_test_utils import make_hf_error is documented via pytest config rather than relying on prepend import mode implicitly.

Context: a companion PR on the plainsight-api side adds revision and detection_type to GT Automation templates, which was one of the root causes of the "not compatible" errors. This PR ensures that if any of those fields are still wrong — or any other Hub failure mode fires — the operator sees an actionable error instead of a generic "not compatible".

Follow-up filed separately (not in this PR): retry/backoff on transient 429 / 5xx at load time — scoped to reliability rather than error surface quality. Tracked in PLAT-962.

Test plan

• TestObjectDetectionBackendLoadErrors — one test per exception branch, all passing
• TestImageClassificationBackendLoadErrors — parallel coverage for the classification backend
• TestGroundingDinoBackendLoadErrors — load-error coverage for the grounding-dino backend
• TestOwlVitBackendLoadErrors — load-error coverage for the owl-vit backend
• pytest tests/ green
• flake8 reports no issues on all modified files
• DCO Signed-off-by trailer present on every commit

[lucasmundim]

Solid PR — the exception hierarchy ordering is correct (verified against the live huggingface_hub MRO: GatedRepoError → RepositoryNotFoundError → HfHubHTTPError → OSError → Exception), all raises chain via from e, and messages are actionable. Test coverage is thorough with one test per branch plus chain-cause verification.

One minor suggestion inline.

[lucasmundim]

Nit: every test in this class patches AutoImageProcessor.from_pretrained (the first call in the try block). Since both from_pretrained calls sit in the same try, the handlers work identically regardless of which call raises — so this is correct today.

However, if someone later refactors the try block (e.g. splits processor and model loading into separate try/except), the model-download error path would be untested. Consider adding one test (e.g. for RepositoryNotFoundError) that patches AutoModelForImageClassification.from_pretrained instead, to anchor both paths. Same applies to the object-detection test file.

Low priority — not blocking.

[shingonoide]

Good catch — applied in c3ecda6. Added one model-side patch test per file (test_repository_not_found_on_model_download_gives_actionable_message) so both from_pretrained call sites stay covered if the try/except ever gets split.

[Copilot]

Pull request overview

Improves debuggability of HuggingFace model load failures by surfacing specific, actionable root-cause errors (Hub auth/gating, missing repo/revision, HTTP failures, architecture mismatches) in the vision backends, with parallel unit-test coverage and a lockfile sync.

Changes:

• Add structured exception handling in ObjectDetectionBackend.load() and ImageClassificationBackend.load() to emit targeted error messages while preserving chained tracebacks (raise ... from e).
• Add unit tests covering each error branch for both backends.
• Sync uv.lock to match the existing openfilter[all]>=0.1.27 pin in pyproject.toml.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
filter_huggingface_vision/backends/object_detection.py	Adds granular HuggingFace Hub + ValueError/fallback error handling for object detection model loads.
filter_huggingface_vision/backends/image_classification.py	Adds parallel granular error handling for image classification model loads.
tests/test_object_detection.py	Adds targeted tests validating the new object detection load error messages and exception chaining.
tests/test_image_classification.py	Adds targeted tests validating the new image classification load error messages and exception chaining.
uv.lock	Updates the locked openfilter[all] specifier to >=0.1.27 to match pyproject.toml.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[shingonoide]

Round 1 reply:

• @lucasmundim suggestion: applied. Added test_repository_not_found_on_model_download_gives_actionable_message in both test files (commit c3ecda6) to anchor both from_pretrained call sites.

[leandrobmarinho]

Code Review

Overview

This PR significantly improves the developer experience by replacing a single catch-all except Exception in both backend load() methods with eight targeted exception branches. Errors that were previously opaque ("not compatible with ...") now surface actionable messages — especially valuable for GT Automation operators hitting misconfigured model IDs, gated repos, or wrong revision pins.

The exception ordering is correct: GatedRepoError is caught before RepositoryNotFoundError (which is its parent in huggingface_hub ≥ 1.10). Test coverage is thorough, all 9 branches per backend are exercised.

---

Issues / Suggestions

1. Duplicated exception-handling logic between backends (maintainability risk)

The entire exception block in object_detection.py and image_classification.py is nearly identical. If a new HF Hub exception appears in a future huggingface_hub release (e.g. EntryNotFoundError, which already exists), both files must be updated in sync. A shared context manager or utility function would eliminate this drift risk:

# backends/_hf_load_errors.py
from contextlib import contextmanager
from huggingface_hub import errors as _hf_errors

@contextmanager
def hf_load_error_handler(model_id: str, revision: str, task: str):
    try:
        yield
    except ImportError as e:
        if "timm" in str(e).lower():
            raise ImportError(
                f"This model (e.g. {model_id}) requires the timm library. "
                "Install it with: pip install timm"
            ) from e
        raise
    except _hf_errors.GatedRepoError as e:
        raise RuntimeError(
            f"Access to '{model_id}' requires accepting its license on the Hub. "
            "Accept it with an authenticated token."
        ) from e
    # ... rest of branches
    except Exception as e:
        raise RuntimeError(
            f"Unexpected failure loading '{model_id}@{revision}' for {task}: {repr(e)}"
        ) from e

Then both load() methods become:

with hf_load_error_handler(model_id, revision, "image classification"):
    self._processor = AutoImageProcessor.from_pretrained(...)
    self._model = AutoModelForImageClassification.from_pretrained(...)

2. Inconsistent repr(e) inclusion in error messages

HfHubHTTPError, ValueError, and the fallback Exception include repr(e) in the message, but GatedRepoError, RepositoryNotFoundError, and RevisionNotFoundError do not. The original exception details (e.g. the specific 401/403 response body from HF Hub) can be useful when the structured message alone isn't enough to diagnose the issue.

Suggest adding {repr(e)} to those three branches for consistency:

except _hf_errors.GatedRepoError as e:
    raise RuntimeError(
        f"Access to '{model_id}' requires accepting its license on the Hub. "
        f"Accept it with an authenticated token. Detail: {repr(e)}"
    ) from e

3. Tests only mock AutoImageProcessor.from_pretrained, not AutoModel*.from_pretrained

All test cases inject exceptions via AutoImageProcessor.from_pretrained. However, the same HF Hub errors (RepositoryNotFoundError, RevisionNotFoundError, etc.) can also be raised by AutoModelForImageClassification.from_pretrained / AutoModelForObjectDetection.from_pretrained — e.g., when the processor is cached locally but the model weights are not. The exception handlers cover both calls, but the tests don't verify the model-loading half.

This is a low-risk gap for now (the handlers are identical), but worth noting so a future refactor doesn't accidentally break the second from_pretrained call.

4. _make_hf_error helper is duplicated across test classes

TestImageClassificationBackendLoadErrors._make_hf_error and TestObjectDetectionBackendLoadErrors._make_hf_error are identical. A module-level helper or a shared BaseHFLoadErrorTests mixin would eliminate the duplication:

def _make_hf_error(cls, message):
    mock_response = MagicMock()
    mock_response.status_code = 404
    return cls(message, response=mock_response)

5. uv.lock manual edit concern

The uv.lock change updates the requires-dist constraint for openfilter from >=0.1.26 to >=0.1.27. Lock files should generally be regenerated via uv lock rather than hand-edited to avoid subtle inconsistencies between the resolved package graph and what's recorded. If this is intentional (pyproject.toml was already bumped separately), it's fine — but it's worth confirming that uv lock --check passes after this change.

---

Summary

Core logic	Correct — exception ordering, from e chaining, and message content are all sound
Code duplication	Moderate concern — two identical 30-line blocks will diverge over time
Test coverage	Good overall; minor gap on the model-weights loading path
repr(e) consistency	Minor — three branches omit it while others include it
uv.lock	Low risk but verify uv lock --check passes

The approach is solid and the improvement to operator experience is clear. Addressing the duplication (issue 1) would be the highest-value change before merging; the rest are polish.

[leandrobmarinho]

Inconsistent repr(e) in error messages

HfHubHTTPError, ValueError, and the fallback Exception include repr(e), but GatedRepoError, RepositoryNotFoundError, and RevisionNotFoundError (this line and the two below) do not. The original HF Hub response body — e.g. a 401/403 payload — can be valuable when the structured message alone is not enough.

Consider adding it for consistency:

except _hf_errors.GatedRepoError as e:
    raise RuntimeError(
        f"Access to '{model_id}' requires accepting its license on the Hub. "
        f"Accept it with an authenticated token. Detail: {repr(e)}"
    ) from e

Same applies to RepositoryNotFoundError and RevisionNotFoundError.

[shingonoide]

Addressed in 42d6b89: the shared hf_load_error_handler context manager now carries repr(e) on every branch (including GatedRepoError, RepositoryNotFoundError, RevisionNotFoundError). Thanks for the catch.

[leandrobmarinho]

Duplicated exception-handling block — consider a shared context manager

This entire except ladder is nearly identical to the one in object_detection.py. If a new HF Hub exception needs to be added in the future (e.g. EntryNotFoundError, which already exists in huggingface_hub), both files must be updated in sync — a common source of drift.

A shared helper in backends/_hf_load_errors.py (or similar) would centralise this logic:

from contextlib import contextmanager
from huggingface_hub import errors as _hf_errors

@contextmanager
def hf_load_error_handler(model_id: str, revision: str, task: str):
    try:
        yield
    except ImportError as e:
        if "timm" in str(e).lower():
            raise ImportError(
                f"This model (e.g. {model_id}) requires the timm library. "
                "Install it with: pip install timm"
            ) from e
        raise
    except _hf_errors.GatedRepoError as e:
        raise RuntimeError(
            f"Access to '{model_id}' requires accepting its license on the Hub. "
            "Accept it with an authenticated token."
        ) from e
    # … other branches …
    except Exception as e:
        raise RuntimeError(
            f"Unexpected failure loading '{model_id}@{revision}' for {task}: {repr(e)}"
        ) from e

Then both load() methods collapse to:

with hf_load_error_handler(model_id, revision, "image classification"):
    self._processor = AutoImageProcessor.from_pretrained(...)
    self._model = AutoModelForImageClassification.from_pretrained(...)

Not a blocker, but worth addressing before the next time this block needs to change.

[shingonoide]

Applied in 42d6b89: extracted filter_huggingface_vision/backends/_hf_load_errors.py with a hf_load_error_handler(model_id, revision, task) context manager. Both object_detection and image_classification now delegate to it, and round 5 (1f3de3d) extends the same wrapper to grounding_dino and owlvit.

[leandrobmarinho]

Same repr(e) inconsistency as in image_classification.py

GatedRepoError, RepositoryNotFoundError, and RevisionNotFoundError drop the original exception detail. Adding {repr(e)} to their messages (as done for HfHubHTTPError and the fallback) keeps error reporting consistent and preserves HF Hub response bodies in logs.

Also mirrors the duplication comment on the classification backend — this block is a copy of the one there.

[shingonoide]

Same as the classification-side thread — repr(e) is now consistent across every branch after the shared handler extraction in 42d6b89.

[leandrobmarinho]

_make_hf_error is duplicated in test_object_detection.py

Both test classes define the identical helper. A module-level function or a shared BaseHFLoadErrorTests mixin would remove the duplication:

# at module level, before the test classes
def _make_hf_error(cls, message):
    mock_response = MagicMock()
    mock_response.status_code = 404
    return cls(message, response=mock_response)

Alternatively, both test classes could inherit from a common base that provides this method. Minor, but avoids the two copies drifting if the constructor signature changes in a future huggingface_hub release.

[shingonoide]

Applied in 42d6b89: extracted tests/_hf_test_utils.py with a shared make_hf_error(cls, message, status_code=404) helper. Both test files import it instead of defining local copies.

[leandrobmarinho]

Duplicate of _make_hf_error defined in test_image_classification.py

Same note as the classification test: this helper is byte-for-byte identical in both files. Moving it to a shared location removes the duplication.

[shingonoide]

Same fix — _make_hf_error moved to tests/_hf_test_utils.py in 42d6b89.

[leandrobmarinho]

Lock file manually edited — consider regenerating via uv lock

The requires-dist specifier for openfilter was changed from >=0.1.26 to >=0.1.27 by hand. Lock files are generated artifacts; editing them directly can leave the recorded dependency graph inconsistent with what uv lock would produce.

If pyproject.toml already has >=0.1.27, running uv lock (or uv lock --check to verify) is the safer path. If this was done intentionally because the uv version used in CI cannot regenerate the file cleanly, a note in the PR description explaining why would help future readers.

[shingonoide]

Thanks — to clarify, no manual edit here. The main branch's pyproject.toml already pinned openfilter[all]>=0.1.27 (see pyproject.toml:28 on main), but uv.lock on main was still at >=0.1.26 — it had drifted out of sync. When I ran uv sync to set up the environment, uv regenerated the lockfile to match pyproject.toml, and I'm shipping that sync in this PR rather than leaving main drifted. uv lock --check passes on the branch.

[shingonoide]

Just to make the earlier reply crisper: the uv.lock change wasn't a manual edit. It was produced automatically by uv sync reconciling the lockfile with pyproject.toml (which on main already pins openfilter[all]>=0.1.27). So uv itself made the adjustment, we're just shipping the sync that should already have been on main.

[shingonoide]

Round 2 reply to @leandrobmarinho (commit e67b1ef):

1. Shared exception handler — applied. Extracted hf_load_error_handler into filter_huggingface_vision/backends/_hf_load_errors.py and rewrote both backends to use it via with hf_load_error_handler(model_id, revision, task):. Both files now share the exact same error mapping; future huggingface_hub error types only need to be added in one place.

2. repr(e) inclusion — applied. GatedRepoError, RepositoryNotFoundError, and RevisionNotFoundError now include Detail: {repr(e)} for consistency with the HTTP/ValueError/fallback branches.

3. Model-side patch coverage — addressed in the previous round (commit c3ecda6) per @lucasmundim's inline nit: added test_repository_not_found_on_model_download_gives_actionable_message in both test files to anchor the second from_pretrained call.

4. _make_hf_error helper deduplication — applied. Moved to tests/_hf_test_utils.py as a module-level make_hf_error function; both test files import from there.

5. uv.lock concern — the file was regenerated by uv sync, not hand-edited. The change was already present in the lockfile's requires-dist block once pyproject.toml's openfilter[all]>=0.1.27 pin was resolved.

[Copilot]

Pull request overview

This PR improves debuggability of HuggingFace model load failures in the vision backends by replacing a generic catch-all with structured, actionable exception handling shared via a new context manager.

Changes:

• Added a shared hf_load_error_handler context manager that maps common HF Hub/load failures to clearer, chained exceptions.
• Updated ObjectDetectionBackend.load() and ImageClassificationBackend.load() to use the shared handler.
• Added/expanded unit tests to cover each error branch and synced uv.lock/release metadata for the new version.

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
filter_huggingface_vision/backends/_hf_load_errors.py	New shared context manager implementing structured HF load error handling.
filter_huggingface_vision/backends/object_detection.py	Swaps inline try/except for the shared load error handler.
filter_huggingface_vision/backends/image_classification.py	Swaps inline try/except for the shared load error handler.
tests/_hf_test_utils.py	Adds shared helper for constructing HF Hub exceptions in tests.
tests/test_object_detection.py	Adds backend load-error tests for object detection.
tests/test_image_classification.py	Adds backend load-error tests for image classification.
uv.lock	Synces openfilter[all] to >=0.1.27 per pyproject.toml.
VERSION	Bumps version to v0.4.5.
RELEASE.md	Adds v0.4.5 release notes describing the new error handling and tests.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[shingonoide]

Good catch — applied in daa596b. Added huggingface-hub>=0.23 to pyproject.toml (and regenerated uv.lock via uv lock). 0.23 is the floor that has the huggingface_hub.errors module layout used here (GatedRepoError, RevisionNotFoundError, RepositoryNotFoundError, HfHubHTTPError all present).

[shingonoide]

Addressed in commit 6ed6413. Instead of bumping the floor, I switched the import from huggingface_hub.errors to huggingface_hub.utils — that path has been stable since hub ~0.16 and is available at the declared floor (>=0.23). Every exception class we need (GatedRepoError, RepositoryNotFoundError, RevisionNotFoundError, HfHubHTTPError, EntryNotFoundError, LocalEntryNotFoundError) is importable from huggingface_hub.utils across the whole supported range. Floor in pyproject.toml is unchanged.

[shingonoide]

Good catch — applied in daa596b. make_hf_error now takes an optional status_code parameter (defaults to 404) so tests simulating 401/403/503 can set the code without monkey-patching. Existing call sites keep working unchanged.

[shingonoide]

Round 3 — Copilot follow-ups applied on top of daa596b (new commit, not amend).

Copilot comments addressed:

1. huggingface-hub floor (discussion_r3133786636) — added huggingface-hub>=0.23 to pyproject.toml. The new _hf_load_errors module imports error classes that only exist from ~0.22+, so the floor protects against an older hub version breaking at import time. uv.lock regenerated via uv lock (single-line requires-dist change).

2. make_hf_error status code (discussion_r3133786674) — added optional status_code parameter (defaults to 404). Existing call sites unaffected; future tests simulating 401/403/503 can pass the real code.

State:

• HEAD: daa596b (new commit on top, not amended, no force-push).
• CI on the previous push (067e4b1) was green. Re-running now for this push.

[shingonoide]

Round 4 reply (commit 6ed6413):

B1 — HF exception import path switched from huggingface_hub.errors to huggingface_hub.utils. The utils path has been stable since hub ~0.16 and is available at the declared floor (>=0.23). No floor bump needed. All six exception classes (GatedRepoError, RepositoryNotFoundError, RevisionNotFoundError, HfHubHTTPError, EntryNotFoundError, LocalEntryNotFoundError) import cleanly from utils.

B2 — (LocalEntryNotFoundError, EntryNotFoundError) branch added before HfHubHTTPError. Neither class inherits from HfHubHTTPError so they were previously falling through to the generic "Unexpected failure" catch-all. Real-world triggers: HF_HUB_OFFLINE=1 with a cold cache, or Vertex workers racing a shared model cache.

S1 — ValueError message softened: dropped the "exposes the required head" assertion — ValueError from from_pretrained also fires for corrupt preprocessor_config.json, unrecognized model type, bad image_size, malformed revision, etc. New message: "Model '{model_id}@{revision}' could not be loaded for {task}. Detail: {repr(e)}".

S2 — two sub-items:

• (a) GatedRepoError message now explicitly mentions HF_TOKEN environment variable.
• (b) HfHubHTTPError branch inspects e.response.status_code — on 401 it emits a dedicated hint pointing to HF_TOKEN and the gated-repo flow, since older hub versions return a generic 401 without raising GatedRepoError.

S4 — ImportError allowlist (_MISSING_DEP_HINTS): covers timm, sentencepiece, detectron2, and torchvision. Unknown deps are still re-raised unchanged.

Tests: 85/85 pass. New cases cover LocalEntryNotFoundError, EntryNotFoundError, 401 branch, and sentencepiece allowlist entry (both backends).

[lucasmundim]

Looks good. Both comments from the first review have been addressed: the shared hf_load_error_handler context manager eliminates duplication, and the model-download path is now tested (test_repository_not_found_on_model_download_gives_actionable_message).

Exception hierarchy ordering is correct (verified against the live MRO: GatedRepoError → RepositoryNotFoundError → HfHubHTTPError → OSError → Exception), all raises chain via from e, repr(e) is now consistently included in every branch, and the huggingface-hub>=0.23 floor correctly guards the errors module import.

A few non-blocking observations inline.

[lucasmundim]

Follow-up suggestion: grounding_dino.py and owlvit.py also call from_pretrained with no error handling at all — they’d be natural candidates to wrap with this context manager in a subsequent PR.

[shingonoide]

Applied in 1f3de3d. Both grounding_dino.py and owlvit.py now wrap their from_pretrained calls in hf_load_error_handler, and tests/test_grounding_dino.py + tests/test_owlvit.py grow matching load-error test classes covering RepositoryNotFoundError, RevisionNotFoundError, and ValueError.

[lucasmundim]

Nit: this bare import (from _hf_test_utils import make_hf_error) works via pytest’s default prepend import mode, but there’s no conftest.py or __init__.py in tests/ to make it explicit. Consider adding [tool.pytest.ini_options] pythonpath = ["tests"] in pyproject.toml so the mechanism is documented rather than relying on pytest internals.

[shingonoide]

Applied in 1f3de3d. Added [tool.pytest.ini_options] pythonpath = ["tests"] to pyproject.toml so the from _hf_test_utils import make_hf_error shorthand is documented via pytest's own config rather than relying on prepend import mode implicitly.

[lucasmundim]

Ultra-nit: double blank line here (before v0.4.4).

[shingonoide]

Applied in 1f3de3d — double blank line before v0.4.4 removed; v0.4.5 notes also expanded to reflect the rounds 3-5 additions.

[shingonoide]

Round 5 — Lucas's follow-ups applied, previously-missed Leandro threads acknowledged.

Commit: 1f3de3d (new commit on top of 6ed6413, no amend, no force-push, DCO signed).

Applied from @lucasmundim's round-4 review:

• grounding_dino.py + owlvit.py now wrap their from_pretrained calls in hf_load_error_handler. Every backend's load path produces the same actionable errors. New load-error test classes (RepositoryNotFound / RevisionNotFound / ValueError) added to tests/test_grounding_dino.py and tests/test_owlvit.py.
• [tool.pytest.ini_options] pythonpath = [\"tests\"] declared in pyproject.toml — the from _hf_test_utils import make_hf_error shorthand is now documented via pytest config rather than relying on `prepend` import mode implicitly.
• Double blank line before v0.4.4 in RELEASE.md removed; v0.4.5 notes expanded to reflect the full scope landing in this PR.

Replied to @leandrobmarinho's earlier threads (repr(e) consistency, shared context manager, duplicated `_make_hf_error`) — all were addressed by the 42d6b89 extraction; inline replies posted pointing to that commit.

Follow-up filed: PLAT-962 tracks adding retry/backoff on transient Hub 429/5xx at load time. Not included in this PR — different concern (reliability, vs error surface quality).

Status: 47/47 non-torchvision tests pass locally; torchvision-dependent tests (object_detection / image_classification) rely on CI. CI running on `1f3de3d`.

[lucasmundim]

Good expansion of the error coverage. Verified the new exception hierarchy against huggingface_hub 1.7.1:

• EntryNotFoundError → Exception (not HfHubHTTPError) — docstring is correct
• LocalEntryNotFoundError → FileNotFoundError → OSError → EntryNotFoundError

Catch order is correct. The _MISSING_DEP_HINTS allowlist, 401 detection, and huggingface_hub.utils import path are all solid choices.

One minor observation inline (non-blocking).

[lucasmundim]

Minor: EntryNotFoundError (without "Local") can also indicate a remote file-not-found — e.g. the repo exists but is missing config.json. The "not found in cache" / "pre-download the model" / "retry after the first worker" guidance doesn’t apply to that case.

Consider either splitting the two classes into separate handlers, or broadening the message to cover both scenarios — e.g.:

Non-blocking.

[shingonoide]

Right, the cache-miss copy reads wrong when it's actually a missing remote file. Broadened the message to cover both repo missing expected files OR local cache incomplete, plus the offline pre-download hint, in a33fb6d.

[leandrobmarinho]

Approval — all items from the previous review have been addressed

Going through each point requested:

1. Duplicated exception-handling logic across backends → Resolved
The hf_load_error_handler context manager was extracted to filter_huggingface_vision/backends/_hf_load_errors.py. Any new huggingface_hub exception type now only needs to be added in one place. All four backends (object_detection, image_classification, grounding_dino, owlvit) now use the shared handler.

2. Inconsistent repr(e) in error messages → Resolved
GatedRepoError, RepositoryNotFoundError, and RevisionNotFoundError now include repr(e) in their messages, consistent with HfHubHTTPError, ValueError, and the fallback Exception.

3. Test coverage limited to processor path → Resolved
test_repository_not_found_on_model_download_gives_actionable_message was added, covering the error path during model weight download — the gap identified in the previous review.

4. Duplicated _make_hf_error helper → Resolved
Centralized in tests/_hf_test_utils.py with [tool.pytest.ini_options] pythonpath = ["tests"] in pyproject.toml, eliminating duplication across test files.

5. Manual uv.lock edit → Resolved
Lock file synced with the existing pin in pyproject.toml.

---

Solid implementation. Using huggingface_hub.utils (stable since 0.23) instead of huggingface_hub.errors is a good compatibility choice. Extending the handler to GroundingDinoBackend and OwlVitBackend — beyond the original scope — closes an inconsistency that would have surfaced eventually.

LGTM.

[stwilt]

Net-new findings only (prior threads are out of scope). Three items — all non-blocking; the first two are tied together as a maintenance footgun around the except ladder in hf_load_error_handler, the third is a test-quality nit.

[stwilt]

Docstring justification is factually wrong at the declared floor. Against huggingface-hub==0.23 (the pin added in daa596b), both classes DO inherit from HfHubHTTPError:

EntryNotFoundError      -> HfHubHTTPError -> HTTPError -> RequestException -> OSError -> Exception
LocalEntryNotFoundError -> EntryNotFoundError -> HfHubHTTPError -> HTTPError -> RequestException -> FileNotFoundError -> OSError -> ValueError -> Exception

The catch order is still correct — but for the opposite reason the comment states: these branches must come first precisely because they're subclasses of HfHubHTTPError (and in LocalEntryNotFoundError's case, also ValueError), so without this explicit handler they'd get swallowed by the later generic branches with the wrong message. A future maintainer reading this docstring could reasonably conclude the ordering is cosmetic and safely reorder — please correct the justification.

[shingonoide]

Good catch. Verified against huggingface-hub==0.23.0 with the requests backbone. EntryNotFoundError IS an HfHubHTTPError there, and LocalEntryNotFoundError is additionally a ValueError. Rewrote the docstring to cite both eras (0.23 subclass-chain + >=1.0 flattened) so a future maintainer doesn't reorder based on a stale justification. Commit: ab15532.

[stwilt]

Follow-up to the docstring nit above — LocalEntryNotFoundError is also a ValueError (via its MRO: … -> FileNotFoundError -> OSError -> ValueError). This makes the ordering relative to L84 load-bearing: if the except ValueError branch is ever hoisted above this one (e.g. during a future refactor), LocalEntryNotFoundError will be silently re-routed and the cache-specific guidance (offline pre-download / shared-cache retry) will disappear without any test catching it.

The existing test_local_entry_not_found_error_message passes because it uses the specific class — it wouldn't catch a reorder. Consider either:

1. A one-line comment above the (LocalEntryNotFoundError, EntryNotFoundError) branch noting LocalEntryNotFoundError inherits from ValueError and must stay above that branch, or
2. A regression test that asserts a LocalEntryNotFoundError produces the cache-specific message verbatim (substring match on "not found in cache" rather than just "cache"), so a reorder falling through to the ValueError branch would flip the test red.

[shingonoide]

Already in flight: you implemented this on refactor/dry-hf-load-error-tests in #23, which tightens assertIn("cache") to assertIn("not found in cache") in both test files. That is exactly what I was about to push. Since your commit also extracts the shared HFLoadErrorTestsMixin and deletes the two target test classes here, landing my duplicate would just force you to re-rebase, so leaving the assertion tightening to land with your refactor. (Did confirm the substring change has teeth by running a reorder mutation at hub 0.23 in a throwaway venv: the loose assertion passes in both orderings, the tightened one flips red when except ValueError is hoisted above the cache branch, so the new assertion does what we need.)

[shingonoide]

Update on my earlier reply: this ended up landing here in a33fb6d after all (the one-line guard comment is right above the (LocalEntryNotFoundError, EntryNotFoundError) branch now), since Lucas's review forced another push and it was cheaper to just include it than wait on PR #23.

[stwilt]

Test name promises repr(e) verification that the body doesn't deliver. The assertions check "org/model" and "HuggingFace Hub" are in the message, but neither implies repr(e) is there — the handler at filter_huggingface_vision/backends/_hf_load_errors.py:82#MM could silently drop the repr(e) interpolation and this test would still pass. Also "503 Service Unavailable" in the message string is cosmetic since make_hf_error defaults status_code=404 — so this is actually exercising the 404 → non-401 path, not a 503 path.

Two small fixes land both nits at once: (a) pass status_code=503 to make_hf_error so the test label matches behavior, and (b) assert self.assertIn("503 Service Unavailable", msg) (or assertIn(repr(exc), msg)) so a regression on repr(e) actually flips the test red. Same comment applies to the identical test at tests/test_object_detection.py:206#JQ-215#MP.

[shingonoide]

Same branch: #23 already tightens test_hf_hub_http_error_includes_repr with status_code=503 and assertIn("503 Service Unavailable"). Mutation-proved the tightened assertion has teeth by dropping repr(e) from the generic HfHubHTTPError branch in _hf_load_errors.py:82 and rerunning: the existing loose assertion stayed green (exactly the hygiene gap you flagged), the tightened one went red, restoring repr(e) flipped it back green. Leaving to land with your refactor to avoid duplicating the test change.

[shingonoide]

Same correction as the load-bearing comment thread: the assertion tightening (status_code=503 + assertIn(repr(exc), msg)) actually landed in this PR in a33fb6d, not on PR #23 like I said before. Same change in test_object_detection.py too.

[stwilt]

FYI — opened #23 as a small follow-up if you want to fold it in.

Extracts an HFLoadErrorTestsMixin in tests/_hf_test_utils.py so the four TestXBackendLoadErrors classes collapse to 5-line subclasses. Net −247 LOC (+205 / −452), and grounding_dino / owlvit pick up the 10 branches they were missing — load-error test count goes from 32 to 52, all 113 tests pass.

Also tightens two assertions from my earlier review pass on this PR: test_hf_hub_http_error_includes_repr now actually exercises 503 + asserts the message text appears (catches repr(e) regressions), and the *EntryNotFoundError tests assert the full "not found in cache" substring so a future reorder that routes LocalEntryNotFoundError through the ValueError branch would fail visibly.

Totally optional — rebase onto your branch, cherry-pick, or ignore.

[lucasmundim]

Design is solid — the shared context manager is the right abstraction, exception catch order is correct, test coverage via the mixin is thorough, and all prior review feedback has been addressed in the code.

One hard blocker (version conflict) and a few non-blocking items inline.

[lucasmundim]

CI blocker: duplicate v0.4.5

The check-release-log job fails with Duplicated version in changelog: v0.4.5. The main branch already shipped a v0.4.5 entry (SDK bump + secret-names fix in 1e86532). This file and RELEASE.md need to bump to v0.4.6.

[shingonoide]

Confirmed, main has v0.4.5 from 1e86532. Bumped VERSION and the RELEASE.md heading to v0.4.6 in a33fb6d.

[lucasmundim]

Two items on this handler:

1. LocalEntryNotFoundError / ValueError ordering is load-bearing at hub 0.23 — stwilt's open comment is valid: at the declared floor, LocalEntryNotFoundError inherits from ValueError (verified via MRO). A one-line comment above this branch would protect against a future reorder, e.g.:

# Must stay above except ValueError — LocalEntryNotFoundError inherits ValueError at hub 0.23

I verified against hub 1.7.1 that the hierarchy was flattened (neither class inherits from HfHubHTTPError or ValueError anymore), so the docstring's "both eras" claim is accurate.

2. EntryNotFoundError message may mislead (echoing lucasmundim's open comment) — EntryNotFoundError (without "Local") can mean a remote file genuinely doesn't exist in the repo (e.g., missing config.json), not just a cache miss. The current "not found in cache" / "pre-download the model" guidance is wrong for that case. Consider broadening the message, e.g.:

"A required file for '{model_id}@{revision}' was not found — the repo may be missing expected files, or the local cache is incomplete. If running offline, pre-download the model. Detail: {repr(e)}"

Neither is blocking, but both are easy fixes while you're here.

[shingonoide]

Added the one-line guard comment above the (LocalEntryNotFoundError, EntryNotFoundError) branch in a33fb6d. Message broadening lands here too, replied on the line 74 thread.

[lucasmundim]

Test assertion doesn't directly verify repr(e) (echoing stwilt's open comment)

The "503 Service Unavailable" assertion indirectly validates repr(e) is present, but if the repr(e) interpolation in the handler were dropped, this test would still pass as long as the model_id appears in the message. A direct assertion would make the test name truthful:

self.assertIn(repr(exc), msg)

Low priority — not blocking.

[shingonoide]

Fair, the indirect substring lets a repr(e) regression slip. Tightened to self.assertIn(repr(exc), msg) so the test name matches what it proves, in a33fb6d.

[shingonoide]

Thanks Lucas. Bumping VERSION and RELEASE.md to v0.4.6 since main shipped v0.4.5 in 1e86532 while this branch was open. Picking up the LocalEntryNotFoundError ordering comment and the EntryNotFoundError message broadening at the same time, replies inline.

[shingonoide]

Thanks Lucas, glad the shared handler + huggingface-hub>=0.23 floor read cleanly. Picked up the EntryNotFoundError nit below.

[shingonoide]

Thanks. Verifying against hub 1.7.1 confirmed EntryNotFoundError flattens to Exception there, so the catch order stays load-bearing across both eras.

[shingonoide]

Thanks Leandro for going point by point. The grounding_dino + owlvit extension was an obvious gap once the shared handler was in place.

[shingonoide]

Thanks stwilt. Rewrote the _hf_load_errors docstring to cite both the 0.23 subclass-chain and the >=1.0 flattened hierarchy in a33fb6d. The two test-tightening items are landing with your refactor in #23 since duplicating them here would just force a re-rebase.

[shingonoide]

Appreciate it. Leaving the assertion tightening to land with #23 so we don't double-apply the same diff. Will rebase this PR on top once #23 lands.

[lucasmundim]

Net-new findings only (prior threads are out of scope). Two items, both non-blocking.

[lucasmundim]

Nit (stwilt's open item, still unaddressed): this creates the error with the default status_code=404 while the message string says "503 Service Unavailable". The test passes because the non-401 HfHubHTTPError branch doesn't inspect the status code, but it's misleading — if someone later adds an assertion on the status code, it'll fail for the wrong reason.

Same issue in tests/test_object_detection.py:209.

exc = make_hf_error(HfHubHTTPError, "503 Service Unavailable", status_code=503)

[lucasmundim]

Ultra-nit: missing trailing newline (\ No newline at end of file in the diff). Some Unix tools (e.g. wc -l, POSIX read) behave unexpectedly without it.

[lucasmundim]

Net-new findings only (prior threads are out of scope). Two items, both non-blocking.

[lucasmundim]

status_code mismatch with message text — stwilt's earlier feedback was partially applied: the assertion is now correct, but the test still constructs the error with default while the message says "503 Service Unavailable". The test works (it exercises the non-401 path), but the mismatch is confusing to future readers.

Passing would make it self-consistent:

Same applies to the identical test in .

[lucasmundim]

Nit: missing trailing newline ( in the diff). POSIX convention — most tools handle it fine, but 0 will report 0 lines and some CI linters flag it.