[https://nvbugs/6507082][fix] Added _resolve_missing_local_path_to_hf_hub_id — a deterministic preemptive… - #16842
[https://nvbugs/6507082][fix] Added _resolve_missing_local_path_to_hf_hub_id — a deterministic preemptive…#16842trtllm-agent wants to merge 2 commits into
_resolve_missing_local_path_to_hf_hub_id — a deterministic preemptive…#16842Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Hugging Face Hub repository ID resolution for nonexistent absolute tokenizer paths and applies it before ChangesTokenizer resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TransformersTokenizer
participant HFCache
participant AutoTokenizer
TransformersTokenizer->>HFCache: scan cache for a unique basename match
HFCache-->>TransformersTokenizer: return Hub repository ID or original path
TransformersTokenizer->>AutoTokenizer: load tokenizer with resolved input
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tensorrt_llm/tokenizer/tokenizer.py`:
- Around line 165-167: Update the path check in the tokenizer’s pretrained-model
directory resolution logic to use os.path.lexists() instead of os.path.isdir(),
preserving any existing absolute path including regular files and symlinks while
retaining the current handling for non-absolute paths.
- Around line 320-321: The resolver currently ignores the caller’s custom cache
directory, which can select the wrong repository. Update
_resolve_missing_local_path_to_hf_hub_id and its call site around
pretrained_model_dir to accept kwargs.get("cache_dir"), then pass that value to
scan_cache_dir(cache_dir=cache_dir). Apply the changes at
tensorrt_llm/tokenizer/tokenizer.py lines 320-321 and 171-174.
- Around line 171-176: Update the cache lookup around scan_cache_dir() to also
catch ValueError and return pretrained_model_dir, preserving the existing
CacheNotFound fallback and AutoTokenizer error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 008f6039-674b-411f-8bb8-fff3082bfec3
📒 Files selected for processing (1)
tensorrt_llm/tokenizer/tokenizer.py
| if not os.path.isabs(pretrained_model_dir) or os.path.isdir( | ||
| pretrained_model_dir): | ||
| return pretrained_model_dir |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not use isdir() as the missing-path check.
os.path.isdir() is false for existing regular files, so an absolute file can be rewritten when its basename matches a cached model. Use os.path.lexists() (or exists() if broken symlinks should count as missing) to preserve every existing path.
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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not os.path.isabs(pretrained_model_dir) or os.path.isdir( | |
| pretrained_model_dir): | |
| return pretrained_model_dir | |
| if not os.path.isabs(pretrained_model_dir) or os.path.lexists( | |
| pretrained_model_dir): | |
| return pretrained_model_dir |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/tokenizer/tokenizer.py` around lines 165 - 167, Update the path
check in the tokenizer’s pretrained-model directory resolution logic to use
os.path.lexists() instead of os.path.isdir(), preserving any existing absolute
path including regular files and symlinks while retaining the current handling
for non-absolute paths.
| from huggingface_hub import scan_cache_dir | ||
| from huggingface_hub.utils import CacheNotFound | ||
| try: | ||
| cache_info = scan_cache_dir() | ||
| except CacheNotFound: | ||
| return pretrained_model_dir |
There was a problem hiding this comment.
🩺 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:
huggingface_hub scan_cache_dir ValueError cache path is a file documented behavior
💡 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:
- 1: https://huggingface.co/docs/huggingface_hub/en/package_reference/cache
- 2: https://huggingface.co/docs/huggingface_hub/main/package_reference/cache
- 3: https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/utils/_cache_manager.py
🏁 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 ValueError from scan_cache_dir(). It also raises when the cache path exists but is a file, so falling back to pretrained_model_dir keeps AutoTokenizer on the existing error path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/tokenizer/tokenizer.py` around lines 171 - 176, Update the cache
lookup around scan_cache_dir() to also catch ValueError and return
pretrained_model_dir, preserving the existing CacheNotFound fallback and
AutoTokenizer error path.
| pretrained_model_dir = _resolve_missing_local_path_to_hf_hub_id( | ||
| pretrained_model_dir) |
There was a problem hiding this comment.
🗄️ 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:
huggingface_hub scan_cache_dir cache_dir parameter documentation
💡 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:
- 1: https://huggingface.co/docs/huggingface_hub/main/package_reference/cache
- 2: https://github.com/huggingface/huggingface_hub/blob/main/src/huggingface_hub/utils/_cache_manager.py
- 3: https://huggingface.co/docs/huggingface_hub/en/package_reference/cache
- 4: https://huggingface.co/docs/huggingface_hub/v1.9.1/package_reference/cache
- 5: https://huggingface.co/docs/huggingface_hub/en/guides/manage-cache
Pass the caller cache_dir through the resolver
tensorrt_llm/tokenizer/tokenizer.py:320-321 forwards kwargs to AutoTokenizer.from_pretrained(), but _resolve_missing_local_path_to_hf_hub_id() still scans the default Hugging Face cache at tensorrt_llm/tokenizer/tokenizer.py:171-174. In custom-cache setups, this can rewrite to the wrong repo id when the same basename exists in multiple caches. Pass kwargs.get("cache_dir") into the helper and call scan_cache_dir(cache_dir=cache_dir).
📍 Affects 1 file
tensorrt_llm/tokenizer/tokenizer.py#L320-L321(this comment)tensorrt_llm/tokenizer/tokenizer.py#L171-L174
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tensorrt_llm/tokenizer/tokenizer.py` around lines 320 - 321, The resolver
currently ignores the caller’s custom cache directory, which can select the
wrong repository. Update _resolve_missing_local_path_to_hf_hub_id and its call
site around pretrained_model_dir to accept kwargs.get("cache_dir"), then pass
that value to scan_cache_dir(cache_dir=cache_dir). Apply the changes at
tensorrt_llm/tokenizer/tokenizer.py lines 320-321 and 171-174.
1b9d08a to
1db8f5a
Compare
1db8f5a to
c4ae02d
Compare
| 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) |
There was a problem hiding this comment.
do we still need to test falcon?
…a HF cache lookup Transformers 5.x tightened validate_repo_id: strings with more than one "/" are now rejected before hf_hub_download can fall back to Hub, so a nonexistent absolute local path (e.g. /code/llm-models/falcon-7b-instruct after the falcon-7b-instruct entry was removed from the shared model catalog) raises HFValidationError instead of falling back gracefully as it did in 4.x. Add _resolve_missing_local_path_to_hf_hub_id: a deterministic preemptive path rewrite invoked at the top of TransformersTokenizer.from_pretrained. If the input is an absolute filesystem path that does not exist as a directory, and its basename uniquely matches exactly one repo in the local huggingface_hub cache, rewrite the input to that repo id so the downstream AutoTokenizer.from_pretrained call resolves against the already-cached snapshot. In every other case (path exists, ambiguous or missing cache match, huggingface_hub unavailable, filesystem error) the input is returned unchanged so the original error path is preserved verbatim -- this is not a try/except fallback around the loader. Fixes test_tokenizer_decode_incrementally[falcon-7b-instruct-...-HF/TRTLLM]: tiiuae/falcon-7b-instruct is already fully populated in the CI containers' HF cache, so the two variants now resolve and pass with 100% perfect matching. Signed-off-by: handongl <handongl@nvidia.com>
Signed-off-by: handongl <handongl@nvidia.com>
8e69bd2 to
b532c45
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
brnguyen2
left a comment
brnguyen2
left a comment
There was a problem hiding this comment.
The root cause here is that the falcon-7b-instruct directory under LLM_MODELS_ROOT no longer exists — the test asks for a path that isn't there. Papering over that in the shared tokenizer loading path means every TRT-LLM user now gets an implicit "if your path is missing, I'll load something from your HF cache instead" behavior, to fix one CI test. Restoring the model in the shared catalog (or dropping/marking that parametrization) is the smaller and more honest fix; I'd want a strong argument for the library change before taking it.
If it does stay, three things are missing:
- It should be opt-in, behind an environment variable. A silent path rewrite shouldn't be the default behavior for every user of
TransformersTokenizer.from_pretrained. Gate it on something likeTLLM_RESOLVE_MISSING_PATH_TO_HF_HUB=1, default off, and set that variable in the CI environment for the affected job. That keeps the default path strictly "missing path → error" while still un-waiving the test. - No tests. The function is pure and trivially unit-testable — monkeypatch
scan_cache_dirand assert each branch (env var unset, non-abs, existing dir, no match, ambiguous match, single match). 34 lines of new resolution logic in the default tokenizer path with zero coverage is the main gap. - The waiver removal is unverified for CI. The un-waived test now passes only on nodes whose HF cache already contains
tiiuae/falcon-7b-instruct. If CI containers start with a cold cache, this reopens as a flake. Has a post-merge run ofA100X-PyTorch-Post-Merge-1been done on a fresh container?
|
|
||
| @classmethod | ||
| def from_pretrained(cls, pretrained_model_dir: str, **kwargs): | ||
| pretrained_model_dir = _resolve_missing_local_path_to_hf_hub_id( |
There was a problem hiding this comment.
This makes the rewrite unconditional for every caller of TransformersTokenizer.from_pretrained. If the workaround stays, please make it opt-in behind an environment variable — e.g. an early if os.getenv("TLLM_RESOLVE_MISSING_PATH_TO_HF_HUB", "0") != "1": return pretrained_model_dir at the top of the resolver, with the variable set in the CI job that needs it.
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.
| and repo.repo_id.rsplit("/", 1)[-1] == basename | ||
| ] | ||
| if len(matches) == 1: | ||
| return matches[0] |
There was a problem hiding this comment.
This rewrite is completely silent. Basename equality is not checkpoint identity — a user pointing at /data/my-finetunes/falcon-7b-instruct that got unmounted will silently load tiiuae/falcon-7b-instruct from the Hub cache and get plausible-looking but wrong tokenization, instead of a clear "path does not exist" error.
Even with the env-var gate suggested at the call site, emit a logger.warning naming both the original path and the substituted repo id before returning matches[0], so the substitution is visible in logs.
| from huggingface_hub import scan_cache_dir | ||
| from huggingface_hub.utils import CacheNotFound | ||
| try: | ||
| cache_info = scan_cache_dir() |
There was a problem hiding this comment.
scan_cache_dir() with no argument scans HF_HOME/the default hub cache. If the caller passed cache_dir=... in **kwargs (it's forwarded straight to AutoTokenizer.from_pretrained at line 327), the resolver can rewrite to a repo id that the actual load then can't find locally and tries to fetch from the network. Either pass the caller's cache_dir through, or skip the rewrite when cache_dir is present.
Also, only CacheNotFound is caught — a permission error or a corrupted cache blob raises out of from_pretrained and turns a recoverable case into a hard failure. Widening to OSError alongside CacheNotFound would keep this strictly best-effort.
| **merged) | ||
|
|
||
|
|
||
| def _resolve_missing_local_path_to_hf_hub_id(pretrained_model_dir: str) -> str: |
There was a problem hiding this comment.
This helper is unreferenced by any test. Please add unit coverage in tests/unittest/llmapi/ with scan_cache_dir monkeypatched: 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.
6 participants
Summary
_resolve_missing_local_path_to_hf_hub_id— a deterministic preemptive path rewrite (not a try/except-fallback) that, when the input is a nonexistent absolute path whose basename uniquely matches one cached HF Hub repo, rewrites it to that repo id; returns the input unchanged in every ambiguous/absent case so the original error path is preserved.Test plan
Links
Dev Engineer Review
_resolve_missing_local_path_to_hf_hub_id()intensorrt_llm/tokenizer/tokenizer.py.TransformersTokenizer.from_pretrained()to apply the resolver beforeAutoTokenizer.from_pretrained().SKIPwaiver entries fromtests/integration/test_lists/waives.txtfortest_tokenizer_decode_incrementally.QA Engineer Review
tests/integration/test_lists/waives.txt.unittest/llmapi/test_llm.py::test_tokenizer_decode_incrementallyunderfalcon-7b-instruct-False-0.95.