fix(inspect): distinguish local-path-not-found from network errors#679
Conversation
Bogus HF IDs and missing local files were both reported as "Network error" because AutoConfig.from_pretrained raises OSError in both cases, and the handler blindly mapped all OSError to NetworkError. Two targeted fixes: - Add a local-path pre-check in inspect() that detects inputs looking like file paths (absolute, backslash, dot-prefix, or file extension) and fails early with "Local path '...' does not exist." before touching HF Hub. - Catch RepositoryNotFoundError explicitly (before the broad OSError handler) in _inspect_model_v2() and re-raise as ModelNotFoundError so HF 404s show "Model not found: ..." instead of "Network error: ...". Closes #542
timenick
left a comment
There was a problem hiding this comment.
Found a blocking regression in the local-path heuristic — bool(_p.suffix) will misclassify popular HF model IDs with dotted version numbers (Phi-3.5, Qwen2.5, Llama-3.1, …) as local paths and fail before any Hub lookup. See inline. Three smaller nits also flagged.
… positives Agent-Logs-Url: https://github.com/microsoft/winml-cli/sessions/3cd1e0cd-3f1f-472f-b66c-b7275eaf967d Co-authored-by: xieofxie <2876650+xieofxie@users.noreply.github.com>
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Address PR #679 review comments: - Replace bool(_p.suffix) with a allowlisted _LOCAL_FILE_EXTS set to avoid misclassifying dotted HF IDs (Phi-3.5, Qwen2.5, Llama-3.1, …) as local paths. Move the constant to module level. - Tighten dot-prefix check to startswith(("./", "../")) so bare model names starting with a dot are not over-eagerly matched. - Fix tmp_path type annotation: pytest.TempPathFactory → Path. - Add test_dotted_hf_id_reaches_hub_path to pin the contract that HF IDs with version dots flow to the Hub branch and are never shown "does not exist".
Root cause: transformers catches RepositoryNotFoundError internally and re-raises it as a plain OSError, so `except RepositoryNotFoundError` never fires in practice. The error was still landing in `except OSError` and being re-raised as NetworkError. Fix: in the OSError handler, detect the recognisable HF Hub "not a valid model identifier / not a local folder" message pattern and raise ModelNotFoundError instead of NetworkError. The full original message is preserved so the private-repository hint is visible in the error output. Also update the test to mock the actual OSError that transformers raises (not RepositoryNotFoundError directly) and assert the private-repo hint is present in the output. Retain a separate test for the direct RepositoryNotFoundError path as a defence-in-depth check.
timenick
left a comment
There was a problem hiding this comment.
LGTM. All four prior review comments cleanly addressed:
bool(_p.suffix)regression → replaced with_LOCAL_FILE_EXTSallowlist via the new_looks_like_local_path()helper (now correctly handles Phi-3.5 / Qwen2.5 / Llama-3.1 / flan-t5-v1.1)- Test type annotation fixed via
TYPE_CHECKINGimport ofPath - Test docstring now matches the actual
missing.onnxtest code test_dotted_hf_id_reaches_hub_pathadded as the suggested regression-pin
Bonus fix in e0f4941: caught that transformers.AutoConfig.from_pretrained re-wraps RepositoryNotFoundError as plain OSError, so the original except RepositoryNotFoundError handler would never have fired on the common path. The added message-pattern match plus test_bogus_hf_id_shows_model_not_found (which uses the real transformers error text and preserves the private-repo hint) closes this gap properly.
🤖 Generated with GitHub Copilot CLI
Summary
Fixes #542. Two distinct failure modes were both surfacing as
"Network error":totally-bogus/does-not-exist) →RepositoryNotFoundErroris anOSErrorsubclass, so the broadexcept OSError → NetworkErrorhandler caught it.\does-not-exist.onnx) →AutoConfig.from_pretrainedraised a plainOSErrorwith a confusing HF Hub message including "Network error"Fix 1 — local path pre-check in
inspect()(inspect.py)Before touching HF Hub, classify the input. If it looks like a local path (absolute, backslash, dot-prefix, or has a file extension — none of which appear in normal
org/modelHF IDs) and does not exist, fail early with:Fix 2 — explicit
RepositoryNotFoundErrorcatch in_inspect_model_v2()(inspect.py)Catch
RepositoryNotFoundErrorbefore the broadOSErrorhandler so HF 404s map toModelNotFoundErrorand surface as:Tests (test_inspect_cli.py)
test_missing_local_file_shows_path_error— absolute tmp path without.onnxtest_missing_local_relative_path_shows_path_error—./does-not-exist.onnxtest_bogus_hf_id_shows_model_not_found— patchesAutoConfig.from_pretrainedto raiseRepositoryNotFoundError