[TRTLLM-13409][fix] give the benchmark-disagg fill gate's retry loop a deadline - #17202
Conversation
10e576e to
d7c12ca
Compare
JunyiXu-nv
changed the title
|
/bot run |
|
PR_Github #63472 [ run ] triggered by Bot. Commit: |
|
PR_Github #63472 [ run ] completed with state
|
…a deadline `_check_benchmark_disagg_gate()` retries until the fill completes, with no bound. While it spins the job is invisible: the `continue` it drives sits before `iter_counter += 1`, so the iteration counter freezes while wall-clock advances, and the only outward sign is a stream of byte-identical iteration lines ~110 ms apart -- this gate's own `time.sleep(0.1)` seen from outside. Archived wedges show tens of thousands of them before Slurm kills the job. Bound the retry on LACK OF PROGRESS rather than on elapsed time: a fill that is merely slow keeps resetting the clock and is never killed; only a fill that makes no progress at all for the whole window raises. `TRTLLM_BENCHMARK_DISAGG_FILL_STALL_SEC=0` disables the bound; the default is 600 s. Raising surfaces the stall through the executor loop's existing error path rather than adding a second one, and the rank that raises names itself. Tests (no GPU): the bound fires after the window; the message names the rank and the knob; a slow-but-advancing fill survives 60 s against a 5 s window; progress resets the clock; 0 disables it; and a single no-progress pass only arms the clock rather than raising. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
…st on CPU Two follow-ups before review. float() accepts "nan" and "inf", and each breaks the new bound in an opposite direction. Every nan comparison is False, so `nan <= 0` does not disable the bound and `stalled_for < nan` does not defer it -- control falls straight through to the raise, firing on the second consecutive stalled call (~0.1s) instead of after the 600s window, which destroys exactly the margin the default exists to give. inf is the mirror: `stalled_for < inf` is always True, so the bound never fires and is silently equivalent to 0. Reject both and fall back to the default, the same guard merged for TLLM_RANK_CRASH_HARD_KILL_GRACE in NVIDIA#16592. The tests are pure monkeypatch with no engine and no GPU, but lacked the cpu_only marker, so the l0_cpu `unittest/_torch/executor` entry collected nothing and they ran only on the h100/b300/gb300 stages. Add the marker so they also run in the CPU stage, where they belong. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
d7c12ca to
b992d6a
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
Included review availability: Your plan provides up to 12 included reviews per hour; 11 remain after this review. WalkthroughThe PR adds configurable stall detection to the benchmark disaggregated-fill gate. It validates the timeout, tracks no-progress intervals, raises diagnostic errors after expiry, and adds CPU-only tests for configuration and retry behavior. ChangesDisaggregated-fill stall detection
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change adds a deadline for a benchmark fill retry loop and preserves progress-based operation; no actionable merge-blocking risk remains beyond normal checks and review. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tensorrt_llm/_torch/pyexecutor/py_executor.py (1)
4089-4097: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftTrack asynchronous transfer progress in the fill-gate deadline.
The deadline resets only for synchronous transfer completion. A fill with periodic asynchronous completions can fail after the original timeout window even though it continues to advance.
tensorrt_llm/_torch/pyexecutor/py_executor.py#L4089-L4097: propagate async completion progress from the transfer-status poll into the model-parallel gate status before calling_fail_if_fill_gate_stalled().tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py#L71-L148: add a CPU-only test that completes one async transfer, leaves another incomplete, and verifies that the deadline starts a new window.🤖 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/_torch/pyexecutor/py_executor.py` around lines 4089 - 4097, Update the fill-gate loop around _fail_if_fill_gate_stalled() in tensorrt_llm/_torch/pyexecutor/py_executor.py:4089-4097 to include asynchronous completion progress from the transfer-status poll when determining model-parallel progress, so periodic async completions reset the deadline. Add a CPU-only test in tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py:71-148 that completes one async transfer while another remains incomplete and verifies a new deadline window begins.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.
Inline comments:
In `@tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py`:
- Around line 43-148: Add complete type annotations to the new _Clock methods,
_executor, and _spin, including parameter and return types; use -> None for
procedural methods. Annotate all nine test functions with their fixture
parameters and -> None, preserving the existing test behavior and fixtures.
---
Outside diff comments:
In `@tensorrt_llm/_torch/pyexecutor/py_executor.py`:
- Around line 4089-4097: Update the fill-gate loop around
_fail_if_fill_gate_stalled() in
tensorrt_llm/_torch/pyexecutor/py_executor.py:4089-4097 to include asynchronous
completion progress from the transfer-status poll when determining
model-parallel progress, so periodic async completions reset the deadline. Add a
CPU-only test in
tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py:71-148 that
completes one async transfer while another remains incomplete and verifies a new
deadline window begins.
🪄 Autofix
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: 85766003-4a91-472b-93f2-d67d3c4b9975
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/py_executor.pytests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py
|
PR_Github #65516 [ run ] triggered by Bot. Commit: |
|
PR_Github #65516 [ run ] completed with state
|
…t now needs The gate now calls _fail_if_fill_gate_stalled(), and MockBenchmarkExecutor binds the gate methods off PyExecutor without having that one or the two attributes it reads. 12 CPU-Generic failures in pipeline 53254, all "AttributeError: 'MockBenchmarkExecutor' object has no attribute '_fail_if_fill_gate_stalled'". Only visible after the rebase: test_benchmark_disagg.py arrived on main after this branch was cut, so the pre-rebase runs never exercised it. Bind the method and seed the state with the bound disabled. That is right twice over: these tests assert retry semantics rather than the deadline, which test_disagg_fill_gate_stall_bound.py covers, and they patch the whole `time` module -- an enabled bound would do arithmetic on a Mock. The `timeout_s <= 0` guard returns before the clock is read, so the patched module is never touched. Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #65569 [ run ] triggered by Bot. Commit: |
|
PR_Github #65569 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #65615 [ run ] triggered by Bot. Commit: |
|
PR_Github #65615 [ run ] completed with state
|
|
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. |
|
Merged main ToT ( Rationale: If the render is still empty after this, the stale-tree theory is wrong and it needs test-db owners. /bot run |
|
/bot run |
|
PR_Github #68375 [ run ] triggered by Bot. Commit: |
|
PR_Github #68375 [ run ] completed with state
|
|
Merging main ToT fixed the GB10 problem — that stage no longer renders an empty list, and tests actually executed for the first time across four runs. Pipeline #55798:
The remaining failure is unrelated to this PR — a timeout in That exact parametrization is already waived under nvbugs/6608387, but only scoped to cc test owners: worth considering whether the |
|
/bot run |
|
PR_Github #68407 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #68455 [ run ] triggered by Bot. Commit: |
|
PR_Github #68407 [ run ] completed with state |
…deadlock Signed-off-by: JunyiXu-nv <219237550+JunyiXu-nv@users.noreply.github.com>
|
/bot run |
|
PR_Github #68490 [ run ] triggered by Bot. Commit: |
|
PR_Github #68455 [ run ] completed with state |
|
PR_Github #68490 [ run ] completed with state
|
|
/bot run |
|
PR_Github #68522 [ run ] triggered by Bot. Commit: |
|
/bot run |
|
PR_Github #68547 [ run ] triggered by Bot. Commit: |
|
PR_Github #68547 [ run ] completed with state
|
|
/bot run |
|
PR_Github #68572 [ run ] triggered by Bot. Commit: |
|
PR_Github #68572 [ run ] completed with state
|
|
/bot run |
|
PR_Github #68707 [ run ] triggered by Bot. Commit: |
|
PR_Github #68707 [ run ] completed with state |
5 participants
The bug
_check_benchmark_disagg_gate()retries until the fill completes, with no bound.While it spins the job is effectively invisible. The
continuethe gate drives sits beforeiter_counter += 1, so the iteration counter freezes while wall-clock advances. The only outward sign is a stream of byte-identical iteration lines about 110 ms apart — which is this gate's owntime.sleep(0.1)observed from outside.Archived wedges show tens of thousands of those lines before Slurm kills the job. One example: 48,486 identical lines over ~90 minutes, with
num_scheduled_requests,kv_cache_utilandcurrank_total_requestsconstant to three decimals, andhost_step_time = 110.14ms.Correction (2026-08-05): those archived wedges are NOT what this PR fixes
A log analysis of 12 of those stages (GB300 disagg perf-sanity, builds 2859-2863)
root-caused them to a different bug, since fixed on main by
37a7c09818("[nvbugs/6510284][fix] Clamp benchmark fill target in PyExecutor", #16961).
There, gate condition (A)
num_fetch_requests >= benchmark_req_queues_sizewasarithmetically unsatisfiable: while the gate is closed nothing completes, so the
fetch count is bounded by
tp_size x max_batch_size, and the harness could set thetarget above that bound (4301 vs 4096, 180 vs 128, 666 vs 512, 8 vs 1). #16961 clamps
the target to the capacity, and the hang burn on those stages fell from ~338 to
~17 GPU-h/day.
So the 110 ms spin signature quoted above is real, but it is that bug's fingerprint,
not evidence of an unbounded wait that survives #16961. I am leaving the description
of the signature in place because it is still exactly what this bound would surface.
Why this PR is still worth having
The gate has three conditions. #16961 makes (A) always reachable. It does nothing
for:
kv_cache_transceiver.check_gen_transfer_complete().Both can stall indefinitely, and the same analysis found that the machinery meant to
bound them cannot fire in the shipped
gen_onlyconfiguration:kv_transfer_timeout_msis 600 s but is unreachable code on the GEN side under
TRTLLM_DISABLE_KV_CACHE_TRANSFER_OVERLAP=1(_recv_disagg_gen_cachereturns beforethe arming block), and in-flight cancellation is separately disabled by
transceiver_runtime: PYTHON.So a (B)/(C) stall today is still an unbounded, silent wait. That is the case this
bound covers. I have no archived instance of a (B)/(C) stall to point at -- the
justification is structural, not empirical, and reviewers should weigh it on that basis.
Related and worth doing alongside: the gate's own diagnostic at
_is_benchmark_disagg_fill_completenames the blocking ranks and per-state counts butis
logger.debug, so 90 minutes of spinning emitted zero lines about the cause. That isnot in this PR.
The fix
Bound the retry on lack of progress, not on elapsed time.
A fill that is merely slow keeps resetting the clock and is never killed. Only a fill that makes no progress at all for the whole window raises.
TRTLLM_BENCHMARK_DISAGG_FILL_STALL_SEC=0disables the bound; the default is 600 s.Raising surfaces the stall through the executor loop's existing error path rather than adding a second one, and the rank that raises names itself in the message.
Tests
tests/unittest/_torch/executor/test_disagg_fill_gate_stall_bound.py— no GPU:0disables the bound;What the first attempt got wrong
Worth recording, because the mistake is easy to repeat.
The original claim was that
_handle_kv_transfer_timeouts_synced()— the drain that would surface the timeout — sits after the gate'scontinue, so moving it above would let the existing error propagate. Two things were wrong:The state named was wrong.
_check_kv_transfer_timeout()setsreq.py_kv_transfer_timed_out, notDISAGG_TRANS_ERROR. That state is set on the transfer-status path (_update_sampler_state_for_disagg_gen_request,_check_disagg_gen_cache_transfer_status), which is unrelated to the timeout.The fix would have done nothing.
_pending_timed_out_requests— the buffer the drain reads — has exactly one populator,_handle_responses(). That is also downstream of thecontinue(L4156 in_executor_loop, reached via_process_previous_batch()at L4685 in_executor_loop_overlap). So during a spin the buffer is never filled, and the relocated drain would have run atp_allgatheron an empty list every iteration — pure cost, no effect.The verification error was checking that the drain was downstream of the
continueand stopping there, without checking where the buffer was filled. The original unit tests passed because the stub modelled the drain as clearing the error directly, which encoded the assumption rather than testing it.Status
Draft — not yet validated on multi-GPU hardware. The reasoning is from source and archived wedge logs; the unit tests cover the bound's arming, firing and reset, not a real stalled transfer.
Dev Engineer Review
_check_benchmark_disagg_gate().TRTLLM_BENCHMARK_DISAGG_FILL_STALL_SECfor overrides.0as disabled.QA Engineer Review
MockBenchmarkExecutorto support the stall-checking logic.tests/integration/test_lists/or associatedtest-db/orqa/files.