[TRTLLM-13409][fix] hard-kill all ranks when one rank's executor loop crashes#16592
[TRTLLM-13409][fix] hard-kill all ranks when one rank's executor loop crashes#16592JunyiXu-nv wants to merge 4 commits into
Conversation
03e5e9e to
cf7fdc1
Compare
|
/bot run --disable-fail-fast |
|
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 skipped from review as they are similar to previous changes (2)
WalkthroughAdds configurable rank-crash hard-kill handling with a grace period, optional watchdog, and defensive error handling. Executor crash cleanup now starts the watchdog before cleanup and invokes direct world termination afterward, with tests covering policy and ordering. ChangesRank crash handling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EventLoop
participant Watchdog
participant Cleanup
participant HardKill
EventLoop->>Watchdog: Arm after crash
EventLoop->>Cleanup: Execute local cleanup
EventLoop->>HardKill: Terminate world after cleanup
Watchdog->>HardKill: Trigger after grace period
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #60270 [ run ] triggered by Bot. Commit: |
|
PR_Github #60270 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60547 [ run ] triggered by Bot. Commit: |
|
PR_Github #60547 [ run ] completed with state
|
|
/bot run |
|
PR_Github #60580 [ run ] triggered by Bot. Commit: |
nv-xtf
left a comment
nv-xtf
left a comment
There was a problem hiding this comment.
LGTM — One non-blocking question inline about kill reachability if cleanup blocks on a PP send handle.
| # the grace inside lets the stashed error reach rank-local | ||
| # waiters and the ready handshake first, so the client sees | ||
| # the original exception rather than a bare worker death. | ||
| hard_kill_on_rank_crash(self.dist.world_size) |
There was a problem hiding this comment.
One reachability concern: the kill only runs after _executor_loop_cleanup() returns, but cleanup performs an unbounded wait() on pending PP send handles. If one of those handles hangs after the crash, this call is never reached, so peer ranks still rely on their 300s HangDetector — the behavior this PR aims to avoid. Could we start a separate watchdog thread before _executor_loop_cleanup() that waits for the grace period and then triggers the hard kill? This would ensure the kill still fires if cleanup blocks on a PP send handle.
There was a problem hiding this comment.
Thanks for review! That really makes sense, adapted a watchdog for the graceful clean up.
|
PR_Github #60580 [ run ] completed with state
|
cf7fdc1 to
ec5a475
Compare
… crashes When a rank's executor loop dies on an exception, the rank stops participating in collectives but nothing tells its peers: every peer blocks in its next collective until its own HangDetector fires 300s later, and the whole multi-GPU test session burns that long for an error that was already known (the A4 AutoDeploy catches are this signature: peers crash, the survivor wedges in ADP until the 300s backstop). Escalate at error time instead: after the loop's local cleanup has woken rank-local waiters, hard-kill the world via the ST-1 propagation path. A grace period (TLLM_RANK_CRASH_HARD_KILL_GRACE, default 10s, negative disables) lets the cleaner error paths win the race first -- the stashed error reaches rank-local response waiters, the init-phase ready handshake returns the real exception to the proxy, and the worker future completes with the original error -- so the client reports the actual failure rather than a bare worker death. Single-rank worlds are exempt (no peers to unblock), and the kill helper never raises (it runs in a finally where an exception would mask the loop's original error). Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…blocks or raises The kill sat after _executor_loop_cleanup() in the finally block, so it was skippable in exactly the situations it exists for: cleanup blocking without bound (unbounded wait() on a PP send handle wedged by the crash) or cleanup raising (aborting the finally before the kill). Either way peers fall back to burning 300s in their own HangDetectors. Arm a daemon watchdog thread BEFORE cleanup that fires the kill at crash + grace regardless of cleanup progress, and nest the post-cleanup kill in its own finally so a raising cleanup cannot skip it. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…pool-session shutdown test The test builds MpiPoolSession via __new__ (a real spawn is neither needed nor wanted), but MpiPoolSession.shutdown() now reads n_workers (added by NVIDIA#16456 while the test was in flight) and _wait_shutdown, both set only in __init__. Provide them explicitly and drop the waive. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
ec5a475 to
c0c1a43
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #60921 [ run ] triggered by Bot. Commit: |
|
PR_Github #60921 [ run ] completed with state
|
|
@coderabbitai review the code as qa engineer, can it handle multi nodes scenarios? in multi nodes scenarios, each node has multiple tasks. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Relevant module outlines ==='
ast-grep outline tensorrt_llm/_torch/pyexecutor/hang_detector.py --items all || true
ast-grep outline tensorrt_llm/_torch/pyexecutor/py_executor.py --items all --match 'PyExecutor|_event_loop_wrapper' || true
ast-grep outline tests/unittest/_torch/executor/test_hang_detector_kill.py --items all || true
echo '=== Crash-kill and propagation definitions/usages ==='
rg -n -C 5 --glob '*.py' \
'\b(propagate_hard_kill|hard_kill_on_rank_crash|start_rank_crash_kill_watchdog|_event_loop_wrapper|RANK_CRASH_KILL_GRACE_ENV)\b' \
tensorrt_llm tests
echo '=== Distributed topology / process-launch context ==='
rg -n -C 4 --glob '*.py' \
'\b(world_size|global_rank|local_rank|node_rank|MPI|mpirun|torchrun|rank)\b' \
tensorrt_llm/_torch/pyexecutor | head -n 500
echo '=== Relevant test body ==='
sed -n '1,420p' tests/unittest/_torch/executor/test_hang_detector_kill.pyLength of output: 106501 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== Hard-kill implementation ==='
sed -n '45,185p' tensorrt_llm/_torch/pyexecutor/hang_detector.py
echo '=== MPI helper definitions and related abort calls ==='
rg -n -C 8 --glob '*.py' \
'^\s*def (mpi_comm|mpi_disabled|global_mpi_rank|mpi_rank)\b|\b(Abort|abort)\s*\(' \
tensorrt_llm tests | head -n 600
echo '=== MPI helper implementation candidates ==='
fd -t f -e py . tensorrt_llm | xargs rg -l \
'^\s*def (mpi_comm|mpi_disabled|global_mpi_rank|mpi_rank)\b' | while IFS= read -r file; do
echo "--- $file"
rg -n -C 5 '^\s*def (mpi_comm|mpi_disabled|global_mpi_rank|mpi_rank)\b' "$file"
done
echo '=== Distributed test and launcher candidates ==='
fd -t f -e py . tests | xargs rg -n -i -C 3 \
'\b(mpirun|mpi4py|MPI\.COMM_WORLD|torchrun|multinode|multi-node|nnodes|nproc.per.node)\b' | head -n 800Length of output: 115054
What should work
Important limitation / riskIf
Without that launcher setting, tasks on other nodes can remain blocked until their independent hang detectors time out. Thus, multi-node correctness currently depends on deployment configuration rather than being fully enforced by this code. Coverage gap
QA recommendation before relying on this in productionAdd a multi-node failure-injection integration test with, for example, two nodes and at least two tasks per node. Force one rank’s executor loop to fail while the remaining ranks are waiting in a collective, then assert:
Also document that all ranks that must die together must belong to the same MPI communicator/job. This mechanism intentionally kills the whole participating world; it is not scoped per node or per individual task. |
|
@coderabbitai is |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🤖 web_search executed:
Length of output: 5224
Without either a successful communicator-wide MPI abort or Slurm step-wide failure propagation, peers on other nodes can remain hung in collectives until another timeout or external job cleanup occurs. For deterministic cluster behavior, I recommend launching the participating ranks in one srun --kill-on-bad-exit=1 ...Also ensure the job does not use Slurm documents that, when |
|
/bot run --disable-fail-fast |
|
PR_Github #61181 [ run ] triggered by Bot. Commit: |
|
PR_Github #61181 [ run ] completed with state
|
|
/bot run |
|
PR_Github #61695 [ run ] triggered by Bot. Commit: |
|
PR_Github #61695 [ run ] completed with state
|
5 participants
Dev Engineer Review
QA Engineer Review
hard_kill_on_rank_crash_event_loop_wrappercrash and clean-exit pathstest_pool_session_shutdown_never_blocks_after_releasetests/integration/test_lists/(test-db/orqa/), so CI/manual test-list coverage is not established.Description
When a rank's executor loop dies on an exception, the rank stops participating in collectives but nothing tells its peers: every peer blocks in its next collective until its own HangDetector fires 300s later, and the whole multi-GPU test session burns that long for an error that was already known (the A4 AutoDeploy catches are this signature: peers crash, the survivor wedges in ADP until the 300s backstop).
Single-rank worlds are exempt (no peers to unblock), and the kill helper never raises (it runs in a finally where an exception would mask the loop's original error).
Test Coverage
PR Checklist
Please review the following before submitting your PR:
PR description clearly explains what and why. If using CodeRabbit's summary, please make sure it makes sense.
PR Follows TRT-LLM CODING GUIDELINES to the best of your knowledge.
Test cases are provided for new code paths (see test instructions)
If PR introduces API changes, an appropriate PR label is added - either
api-compatibleorapi-breaking. Forapi-breaking, includeBREAKINGin the PR title.Any new dependencies have been scanned for license and vulnerabilities
CODEOWNERS updated if ownership changes
Documentation updated as needed
Update tava architecture diagram if there is a significant design change in PR.
The reviewers assigned automatically/manually are appropriate for the PR.
Please check this after reviewing the above items as appropriate for this PR.
GitHub Bot Help
To see a list of available CI bot commands, please comment
/bot help.