-
Notifications
You must be signed in to change notification settings - Fork 2.7k
[https://nvbugs/6507082][fix] Added _resolve_missing_local_path_to_hf_hub_id — a deterministic preemptive…
#16842
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -155,6 +155,38 @@ def _fallback_to_fast_tokenizer(pretrained_model_dir: str, | |||||||||||||
| **merged) | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def _resolve_missing_local_path_to_hf_hub_id(pretrained_model_dir: str) -> str: | ||||||||||||||
| """Rewrite a nonexistent absolute path to a matching HF Hub repo id. | ||||||||||||||
|
|
||||||||||||||
| Transformers 5.x's tightened ``validate_repo_id`` rejects strings with | ||||||||||||||
| more than one ``/`` before the Hub lookup can fall back, so absolute | ||||||||||||||
| filesystem paths that no longer exist now raise ``HFValidationError`` | ||||||||||||||
| instead of resolving from the cache as they did in 4.x. When the local | ||||||||||||||
| HF cache holds exactly one model repo whose basename matches, rewrite | ||||||||||||||
| to that repo id; otherwise return unchanged so the original error path | ||||||||||||||
| is preserved. | ||||||||||||||
| """ | ||||||||||||||
| if not os.path.isabs(pretrained_model_dir) or os.path.isdir( | ||||||||||||||
| pretrained_model_dir): | ||||||||||||||
| return pretrained_model_dir | ||||||||||||||
|
Comment on lines
+169
to
+171
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Do not use
Proposed fix- if not os.path.isabs(pretrained_model_dir) or os.path.isdir(
+ if not os.path.isabs(pretrained_model_dir) or os.path.lexists(
pretrained_model_dir):📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||
| basename = os.path.basename(pretrained_model_dir.rstrip("/")) | ||||||||||||||
| if not basename: | ||||||||||||||
| return pretrained_model_dir | ||||||||||||||
| from huggingface_hub import scan_cache_dir | ||||||||||||||
| from huggingface_hub.utils import CacheNotFound | ||||||||||||||
| try: | ||||||||||||||
| cache_info = scan_cache_dir() | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Also, only |
||||||||||||||
| except CacheNotFound: | ||||||||||||||
| return pretrained_model_dir | ||||||||||||||
|
Comment on lines
+175
to
+180
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo "== tokenizer.py context =="
sed -n '145,190p' tensorrt_llm/tokenizer/tokenizer.py
echo
echo "== search for scan_cache_dir usage =="
rg -n "scan_cache_dir|CacheNotFound" tensorrt_llm -S
echo
echo "== Hugging Face cache docs/source references in repo =="
rg -n "huggingface_hub|cache dir|scan_cache_dir" -S README* docs tensorrt_llm || trueRepository: NVIDIA/TensorRT-LLM Length of output: 8001 🌐 Web query:
💡 Result: Yes, the ValueError "cache path is a file" is a documented behavior of the scan_cache_dir function in the huggingface_hub library [1][2]. When you call scan_cache_dir, the library performs a validation check on the provided cache directory path. If the path specified exists but points to a file instead of a directory, the function explicitly raises a ValueError with a message indicating that it expects a directory but found a file [3]. This behavior is documented in the official Hugging Face documentation under the Cache-system reference [1][2]. The documentation explicitly states under the "Raises" section for scan_cache_dir that a ValueError will be thrown if the cache directory is a file rather than a directory [1][2]. The underlying implementation uses Python's pathlib.Path.is_file method to perform this check before proceeding with the scan [3]. Citations:
🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect, sys
try:
import huggingface_hub
from huggingface_hub import scan_cache_dir
from huggingface_hub.utils import CacheNotFound
print("huggingface_hub version:", getattr(huggingface_hub, "__version__", "unknown"))
print("scan_cache_dir:", scan_cache_dir)
print("CacheNotFound:", CacheNotFound)
print("scan_cache_dir doc:")
print(inspect.getdoc(scan_cache_dir) or "<no doc>")
except Exception as e:
print("IMPORT_ERROR:", type(e).__name__, e)
PYRepository: NVIDIA/TensorRT-LLM Length of output: 225 🏁 Script executed: #!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect
try:
import huggingface_hub
from huggingface_hub import scan_cache_dir
from huggingface_hub.utils import CacheNotFound
print("huggingface_hub version:", getattr(huggingface_hub, "__version__", "unknown"))
print("scan_cache_dir:", scan_cache_dir)
print("CacheNotFound:", CacheNotFound)
print("scan_cache_dir doc:")
print(inspect.getdoc(scan_cache_dir) or "<no doc>")
except Exception as e:
print("IMPORT_ERROR:", type(e).__name__, e)
PYRepository: NVIDIA/TensorRT-LLM Length of output: 225 Catch 🤖 Prompt for AI Agents |
||||||||||||||
| matches = [ | ||||||||||||||
| repo.repo_id for repo in cache_info.repos if repo.repo_type == "model" | ||||||||||||||
| and repo.repo_id.rsplit("/", 1)[-1] == basename | ||||||||||||||
| ] | ||||||||||||||
| if len(matches) == 1: | ||||||||||||||
| return matches[0] | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This rewrite is completely silent. Basename equality is not checkpoint identity — a user pointing at Even with the env-var gate suggested at the call site, emit a |
||||||||||||||
| return pretrained_model_dir | ||||||||||||||
|
|
||||||||||||||
|
|
||||||||||||||
| def maybe_fix_byte_level_tokenizer(tokenizer, pretrained_model_dir: str, | ||||||||||||||
| **kwargs): | ||||||||||||||
| """Work around Transformers 5.x LlamaTokenizer overriding tokenizer.json. | ||||||||||||||
|
|
@@ -289,6 +321,8 @@ def __repr__(self) -> str: | |||||||||||||
|
|
||||||||||||||
| @classmethod | ||||||||||||||
| def from_pretrained(cls, pretrained_model_dir: str, **kwargs): | ||||||||||||||
| pretrained_model_dir = _resolve_missing_local_path_to_hf_hub_id( | ||||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This makes the rewrite unconditional for every caller of That keeps the default behavior for users unchanged (missing path → clear error) and confines the fallback to the environment that actually needs it, which is the only place it's justified. |
||||||||||||||
| pretrained_model_dir) | ||||||||||||||
|
Comment on lines
+324
to
+325
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the relevant tokenizer code.
python3 - <<'PY'
from pathlib import Path
path = Path("tensorrt_llm/tokenizer/tokenizer.py")
lines = path.read_text().splitlines()
for start, end in [(140, 190), (300, 340)]:
print(f"\n--- {path}:{start}-{end} ---")
for i in range(start, end + 1):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
# Look for cache_dir handling in this file.
rg -n "cache_dir|scan_cache_dir|from_pretrained|_resolve_missing_local_path_to_hf_hub_id" tensorrt_llm/tokenizer/tokenizer.py
# If the Python packages are available, print the relevant API signatures/docs.
python3 - <<'PY'
import inspect
for mod_name, obj_name in [
("huggingface_hub", "scan_cache_dir"),
("transformers", "AutoTokenizer"),
]:
try:
mod = __import__(mod_name, fromlist=[obj_name])
obj = getattr(mod, obj_name)
print(f"\n--- {mod_name}.{obj_name} ---")
if obj_name == "AutoTokenizer":
print("from_pretrained signature:", inspect.signature(obj.from_pretrained))
else:
print("signature:", inspect.signature(obj))
except Exception as e:
print(f"\n--- {mod_name}.{obj_name} unavailable ---")
print(type(e).__name__, e)
PYRepository: NVIDIA/TensorRT-LLM Length of output: 6659 🌐 Web query:
💡 Result: The scan_cache_dir function in the huggingface_hub library is used to programmatically scan your Hugging Face cache directory [1][2]. The function accepts an optional parameter named cache_dir [1][3]: - Parameter: cache_dir - Type: str, Path, or None [2][4] - Description: Specifies the path to the cache directory you wish to scan [1][3]. - Default: If this parameter is not provided (or set to None), the function defaults to the standard Hugging Face cache directory [1][4]. When called, scan_cache_dir returns an HFCacheInfo structure containing a detailed report of the contents of the specified directory, organized by repositories, revisions, and files [1][5]. If a repository is found to be corrupted, the function handles the error internally and includes a CorruptedCacheException in the report's warnings rather than crashing [1][2]. Citations:
Pass the caller cache_dir through the resolver
📍 Affects 1 file
🤖 Prompt for AI Agents |
||||||||||||||
| try: | ||||||||||||||
| tokenizer = AutoTokenizer.from_pretrained(pretrained_model_dir, | ||||||||||||||
| **kwargs) | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -421,8 +421,6 @@ unittest/executor/test_rpc_worker.py SKIP (https://nvbugs/5605741) | |
| unittest/llmapi/test_additional_model_outputs.py -m "gpu2" SKIP (https://nvbugs/6428091) | ||
| unittest/llmapi/test_additional_model_outputs.py::test_additional_model_outputs_integration_pp2 SKIP (https://nvbugs/6427411) | ||
| unittest/llmapi/test_llm.py::test_llm_with_customized_tokenizer SKIP (https://nvbugs/6507080) | ||
| unittest/llmapi/test_llm.py::test_tokenizer_decode_incrementally[/scratch.trt_llm_data/llm-models/falcon-7b-instruct-False-0.95-HF] SKIP (https://nvbugs/6507082) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we still need to test falcon? |
||
| unittest/llmapi/test_llm.py::test_tokenizer_decode_incrementally[/scratch.trt_llm_data/llm-models/falcon-7b-instruct-False-0.95-TRTLLM] SKIP (https://nvbugs/6507082) | ||
| unittest/llmapi/test_llm_multi_gpu_pytorch.py -m "gpu2" SKIP (https://nvbugs/6428092) | ||
| unittest/llmapi/test_llm_multi_gpu_pytorch.py::test_llm_get_stats_pp2[False-False-True] SKIP (https://nvbugs/6432826) | ||
| unittest/llmapi/test_llm_multi_gpu_pytorch.py::test_llm_get_stats_pp4[False-False-True] SKIP (https://nvbugs/6427411) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This helper is unreferenced by any test. Please add unit coverage in
tests/unittest/llmapi/withscan_cache_dirmonkeypatched: opt-in env var unset (must return input unchanged without touching the cache), non-absolute input, existing dir,CacheNotFound, zero matches, two matches (must return input unchanged), and exactly one match. All of these are pure-Python and need no GPU or model weights.