[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
|
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: 🚥 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.
…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>
1b9d08a to
1db8f5a
Compare
2 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(pretrained_model_dir)intensorrt_llm/tokenizer/tokenizer.pyto deterministically rewrite nonexistent absolute local paths to a matching Hugging Face Hubrepo_idwhen the basename uniquely matches a cached model repo; otherwise returns the input unchanged to preserve Transformers 5.x validation/fallback behavior.TransformersTokenizer.from_pretrained()to invoke the resolver before callingAutoTokenizer.from_pretrained().NcclEPstrategy:tensorrt_llm/_torch/modules/fused_moe/communication/nccl_ep.pyandnccl_ep_utils.py(nccl4pynccl.ep-backed, rank-major expert parallelism).communication_factory.pyto include NCCL EP in the auto-selection priority and to provide a structured “unavailable reason” gate.moe_scheduler.py) to treatNcclEPlike other strategies requiring non-Nonetoken_final_scales.AlltoallMethodType.NcclEPinfused_moe/interface.py.local_num_experts < top_kby padding with deterministic out-of-shard expert IDs.bench_moe_comm.py:--verifysentinel coverage for dispatch correctness and includedNCCL_EPas a benchmark backend.test_communication_factory.py, plus a new CUDA graph replay routing test intest_moe_comm.py).local_num_experts < top_kwarmup case.tests/integration/test_lists/test-db/l0_gb200_multi_gpus.yml.test_tokenizer_decode_incrementally(Falcon 7B instruct, HF and TRTLLM variants), allowing the test to run again.cpp/tensorrt_llm/runtime/utils/mpiUtils.cpp: avoid callingMPI_Comm_freeafterMPI_Finalized()to prevent exit-time heap corruption.nccl4py>=0.3.1,<0.4to supportnccl.ep.QA Engineer Review
Test-list changes (test-db / waives)
tests/integration/test_lists/test-db/l0_gb200_multi_gpus.ymlunittest/_torch/modules/moe/test_moe_comm.py::TestMoEComm::test_nccl_ep_cuda_graph_replay_uses_updated_routingtests/integration/test_lists/waives.txtunittest/llmapi/test_llm.py::test_tokenizer_decode_incrementally[/scratch.trt_llm_data/llm-models/falcon-7b-instruct-False-0.95-HF]unittest/llmapi/test_llm.py::test_tokenizer_decode_incrementally[/scratch.trt_llm_data/llm-models/falcon-7b-instruct-False-0.95-TRTLLM]Verdict: needs follow-up (CBTS/coverage results not available here).
Test code changes (functions added/modified)
tests/unittest/_torch/modules/moe/test_communication_factory.py(new file)test_nccl_ep_installed_handles_runtime_probe_failuretest_forced_nccl_ep_validates_preconditionstest_forced_nccl_ep_allows_missing_moe_max_num_tokenstest_auto_selection_uses_nccl_ep_with_missing_moe_max_num_tokenstest_auto_selection_skips_nccl_ep_when_preconditions_failtest_auto_selection_skips_nccl_ep_for_quantized_moetest_auto_selection_falls_back_when_nccl_probe_runtime_failstest_nccl_ep_context_init_rejects_cuda_graph_capturetest_nccl_ep_handle_init_rejects_cuda_graph_capturetest-db/context; needs follow-up.tests/unittest/_torch/misc/test_autotuner.pytest_trtllm_gen_moe_dummy_topk_local_experts_less_than_topk(NVBugs 6457853)test-db/context; needs follow-up.tests/unittest/_torch/modules/moe/test_moe_comm.pyTestMoEComm::test_nccl_ep_cuda_graph_replay_uses_updated_routingtest-db/l0_gb200_multi_gpus.ymlentry added).Overall test verdict: needs follow-up.