[https://nvbugs/6115560][fix] catch OSError in config_file_lock for NFS compatibility - #11960
Conversation
📝 WalkthroughWalkthroughExpanded exception handling in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tensorrt_llm/_torch/model_config.py`:
- Around line 51-54: The contextmanager config_file_lock currently contains
multiple yield points and broad try/excepts that can catch exceptions raised by
the with-block; refactor it to a single-yield pattern by performing lock
acquisition and any related filesystem error handling before yielding, then
yield exactly once to the caller, and perform lock release/cleanup in a finally
block after the yield; ensure only the lock acquisition logic (e.g.,
filelock.FileLock.acquire and fallback to tempfile logic) is wrapped in
try/except for PermissionError/OSError/filelock.Timeout, and remove any
try/except that spans the yield so exceptions from the with-block bubble up
normally.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 46c0c039-a585-4aad-aff3-13f66709b223
📒 Files selected for processing (1)
tensorrt_llm/_torch/model_config.py
|
/bot run --disable-fail-fast |
|
PR_Github #38459 [ run ] triggered by Bot. Commit: |
|
PR_Github #38459 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #38492 [ run ] triggered by Bot. Commit: |
|
PR_Github #38492 [ run ] completed with state
|
chienchunhung
left a comment
There was a problem hiding this comment.
The PR looks good to me. PS: We might need to clean up something in the follow-up PR.
|
/bot run --disable-fail-fast |
|
PR_Github #38606 [ run ] triggered by Bot. Commit: |
|
PR_Github #38606 [ run ] completed with state
|
72a8665 to
e548b35
Compare
|
/bot run |
|
PR_Github #49372 [ run ] triggered by Bot. Commit: |
|
PR_Github #49372 [ run ] completed with state
|
|
@sara4dev @pengbowang-nv — fresh CI (L0_MergeRequest_PR #39025) confirms Representative stack trace (e.g. The test calls Two minimal fix options, both keep the bug-fix intent for ENOLCK/ESTALE: A. Narrow to specific errnos (matches the original review suggestion): except OSError as e:
if e.errno not in {errno.ENOLCK, errno.ESTALE, errno.EACCES, errno.EPERM}:
raise
# ... tempdir fallbackB. Scope the broad catch to try:
lock.acquire(timeout=timeout)
except (PermissionError, OSError, filelock.Timeout):
# tempdir fallback
...
else:
try:
yield
finally:
lock.release()I think either resolves the failing tests – @sara4dev can you please take a look? |
…rnos in config_file_lock Catching broad OSError in config_file_lock swallowed legitimate non-lock OSErrors raised inside the yielded body (notably HuggingFace's RepositoryNotFoundError, an OSError subclass), which broke 29 tests in unittest._torch.executor.test_pytorch_model_engine with "RuntimeError: generator didn't stop after throw()". Narrow the catch to the errnos actually relevant for lock-infrastructure failures: EACCES, EPERM (PermissionError), ENOLCK, ESTALE (NFS locking). All other OSError subclasses propagate to the caller. Addresses pengbowang-nv review feedback on PR NVIDIA#11960. Signed-off-by: Chang Liu <9713593+chang-l@users.noreply.github.com>
|
Applied Option A from earlier comment since author was unresponsive past Friday EOB. New commit narrows the /bot run |
…rnos in config_file_lock Catching broad OSError in config_file_lock swallowed legitimate non-lock OSErrors raised inside the yielded body (notably HuggingFace's RepositoryNotFoundError, an OSError subclass), which broke 29 tests in unittest._torch.executor.test_pytorch_model_engine with "RuntimeError: generator didn't stop after throw()". Narrow the catch to the errnos actually relevant for lock-infrastructure failures: EACCES, EPERM (PermissionError), ENOLCK, ESTALE (NFS locking). All other OSError subclasses propagate to the caller. Addresses pengbowang-nv review feedback on PR NVIDIA#11960. Signed-off-by: Chang Liu <9713593+chang-l@users.noreply.github.com>
3003656 to
4d86ed2
Compare
|
Rebased with sign-offs to satisfy DCO (force-pushed; content unchanged — verified tree hash equals pre-rebase). New HEAD: /bot run |
|
/bot run |
|
PR_Github #50010 [ run ] triggered by Bot. Commit: |
|
PR_Github #50010 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #50023 [ run ] triggered by Bot. Commit: |
|
PR_Github #50023 [ run ] completed with state
|
On NFS-backed HF_MODULES_CACHE paths, filelock operations can raise OSError with errno ENOLCK (No locks available) or ESTALE (Stale file handle) instead of PermissionError. This causes config_file_lock() to crash rather than falling back to the tempdir-based lock. Add OSError to the exception handlers so the existing fallback logic handles NFS locking failures gracefully. Fixes NVIDIA#11958 Signed-off-by: Chang Liu <9713593+chang-l@users.noreply.github.com>
…rnos in config_file_lock Catching broad OSError in config_file_lock swallowed legitimate non-lock OSErrors raised inside the yielded body (notably HuggingFace's RepositoryNotFoundError, an OSError subclass), which broke 29 tests in unittest._torch.executor.test_pytorch_model_engine with "RuntimeError: generator didn't stop after throw()". Narrow the catch to the errnos actually relevant for lock-infrastructure failures: EACCES, EPERM (PermissionError), ENOLCK, ESTALE (NFS locking). All other OSError subclasses propagate to the caller. Addresses pengbowang-nv review feedback on PR NVIDIA#11960. Signed-off-by: Chang Liu <9713593+chang-l@users.noreply.github.com>
4d86ed2 to
fd6ccbc
Compare
|
/bot run |
|
PR_Github #50582 [ run ] triggered by Bot. Commit: |
|
PR_Github #50582 [ run ] completed with state |
|
hi @chang-l , option-A lead to deepseek_r1_0528 error in GB300. I create a pr to apply option-B. Do you think it's OK? |
Summary
config_file_lock()intensorrt_llm/_torch/model_config.pycrashes whenHF_MODULES_CACHEresides on an NFS-mounted filesystem. On NFS,filelockoperations can raiseOSErrorwith errnoENOLCK(No locks available) orESTALE(Stale file handle) instead ofPermissionError. Since the current exception handler only catchesPermissionErrorandfilelock.Timeout, these NFS-specific errors bypass the existing tempdir fallback and crash the process.This is particularly impactful in multi-node GPU clusters where a shared NFS cache is standard practice — every pod that tries to load a model config concurrently hits this crash.
Changes
Add
OSErrorto both exception handlers inconfig_file_lock():except (PermissionError, filelock.Timeout)→except (PermissionError, OSError, filelock.Timeout)— triggers the tempdir fallback for NFS errorsexcept (PermissionError)→except (PermissionError, OSError)— handles the unlikely case where tempdir also failsSince
PermissionErroris a subclass ofOSError, catchingOSErrortechnically covers both, but keepingPermissionErrorexplicit preserves the original intent and readability.Root Cause
NFSv3 uses the Network Lock Manager (NLM) protocol for file locking, which is unreliable for cross-node
flock()/fcntl()operations. Whenfilelock.FileLockattempts to acquire a lock on an NFS path:OSError: [Errno 37] No locks available(ENOLCK)OSError: [Errno 116] Stale file handle(ESTALE)The existing fallback to
/tmp(local ephemeral storage) is the correct behavior for this case — it just wasn't being triggered.Reproduction
Workaround (for users on affected versions)
Set
HF_MODULES_CACHE=/tmp/hf_modulesas an environment variable to redirect the lock file to local storage.Fixes #11958
Made with Cursor
Summary by CodeRabbit