[https://nvbugs/6438685][fix] Prevent leaked ADP pad dummy from crashing the executor - #16161
[https://nvbugs/6438685][fix] Prevent leaked ADP pad dummy from crashing the executor#16161lingjiew wants to merge 5 commits into
Conversation
With attention DP enabled, when any rank schedules an empty batch,
can_queue goes False on every rank and both _forward_step and
_update_request_states are skipped for that iteration. The pad dummy
inserted by _pad_attention_dp_dummy_request is only removed inside
_update_request_states_tp, so it survives ("leaks") into the next
iteration: gather_all_rank_states counts it, the rank can exceed
expected_num_active_requests (hard-capped by pp_size * max_batch_size),
and the assert in _pad_attention_dp_dummy_request kills the executor.
Observed in disaggregated serving under a KV-transfer burst: a rank
holding only GENERATION_TO_COMPLETE requests is counted as schedulable
by _count_schedulable_active_requests (so no pad dummy is inserted for
it), while MicroBatchScheduler refuses those requests via
no_schedule_after_state and schedules batch=0 on that rank. That
produces the fleet-wide skipped iteration; ranks whose active requests
were all awaiting KV transfer had inserted pad dummies, which then leak
and trip the assert on the following iteration.
Fix, two parts:
- _fetch_new_requests: terminate any pad dummy that survived a skipped
iteration before gathering rank states, mirroring the removal
sequence in _update_request_states_tp.
- _count_schedulable_active_requests: align the pad predicate with the
scheduler's no_schedule_after_state bound by excluding requests at
state >= GENERATION_TO_COMPLETE, so a rank that cannot actually
schedule anything gets its pad dummy (keeping it inside the
collectives) instead of forcing a fleet-wide skipped iteration.
Unit tests: regression tests for both parts (leaked-dummy strip in
_fetch_new_requests; pad dummy inserted for a TO_COMPLETE-only rank in
disaggregated mode), and the ADP mock request factories now provide
the state_value attribute the new predicate reads.
Validated on GB300 disaggregated serving (Qwen3.5 agentic benchmark,
42 cells x 3600 s incl. high-concurrency cache-pressure warmup): the
previously deterministic assert crash no longer reproduces.
Signed-off-by: Lingjie Wu <lingjiew@nvidia.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review feedback (lancelly): the cleanup in _fetch_new_requests guarantees the invariant defensively, but it would also silently absorb any future dummy-leak bug that the predicate fix does not cover, hiding it from asserts. Remove it and rely on the root-cause fix alone: with the pad predicate aligned to the scheduler's no_schedule_after_state bound, the fleet-wide skipped iteration that produced the leak no longer occurs on the disaggregated path. The non-disaggregated variant of the stall is deliberately left unchanged (existing counting semantics are pinned by test_pad_dummy_counts_generation_to_complete); it can be revisited if it is ever observed in aggregated serving. Also removes the regression test and stub for the deleted cleanup. Signed-off-by: Lingjie Wu <lingjiew@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe executor’s disaggregated scheduling count now excludes requests in or beyond ChangesDisaggregated schedulability
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant PyExecutor_run_pad
participant PyExecutor_count_schedulable
participant active_requests
participant scheduler
PyExecutor_run_pad->>PyExecutor_count_schedulable: count schedulable active requests
PyExecutor_count_schedulable->>active_requests: inspect request state_value
active_requests-->>PyExecutor_count_schedulable: return request states
PyExecutor_count_schedulable-->>PyExecutor_run_pad: return filtered count
PyExecutor_run_pad->>scheduler: queue pad dummy when needed
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chienchunhung
left a comment
There was a problem hiding this comment.
Thanks for catching and fixing this issue, while preserving the cap logic!
Also I think this PR potentially resolves https://nvbugspro.nvidia.com/bug/6438685. Can you include the bug info in PR title & description? Thanks!
| # Mirror the upper bound of the scheduler's schedulability window: | ||
| # MicroBatchScheduler refuses requests at state >= | ||
| # GENERATION_TO_COMPLETE (no_schedule_after_state), so counting them | ||
| # here as schedulable | ||
| # suppresses the pad dummy while the rank still schedules batch=0, | ||
| # which turns can_queue False fleet-wide and stalls every rank. | ||
| to_complete_value = LlmRequestState.GENERATION_TO_COMPLETE.value | ||
|
|
||
| def _is_pad_schedulable(req) -> bool: | ||
| if (req.is_disagg_generation_init_state | ||
| or req.is_disagg_generation_transmission_in_progress): | ||
| return False | ||
| return req.state_value < to_complete_value |
There was a problem hiding this comment.
By the way, this return predicate makes a rank containing only GENERATION_TO_COMPLETE requests enter the existing dummy-allocation path. Since both V1 and V2 add_dummy_requests() may return None, could you confirm that disaggregated ADP reserves sufficient dummy capacity while terminal requests still retain their resources?
There was a problem hiding this comment.
It can't be decided that disaggregated ADP reserves sufficient dummy capacity for now. This will be another change to reserve the dummy capacity. Added a warning here but not totally fix it in this PR.
…on failure Review follow-ups: - _count_schedulable_active_requests now mirrors the scheduler's FULL schedulability window [CONTEXT_INIT, GENERATION_TO_COMPLETE) instead of only the upper bound. The lower bound covers DISAGG_CONTEXT_WAIT_SCHEDULER (gen-first mode on the context server), which the scheduler refuses via no_schedule_until_state — the left-boundary mirror of the TO_COMPLETE mismatch. Disaggregated serving is decoder-only, so CONTEXT_INIT is the correct until-state here; the explicit disagg-generation guards are subsumed by the lower bound. - _pad_attention_dp_dummy_request: add_dummy_requests() may return None when the rank has no free cache resources (possible while non-schedulable requests still hold theirs). Guard it: log a warning and skip padding for this iteration instead of crashing on an unchecked [0]; the resource-holding requests complete within an iteration or two and padding succeeds on the retry. - Tests: left-boundary regression test (WAIT_SCHEDULER-only rank receives a pad dummy), allocation-failure test (no crash, no dummy), and the ADP mock sentinels now expose the real disagg state ints that the window comparison reads. Signed-off-by: Lingjie Wu <lingjiew@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Lingjie Wu <1219151854@qq.com>
yapf/ruff-format fixups exactly as emitted by the PR pre-commit run. Signed-off-by: Lingjie Wu <lingjiew@nvidia.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
It seems that this PR is a subset of #16279, could you please take a look at it? @lingjiew @chienchunhung |
|
…location failure Adopt the fix from PR NVIDIA#16161 (closed in favor of this PR): 1. _count_schedulable_active_requests (disaggregated mode): count only requests inside the scheduler's schedulability window [CONTEXT_INIT, GENERATION_TO_COMPLETE), mirroring no_schedule_until_state / no_schedule_after_state. States outside the window (awaiting KV transfer, DISAGG_CONTEXT_WAIT_SCHEDULER, GENERATION_TO_COMPLETE) cannot produce forward work; counting them suppresses the ADP pad dummy while the rank schedules batch=0, which turns can_queue False fleet-wide and stalls every rank (surfaces as the expected_num_active_requests assert or leaked pad dummies). Non-disaggregated behavior is unchanged. 2. _pad_attention_dp_dummy_request: add_dummy_requests returns None when no cache resources are free for even a 1-token dummy; skip padding for the iteration instead of crashing on an unchecked [0]. Signed-off-by: Xianjie <5410381+qiaoxj07@users.noreply.github.com>
Sure; will take a look today. |
Description
Resolves NVBug 6438685.
With attention DP enabled, a pad dummy request can leak across iterations and crash the executor with:
Observed as a deterministic crash in disaggregated serving (wide-EP generation servers, GB300) whenever a burst of KV-cache transfers coincides with requests finishing: 21/21 benchmark cells died within the first minutes of a high-concurrency agentic workload.
Root cause
Three interacting behaviors:
_count_schedulable_active_requestscounts requests at stateGENERATION_TO_COMPLETEas schedulable, butMicroBatchSchedulerrefuses them (no_schedule_after_state=GENERATION_TO_COMPLETE). A rank holding only such requests therefore gets no pad dummy yet schedules batch = 0.can_queueis "no zero in the all-gathered batch sizes", so that one rank turnscan_queueFalse on every rank;_forward_stepand_update_request_statesare both skipped. The only same-iteration dummy removal lives in_update_request_states_tp, so pad dummies inserted on other ranks (e.g. ranks whose requests are all awaiting KV transfer) survive into the next iteration.gather_all_rank_statescounts the leaked dummy; withexpected_num_active_requestscapped atpp_size * max_batch_size, a rank at cap + 1 leaked dummy trips the assert above. (The cap itself is correct and untouched.)Fix
_count_schedulable_active_requests: align the pad predicate with the scheduler's FULL schedulability window[CONTEXT_INIT, GENERATION_TO_COMPLETE)(disaggregated path), mirroring bothno_schedule_until_stateandno_schedule_after_state. The lower bound also coversDISAGG_CONTEXT_WAIT_SCHEDULER(gen-first mode on the context server) — the left-boundary mirror of the same mismatch. A rank that cannot actually schedule anything now receives its pad dummy and stays inside the collectives instead of forcing a fleet-wide skipped iteration — the leak-producing skip no longer occurs, so no dummy survives into the next round._pad_attention_dp_dummy_request: guard theadd_dummy_requests()return — both V1 and V2 managers may returnNonewhen the rank has no free cache resources (possible while non-schedulable requests still hold theirs). On failure, log a warning and skip padding for the iteration instead of crashing on an unchecked[0]; the resource-holding requests complete within an iteration or two and padding succeeds on the retry.Review note: an earlier revision also carried a defensive cleanup in
_fetch_new_requeststhat terminated any dummy surviving a skipped iteration. Per review feedback it was removed — such a net would also silently absorb future leak bugs the predicate fix does not cover, hiding them from the assert. The root-cause fix stands alone.Test Coverage
tests/unittest/_torch/executor/test_py_executor.py:test_pad_dummy_added_when_only_to_complete_requests_disagg— in disaggregated mode a rank holding onlyGENERATION_TO_COMPLETErequests now receives a pad dummy. The existingtest_pad_dummy_counts_generation_to_complete(non-disagg path, transceiverNone) is unchanged and still passes, since the predicate change is scoped to the disaggregated path.test_pad_dummy_added_when_only_wait_scheduler_requests_disagg— left-boundary mirror: a rank holding onlyDISAGG_CONTEXT_WAIT_SCHEDULERrequests receives its pad dummy.test_pad_dummy_allocation_failure_skips_padding—add_dummy_requests() -> Nonedegrades to a skipped iteration instead of a crash.state_valueattribute (with real disagg state ints) the window comparison reads.Notes / known limitations
state >= GENERATION_TO_COMPLETEbound also excludesGENERATION_COMPLETE,DISAGG_CONTEXT_TRANS_IN_PROGRESS,DISAGG_CONTEXT_COMPLETE, andDISAGG_GENERATION_WAIT_TOKENS— all likewise refused by the scheduler, so each exclusion is in the safe direction and extends the same fix to ctx-server / wait-tokens variants of the stall.None) keeps its existing counting semantics; a rank whose requests are all on their final token (GENERATION_TO_COMPLETEvia the overlap path) can in principle still cause a fleet-wide skipped iteration there. This is deliberately out of scope for this PR (the current focus is the disaggregated path, and the non-disagg semantics are pinned by an existing unit test); it can be revisited if the stall is ever observed in aggregated serving.PR Checklist
[None][fix] ...)