[https://nvbugs/6435642][fix] detect killed MPI executor workers#16338
[https://nvbugs/6435642][fix] detect killed MPI executor workers#16338chienchunhung wants to merge 1 commit into
Conversation
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
792a8c7 to
67d5ab1
Compare
|
/bot run --disable-fail-fast --stage-list "A10-PyTorch-1, A10-PyTorch-2" |
|
PR_Github #59029 [ run ] triggered by Bot. Commit: |
|
PR_Github #59029 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #59050 [ run ] triggered by Bot. Commit: |
📝 WalkthroughWalkthroughThe proxy now monitors local worker process liveness, workers report process identities during initialization, and fatal shutdown can occur when a process dies while its MPI future remains pending. Tests cover pidfd, procfs, identity filtering, and proxy health behavior. ChangesWorker liveness monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker as MPI worker leader
participant Proxy as GenerationExecutorProxy
participant Monitor as WorkerProcessMonitor
participant Process as Local worker process
Worker->>Proxy: ready status with worker identities
Proxy->>Monitor: register identities
Monitor->>Process: open pidfd or read procfs state
Process-->>Monitor: exit event or changed process state
Monitor-->>Proxy: dead worker identity
Proxy->>Proxy: record fatal error and call pre_shutdown()
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/executor/test_fatal_error_health_check.py (1)
523-624: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo coverage gaps in
TestWorkerProcessMonitor.Well-covered overall, but missing:
- A test where
os.pidfd_openraises a genericOSError(notProcessLookupError) — should fall back to procfs registration (lines 122-123 inworker_process_monitor.py), currently untested.- A test for the zombie-state (
"Z") death branch specifically (only start-time mismatch is exercised today, in both the pidfd post-open-validation andfind_dead_workerprocfs paths).As per path instructions, "suggest concrete list file names and whether coverage is sufficient, insufficient, or needs follow-up outside the PR": add these two cases to
tests/unittest/executor/test_fatal_error_health_check.py::TestWorkerProcessMonitor.🧪 Suggested additional tests
def test_pidfd_open_generic_oserror_falls_back_to_procfs(self, identity): if identity.start_time is None: identity = identity._replace(start_time=100) monitor = WorkerProcessMonitor() with patch.object(_monitor_mod.os, "pidfd_open", side_effect=OSError("EMFILE"), create=True): monitor.register([identity]) assert identity in monitor._procfs_identities def test_zombie_state_is_detected(self, identity): if identity.start_time is None: identity = identity._replace(start_time=100) monitor = WorkerProcessMonitor() with ( patch.object(_monitor_mod.os, "pidfd_open", None, create=True), patch.object(_monitor_mod, "_read_process_state", return_value=("Z", identity.start_time)), ): monitor.register([identity]) assert monitor.find_dead_worker() == identity🤖 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 `@tests/unittest/executor/test_fatal_error_health_check.py` around lines 523 - 624, Add two cases to TestWorkerProcessMonitor: verify a generic OSError from os.pidfd_open falls back to procfs registration and retains the identity in _procfs_identities, and verify a procfs state of "Z" is reported by find_dead_worker as the dead identity. Preserve the existing start-time setup and mocking patterns used by the neighboring tests.Source: Path instructions
🤖 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.
Nitpick comments:
In `@tests/unittest/executor/test_fatal_error_health_check.py`:
- Around line 523-624: Add two cases to TestWorkerProcessMonitor: verify a
generic OSError from os.pidfd_open falls back to procfs registration and retains
the identity in _procfs_identities, and verify a procfs state of "Z" is reported
by find_dead_worker as the dead identity. Preserve the existing start-time setup
and mocking patterns used by the neighboring tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 00a922fa-3189-4f1f-8116-5cbf65686d17
📒 Files selected for processing (4)
tensorrt_llm/executor/proxy.pytensorrt_llm/executor/worker.pytensorrt_llm/executor/worker_process_monitor.pytests/unittest/executor/test_fatal_error_health_check.py
JunyiXu-nv
left a comment
There was a problem hiding this comment.
Overall LGTM. CC @QiJune for viz
What changed
MpiPoolSessionworkers with pidfds, with a/procidentity check as fallback.GenerationExecutorProxy.check_health()and the background error monitor, then initiate the existing nonblocking shutdown path.Why
NVBug 6435642 tracks a failure mode where the
mpi4py.futures.serverprocess is killed by OOM/SIGKILL while its future remains pending and no error reaches the proxy error queue. The parent TRT-LLM/Dynamo worker therefore remains alive and continues reporting healthy even though inference capability is lost.PR #12718 added the fatal-error propagation and shutdown foundation, but hard-killed MPI workers can bypass its future/error-queue checks. Active OS process-liveness monitoring closes that gap for the locally spawned
MpiPoolSessiontopology.Impact
When an engine-core MPI worker is killed,
check_health()now returnsFalse, records a fatal error containing the worker rank and PID, and allows embedding runtimes such as Dynamo to remove and recover the failed worker instead of routing requests that will hang.Validation
pytest -q tests/unittest/executor/test_fatal_error_health_check.py --confcutdir=tests/unittest/executor -k 'not TestOpenAIHealthEndpoint'str | Noneannotation inscripts/check_test_list.py.NVBug: 6435642
Summary by CodeRabbit
New Features
Tests