[None][fix] Tolerate ADP pad-dummy surplus instead of asserting - #17278
Conversation
|
@lancelly please review. |
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughThe change adds model-specific ADP dummy-fix gates, keeps overlap headroom limited to DeepSeek-V4, and replaces a fatal surplus-request assertion with warning-based normalization. Regression tests cover model gates and schedulable or unschedulable surplus requests. ChangesADP dummy handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (1)
tests/unittest/_torch/executor/test_py_executor.py (1)
1492-1492: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd return annotations to the new test functions.
Add
-> Noneto both test function definitions. The coding guidelines require annotations on every function.Also applies to: 1512-1512
🤖 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/_torch/executor/test_py_executor.py` at line 1492, Add the required `-> None` return annotation to both new test functions, including `test_pad_dummy_tolerates_surplus_over_expected_on_busy_rank` and the additional test identified by the review range.Source: Coding guidelines
🤖 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/_torch/executor/test_py_executor.py`:
- Line 1492: Add the required `-> None` return annotation to both new test
functions, including
`test_pad_dummy_tolerates_surplus_over_expected_on_busy_rank` and the additional
test identified by the review range.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1a5a7dff-c104-43ff-9e51-d407098c848c
📒 Files selected for processing (4)
tensorrt_llm/_torch/pyexecutor/_util.pytensorrt_llm/_torch/pyexecutor/py_executor.pytests/unittest/_torch/executor/test_py_executor.pytests/unittest/_torch/executor/test_seq_slot_sizing.py
7fa1eda to
e671221
Compare
|
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. |
lancelly
left a comment
lancelly
left a comment
There was a problem hiding this comment.
LGTM. @chienchunhung please take a look, thanks~
|
/bot run --disable-fail-fast |
|
PR_Github #63950 [ run ] triggered by Bot. Commit: |
|
PR_Github #63950 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64029 [ run ] triggered by Bot. Commit: |
longlee0622
left a comment
longlee0622
left a comment
There was a problem hiding this comment.
approve to unblock build generation
|
PR_Github #64029 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64050 [ run ] triggered by Bot. Commit: |
|
PR_Github #64050 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64114 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #64264 [ run ] triggered by Bot. Commit: |
Every disaggregated attention-DP generation cell of Qwen3.5-397B dies on
py_executor.py _pad_attention_dp_dummy_request
assert self.expected_num_active_requests >= len(self.active_requests)
Several ranks lose their event loop at once and the survivors
HangDetector-abort ~300 s later. Attention DP is required to reproduce: the
method returns early when it is off, and 54 TEP cells never hit it while 6
of 7 ADP cells did.
The router derives the expectation as
min(max(ceil(multiplier * fair_share), max(per_rank_loads)),
max_num_active_requests)
(AttentionDpRouter._expected_num_active_requests). The per-rank-load floor
normally keeps the value at or above this rank's own load, but the hard cap
is applied last, so any rank holding max_num_active_requests + 1 requests
breaks the relation. An attention-DP pad dummy does exactly that when it
survives an iteration that was skipped fleet-wide: can_queue False skips
both _forward_step and _update_request_states, _update_request_states_tp is
the only place the dummy is removed, and the next gather_all_rank_states
counts the survivor.
The measured signature matches that derivation. Across 13 cells (dep8,
dep16, dep32; max_batch_size 4 to 64), 1132 occurrences:
len(active_requests) - expected_num_active_requests is exactly 1 every
single time, and expected_num_active_requests always equals max_batch_size.
Inside this method the expectation is consumed only by the idle-rank test,
and a rank holding surplus requests needs no dummy, so tolerate the surplus
and warn instead of asserting. The tolerated value is clamped into a local
so downstream consumers still observe the router's number. Returning early
would be wrong: _count_schedulable_active_requests excludes requests still
in KV transfer, so a rank with many active requests can still have none
schedulable and legitimately need a dummy.
Also add qwen3_5_moe to the gate that scopes the existing ADP dummy fixes.
Those branches all sit after the assert, so widening the gate alone does
not stop the crash - measured, the run still died on the same line with the
widened gate in place - but the model does need them.
should_enable_dsv4_overlap_headroom is deliberately pinned to deepseek_v4
rather than reusing the widened gate, because it doubles max_num_sequences
and changes the memory envelope.
Validated on GB300 disaggregated serving (Qwen3.5-397B-A17B-NVFP4-V2, 9 ADP
cells, 900 to 3600 s each): the tolerance fires 10 to 273 times per cell,
0 AssertionError, and 66352 decode iterations on a previously fatal cell.
Signed-off-by: Lingjie Wu <lingjiew@nvidia.com>
e671221 to
d2450b8
Compare
|
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. |
|
/bot run --disable-fail-fast |
|
PR_Github #64309 [ run ] triggered by Bot. Commit: |
|
PR_Github #64264 [ run ] completed with state |
|
PR_Github #64309 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
1 similar comment
|
/bot run --disable-fail-fast |
|
/bot run --disable-fail-fast |
|
PR_Github #64411 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #64411 [ run ] completed with state
|
|
PR_Github #64449 [ run ] triggered by Bot. Commit: |
|
PR_Github #64449 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64497 [ run ] triggered by Bot. Commit: |
|
/bot run --disable-fail-fast |
|
PR_Github #64497 [ run ] completed with state
|
|
/bot run --disable-fail-fast |
|
PR_Github #64557 [ run ] triggered by Bot. Commit: |
|
PR_Github #64562 [ run ] triggered by Bot. Commit: |
|
PR_Github #64557 [ run ] completed with state |
|
PR_Github #64562 [ run ] completed with state |
6 participants
Description
Every disaggregated attention-DP generation cell of Qwen3.5-397B dies on
Several ranks lose their event loop simultaneously and the survivors HangDetector-abort ~300 s later. Attention DP is required to reproduce — the method returns early when it is off, and 54 TEP cells never hit it while 6 of 7 ADP cells did.
Why the invariant breaks
AttentionDpRouter._expected_num_active_requestscomputesThe per-rank-load floor normally keeps the value at or above this rank's own load — but the hard cap is applied last, so any rank holding
max_num_active_requests + 1requests breaks the relation.An attention-DP pad dummy does exactly that when it survives an iteration that was skipped fleet-wide:
can_queue == Falseskips both_forward_stepand_update_request_states,_update_request_states_tpis the only place the dummy is removed, and the nextgather_all_rank_statescounts the survivor.Measured signature
Across 13 cells (dep8 / dep16 / dep32,
max_batch_size4–64), 1,132 occurrences:len(active_requests) - expected_num_active_requestsexpected_num_active_requestsmax_batch_sizeBoth follow from the formula above. A random over-subscription would not produce a surplus that is always exactly one, on every topology, at every batch size.
Changes
py_executor.py— replace the bareassertwith a warning. Inside this method the expectation is consumed only by the idle-rank test, and a rank holding surplus requests needs no dummy. The tolerated value is clamped into a local so downstream consumers still observe the router's number.Returning early instead would be wrong:
_count_schedulable_active_requestsexcludes requests still in KV transfer, so a rank with many active requests can still have none schedulable and legitimately need a dummy. There is a test for that._util.py— addqwen3_5_moetoshould_enable_dsv4_adp_dummy_fixes. Those branches all sit after the assert, so widening the gate alone does not stop the crash (measured: the run still died on the same line with the widened gate in place), but the model does need them.should_enable_dsv4_overlap_headroomis deliberately pinned todeepseek_v4rather than reusing the widened gate — it doublesmax_num_sequencesand changes the memory envelope. A test covers that it does not leak.Relationship to prior work
25a2422c12, "Scope overlap fixes to DeepSeek-V4"), scoped them tomodel_type == "deepseek_v4". That commit is why non-DSv4 ADP + disagg models fall back to the pre-[https://nvbugs/6438685][fix] Prevent leaked ADP pad dummy from crashing the executor #16161 behavior._fetch_new_requests; that part was dropped during review in favour of the root-cause predicate fix alone. New data point: with that predicate fix and_finalize_adp_dummy_allocationboth active, the leak still occurs 10–273 times per cell — so the predicate fix is necessary but not sufficient, and something must tolerate or reap the survivor.Happy to fold a reap back in instead of tolerating if reviewers prefer that shape; the warning is deliberately per-occurrence (not
warning_once) so a persistent leak stays visible, which was the original review concern.Test Coverage
tests/unittest/_torch/executor/test_py_executor.pytest_pad_dummy_tolerates_surplus_over_expected_on_busy_rank— surplus tolerated, no dummy, router value not mutatedtest_pad_dummy_still_added_when_surplus_requests_are_unschedulable— tolerating must not short-circuit paddingtests/unittest/_torch/executor/test_seq_slot_sizing.pytest_dsv4_adp_dummy_fix_gate—qwen3_5_moeon, PP off,llamaunaffectedtest_dsv4_overlap_headroom_gate— widened gate does not leak into the headroom gateSilicon validation
GB300 disaggregated serving, Qwen3.5-397B-A17B-NVFP4-V2, 9 ADP cells, 900–3600 s each: tolerance fires 10–273 times per cell, 0
AssertionError, 66,352 decode iterations on a cell that was previously fatal within minutes. Before: 6 of 7 ADP cells died.PR Checklist
Dev Engineer Review
_pad_attention_dp_dummy_requestwith a warning and local clamping.qwen3_5_moe.deepseek_v4path.QA Engineer Review
Added tests:
test_pad_dummy_tolerates_surplus_over_expected_on_busy_ranktest_pad_dummy_still_added_when_surplus_requests_are_unschedulableAdded parameterized
qwen3_5_moecoverage for single-stage and multi-stage pipeline gating. Existingdeepseek_v4overlap-headroom coverage remains in place.No
test-db/orqa/entries were added. The new test functions are not listed in the provided test-list changes. GB300 validation covered nine Qwen3.5 ADP cells without anAssertionError.Verdict: needs follow-up