[https://nvbugs/6093911][fix] Fix disagg gen-only benchmark hang under ADP router imbalance - #13347
Conversation
|
/bot run --disable-fail-fast |
|
PR_Github #45018 [ run ] triggered by Bot. Commit: |
|
PR_Github #45018 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #45236 [ run ] triggered by Bot. Commit: |
16533e3 to
f98e2fd
Compare
|
PR_Github #45236 [ run ] completed with state |
📝 WalkthroughWalkthroughRefactors the benchmark-disagg fill gate readiness logic from counting generation requests to evaluating executor state, including active request states and KV cache transfer completion. In ADP mode, synchronizes readiness across TP ranks using allgather. Updates corresponding test suite to validate state-based predicate behavior and adds regression coverage for rank skew scenarios. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/unittest/_torch/executor/test_benchmark_disagg.py (1)
233-233: Optional: Linter warning about multiplication sign character.Static analysis flags the
×character as ambiguous (RUF002). While the mathematical notation is clearer for readers, you can silence this by using ASCIIxor escaping if strict linting is required.- (e.g. 31×256 + 1×255 = 8191 < 8192). The new state-based predicate + (e.g. 31*256 + 1*255 = 8191 < 8192). The new state-based predicate🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unittest/_torch/executor/test_benchmark_disagg.py` at line 233, Replace the non-ASCII multiplication character in the test string (e.g. "31×256 + 1×255 = 8191 < 8192") with an ASCII 'x' or escape it to satisfy the linter; update the literal in tests/unittest/_torch/executor/test_benchmark_disagg.py so occurrences of '×' become 'x' (or an escaped sequence) to silence RUF002 while preserving the intended text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/unittest/_torch/executor/test_benchmark_disagg.py`:
- Line 233: Replace the non-ASCII multiplication character in the test string
(e.g. "31×256 + 1×255 = 8191 < 8192") with an ASCII 'x' or escape it to satisfy
the linter; update the literal in
tests/unittest/_torch/executor/test_benchmark_disagg.py so occurrences of '×'
become 'x' (or an escaped sequence) to silence RUF002 while preserving the
intended text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c7e428f7-465d-4269-9384-c8905b912cbc
📒 Files selected for processing (2)
tensorrt_llm/_torch/pyexecutor/py_executor.pytests/unittest/_torch/executor/test_benchmark_disagg.py
Update the benchmark disagg fill design docs to reflect the final three-part PR NVIDIA#13347 fix: state-based gate, ADP router cap, and fill-phase fail-fast suppression. Add a dedicated note explaining why PR NVIDIA#12206 fail-fast must be disabled while the benchmark fill barrier is active. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
…stuck-state finding Document the v2.2 follow-up to PR NVIDIA#13347 and a separate latent issue discovered while validating it locally: * 07-fill-phase-flow-control.md (new): v2.2 production fix. Reintroduces the per-iteration `tp_size` admission cap that PR NVIDIA#12091 had as part of the blocking fill loop, and that PR NVIDIA#12208 implicitly removed when it eliminated that loop. Without it, on `dep32 × bs256 == con8192` the GEN server is OOM-killed during burst admission before any KV transfer can drain. Includes safety analysis, test coverage notes, and a tunable-cap discussion. * 08-fill-phase-stuck-state-finding.md (new): a separate latent deadlock observed during local 8-GPU validation, where the state-based gate stays closed because some active requests remain in INIT after CTX has finished prefilling. Explicitly framed as independent of v2.1 / v2.2 (with a per-patch coverage table proving none of those patches touch the deadlock's mechanism), with repro instructions and recommended next steps. * README.md: lineage table gains a v2.2 row; current-state section now enumerates four issues with v2.1/v2.2 attribution; documents table and reading order updated; "Relationship between the steps" notes v2.1 and v2.2 both fall under Step 1. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
…VIDIA#13347 Update the design docs to reflect that the v2.2 fill-phase admission cap ships in PR NVIDIA#13347 alongside the v2.1 three-part fix, instead of as a separate follow-up PR. The PR's CI gate cannot pass without v2.2 anyway, so the four commits all land together as fixes for the same nvbug-tracked wide-EP Kimi failure. Changes: * README.md: current-state section, lineage table v2.2 row, reading order, and "Relationship between the steps" all now reference PR NVIDIA#13347 as the home for both v2.1 and v2.2. * 07-fill-phase-flow-control.md: header PR reference updated from "follow-up #TBD" to "PR NVIDIA#13347 (4th commit)". * 08-fill-phase-stuck-state-finding.md: status line clarified to reference both v2.1 and v2.2 as the patches this finding is independent of. Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com> Made-with: Cursor
75d5c33 to
2cce23b
Compare
56bd785 to
a2c6107
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #46198 [ run ] triggered by Bot. Commit: |
|
PR_Github #46198 [ run ] completed with state
|
|
Some tests seem to fail due to your changes. For example: Also the release-check job is failing. Could you take a look? |
f2bd50b to
d408754
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #46460 [ run ] triggered by Bot. Commit: |
Some of the new tests introduced by this PR failed after rebasing to the latest HEAD; updated the tests.
Yup over the past few days I have been working closely with QA on the Kimi2 test. |
|
PR_Github #46460 [ run ] completed with state
|
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
82d65f8 to
0562b7c
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #48643 [ run ] triggered by Bot. Commit: |
…ertion Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #48652 [ run ] triggered by Bot. Commit: |
|
PR_Github #48652 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #48965 [ run ] triggered by Bot. Commit: |
|
PR_Github #48965 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #48977 [ run ] triggered by Bot. Commit: |
|
PR_Github #48977 [ run ] completed with state
|
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
|
/bot run --disable-fail-fast |
|
PR_Github #49233 [ run ] triggered by Bot. Commit: |
|
PR_Github #49233 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #49463 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #49486 [ run ] triggered by Bot. Commit: |
|
PR_Github #49463 [ run ] completed with state |
|
PR_Github #49486 [ run ] completed with state |
…r ADP router imbalance (NVIDIA#13347) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
…r ADP router imbalance (NVIDIA#13347) Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Summary by CodeRabbit
Bug Fixes
Tests
Summary
Fixes four ADP/NIXL regressions PR #12208 introduced when it removed the blocking fill loop, and corrects a long-latent KV oversubscription in the Kimi-2 8k1k perf-sanity test config that #12208's fill-phase changes exposed.
Production fixes (2 commits)
PR #12208 replaced the blocking fill loop with a non-blocking gate to fix an MPI hard deadlock. The replacement was correct for MPI but introduced four ADP/NIXL-side regressions, all addressed here:
(num_fetched ≥ queue_size) ∧ (no active req in INIT/TRANS) ∧ (no transceiver pending). Fires uniformly across ranks.max_num_active_requests(= per-rankmax_batch_size) on bothDefaultADPRouterandKVCacheAwareADPRouter— composes with upstream'sfair_share_multiplierloose cap. Fixes nvbug 6071070; without it, bulk admission can route pastmax_batch_sizeand leave excess requests stuck inINIT.tp_size-per-iter admission throttle in_pop_from_waiting_queue— restores the pre-[TRTLLM-11492][fix] Fix benchmark disagg deadlock by eliminating blocking fill loop #12208 conservative admission rate without reintroducing the blocking pattern. Scoped to non-warmup benchmark fill.Test config fix (1 commit): Kimi-2 GEN KV fraction
0.6 → 0.78Memory math (B200/GB200, 184 GiB HBM)
bs=256 × max_seq_len=9256At 0.6, only 203 of 256 max-batch sequences fit per rank — 53 are perpetually paused, costing ~21 % of decode throughput across the entire run. 0.78 is the floor that fits the workload while preserving the ~22 GiB needed for NIXL receive buffers, NCCL workspace, EPLB layer churn, and CUDA caching-allocator fragmentation.
Why the test was passing before PR #12208
Same YAML, same oversubscription — but timing differed:
DEFAULT_TIMEOUTwith 2–5 min of cluster-variance-dependent margin.concurrency = capacity). The flag adds only ~50 s ofall_gatheroverhead, but it ate the cluster-variance margin that had been hiding the oversubscription.The test was always running on luck. Bumping to 0.78 makes it do what it claims (
bs=256steady-state) instead of depending on the right side of a 2–5 min margin.Why this YAML change is bundled into this PR
The bump is a one-line test-config correctness fix that stands on its own — it would have been correct at any point in the test's lifetime. It belongs here because (a) the gate fixes are necessary but not sufficient for the Kimi-2 CI to reach green, and (b) the two changes share the same failure narrative.
Test coverage
Production fixes are covered by
tests/unittest/_torch/executor/test_benchmark_disagg.py(10 test classes, 28 tests, no GPU required):TestFillCompleteStateBased(4 tests, parameterized over the predicate's truth table),TestFillCompleteADP(3 tests, cross-rankallgather),TestFillCompleteADPRouterImbalance(2 tests, per-rank rebalancing edge cases),TestCheckBenchmarkDisaggGate(3 tests, wait-loop behavior),TestCanForwardGating(parameterized initial state)TestADPRouterPerRankCap(3 tests, parameterized over bothDefaultADPRouterandKVCacheAwareADPRouter: cap applied at max, no rank exceeds, no-op below max)TestFailFastSuppressedDuringFill(3 tests: suppressed during fill, resumes after, full(fill_active × is_warmup)matrix)tp_size-per-iter admission throttleTestBenchmarkFillAdmissionFlowControl(2 tests, parameterized overtp_size ∈ {1, 4, 32}: cap applied during fill, full capacity used after)TestPadAttentionDpDummyBenchmarkDisagg(4 tests, dummy-request skip during fill),TestPrepareAndScheduleBatchNoBlock(1 test, fetch called once per iter)TestFillPhaseEndToEnd::test_full_lifecycle(admission-cap → KV-transfer-complete → gate-opens → normal admission, exercising both production code paths together)The Kimi-2 perf-sanity test has been verified separately: https://nvbugspro.nvidia.com/bug/6093911?commentNumber=9
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)
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.