[https://nvbugs/6487039][fix] Generalize ADP dummy lifecycle - #16921
[https://nvbugs/6487039][fix] Generalize ADP dummy lifecycle#16921chienchunhung wants to merge 4 commits into
Conversation
|
/bot run --disable-fail-fast --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2" |
|
PR_Github #62036 [ run ] triggered by Bot. Commit: |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change generalizes ADP dummy handling, makes scheduler state ranges explicit, and propagates runtime sequence-slot capacity into model loading, MRoPE caches, speculative metadata, guided decoding, and executor padding behavior. ChangesRuntime capacity and ADP scheduling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ModelEngine
participant ModelLoader
participant ModelConfig
participant PyExecutor
participant RequestScheduler
ModelEngine->>ModelLoader: pass max_num_seq_slots
ModelLoader->>ModelConfig: attach runtime slot capacity
ModelEngine->>PyExecutor: provide generic ADP dummy-fix flag
PyExecutor->>RequestScheduler: check request schedulability
RequestScheduler-->>PyExecutor: return state-range eligibility
PyExecutor->>PyExecutor: allocate or finalize ADP padding dummy
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
PR_Github #62036 [ run ] completed with state |
|
Flagging one thing before this goes further: the gate widening isn't inert for non-DeepSeek-V4 models in the disagg + attention-DP case, which is the case it targets. Comparing against
That combination is what #16279 deliberately scoped to DSv4, and the PR description acknowledges as much. The test change makes this harder to catch rather than easier: Minor, unrelated: this removes the same |
|
/bot run --disable-fail-fast |
|
The dummy path is generalized but the paired 2x seq-slot headroom stays DSv4-only, worth generalizing both, or gating on the overlap scheduler. Could we also land the code fix alone and leave the waiver removal to a follow-up? |
Thanks. I added Existing allocation-failure and rollback tests cover the other two behavior changes. |
tburt-nv
left a comment
There was a problem hiding this comment.
The whole unittest/_torch/executor directory is covered in the test-db.
IIUC these are separate lifecycle conditions rather than paired requirements. The dummy fix is needed when rank-local padding or allocation disagrees, including this Kimi configuration where overlap is disabled. The 2× slot pool addresses overlap backfill before previous-iteration slot release; this case uses MTP0 with Gating dummy handling on overlap would therefore disable the reported fix, while generalizing slot headroom would require broader validation of all |
|
PR_Github #63137 [ run ] completed with state
|
|
/bot run --disable-fail-fast --stage-list "A10-Build_Docs, B300-PyTorch-2" |
|
PR_Github #63244 [ run ] triggered by Bot. Commit: |
|
PR_Github #63244 [ run ] completed with state |
|
/bot run --disable-fail-fast --stage-list "A10-Build_Docs, B300-PyTorch-2" |
|
PR_Github #63557 [ run ] triggered by Bot. Commit: |
|
PR_Github #63557 [ run ] completed with state |
8a8286c to
36b2c48
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #63852 [ run ] triggered by Bot. Commit: |
BowenFu
left a comment
There was a problem hiding this comment.
Scheduler eligibility now drives ADP counting, mixed-rank rollback has focused coverage, overlap capacity reaches each slot-indexed consumer, and the waiver change is removed. Approved; please merge after full CI passes.
|
PR_Github #63852 [ run ] completed with state
|
|
Hi @chienchunhung , thanks for the effort. Please ensure that multi-GPU testing is fully validated. The new schedulable window excludes the in-transfer states, so a draining context server now builds a full-length context dummy every iteration. Fail-soft, but new work for every non-DeepSeek model. Can the role be re-evaluated while draining? |
brnguyen2
left a comment
There was a problem hiding this comment.
Approving — the comments below are optional touch-ups, not blockers.
The fix matches the failure mechanism and the generalization is the right direction — delegating the ADP count to the scheduler's own state contract removes the duplicated window that caused the desync. Two things to settle before merge:
- Blast radius: the gate widens from one model to every non-PP disagg ADP deployment, and the 2× slot sizing now applies to every non-PP overlap config (the default). Targeted B200 stages passed; please confirm pipeline #51225 lands green before merging, since that's the first full-coverage run of the generalized path.
- The description is thorough and honest about what this does and doesn't fix (test stays waived) — appreciated.
Remaining comments are inline; nothing blocking.
|
The extra slots are on by default for every non-pipeline setup now, but the bound worked out on this thread is a small constant, not double. The biggest buffer is allocated before the cache pool is sized from free memory, so this shrinks the cache for every default speculative setup. |
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
36b2c48 to
6408929
Compare
|
/bot run --disable-fail-fast --stage-list "DGX_B200-4_GPUs-PyTorch-3, DGX_B200-8_GPUs-PyTorch-1" |
|
PR_Github #64417 [ run ] triggered by Bot. Commit: |
Hi @Shixiaowei02 Thanks for raising this good point. The generic non-overlap ADP path now derives forward intent from scheduler-eligible real requests on every iteration and combines a scalar context/generation/none intent across ADP ranks. A rank that is only draining transfers therefore adds no dummy when no peer has compute, or adds a correctly shaped dummy matching the peer’s context or generation work. The additional collective is limited to non-PP, non-overlap disaggregated ADP. The existing overlap path remains unchanged and does not incur this collective. I added coverage for transfer-only draining, context-role re-evaluation, and preservation of the overlap behavior. I will validate the resulting head with targeted multi-GPU CI.
The generic 2 * max_batch_size expansion has been removed. Normal non-PP configurations retain max_batch_size, while the extra headroom remains opt-in only for the previously validated DeepSeek-V4 one-model MTP overlap path. PP retains its existing The broader overlap bound and small-constant solution remain follow-up scope in #17278. |
|
PR_Github #64417 [ run ] completed with state |
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2" |
|
PR_Github #64501 [ run ] triggered by Bot. Commit: |
|
PR_Github #64501 [ run ] completed with state
|
Summary
Fixes NVBUG 6487039.
This PR fixes the model-independent, non-overlap disaggregated attention-DP dummy lifecycle. It generalizes the transactional dummy handling introduced for DeepSeek-V4 in #16279 without enabling generic overlap headroom.
The affected GB300 Kimi E2E test remains waived. This PR fixes the dummy-request assertion and leak, but not the separate Python-transceiver liveness and performance failures also exposed by that workload.
Failure mechanism
The scheduler admits only requests in its configured scheduling-state window. The legacy active-request count could nevertheless treat transfer, wait, or terminal requests outside that window as schedulable. A rank holding only those requests could omit a required pad dummy while peer attention-DP ranks allocated tentative dummies.
If the TP-wide queue decision rejected that iteration, the tentative peer dummies were not rolled back. They accumulated until the executor hit:
The subsequent
threads can only be started onceerror and HangDetector/MPI abort were recovery consequences of the leaked dummies.A sticky context/generation role introduced another draining hazard: transfer-only requests could cause a context server to allocate a full-length context dummy even when no rank intended to run a forward pass.
Fix
ENCODER_INITboundary for encoder-decoder models.none/generation/context forward intent every iteration and combine it with one scalar TPMAXreduction. Transfer-only ranks add no dummy when no peer has real forward work; otherwise they add a dummy matching the peer's compute role.This PR does not expand sequence-slot capacity for default non-PP configurations. Normal non-PP configurations remain at
max_batch_size; PP remains atpp_size * max_batch_size. The existing2 * max_batch_sizeheadroom remains opt-in only for the validated DeepSeek-V4 one-model MTP overlap configuration.Verification
Conclusion: The NVBUG 6487039 dummy-request lifecycle defect and its reported assertion/recovery signature are fixed and regression-validated. The complete GB300 Kimi E2E test is not verified passing end-to-end and remains waived because it still encounters independent KV-transfer/Python-transceiver liveness failures.
6408929bcin pipeline #52299:DGX_B200-4_GPUs-PyTorch-3: 8 passed, 4 skipped by pre-existing NVBUG waivers; pytest exit code 0.DGX_B200-8_GPUs-PyTorch-1: 12 passed, 2 skipped by pre-existing NVBUG waivers; pytest exit code 0.Scope and follow-up
This PR intentionally does not:
Generic overlap lifecycle and headroom are follow-up scope in #17278. The broader Python-transceiver and workload-liveness issues remain tracked in #16918 and #16920.