[https://nvbugs/6487039][fix] Generalize ADP dummy lifecycle - #16921
[https://nvbugs/6487039][fix] Generalize ADP dummy lifecycle#16921chienchunhung wants to merge 5 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 |
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. |
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
|
6408929 to
acaf894
Compare
|
/bot run --disable-fail-fast --disable-reuse-test --stage-list "DGX_B200-PyTorch-5, GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2" |
acaf894 to
d0d14ef
Compare
|
PR_Github #64728 [ run ] triggered by Bot. Commit: |
|
PR_Github #64729 [ run ] triggered by Bot. Commit: |
|
PR_Github/16921-d0d14ef #64728 was force-killed by a newer pipeline run. |
|
PR_Github #64729 [ run ] completed with state |
|
/bot run --disable-fail-fast |
|
PR_Github #64752 [ run ] triggered by Bot. Commit: |
|
PR_Github #64752 [ run ] completed with state
|
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>
Signed-off-by: Chien-Chun Hung <2679986+chienchunhung@users.noreply.github.com>
d0d14ef to
7c9c567
Compare
|
/bot run --disable-fail-fast |
|
PR_Github #65097 [ run ] triggered by Bot. Commit: |
Summary
Fixes NVBUG 6487039.
Attention-DP ranks must enter each collective forward with compatible batches, so a rank with no real work pads its batch with a synthetic request when a peer will run compute. This PR fixes the dummy lifecycle in the non-PP disaggregated path:
The scope is deliberate. Pipeline parallelism keeps its existing fallback. Generic overlap forward-intent is not generalized here; the validated model-specific scheduler-aware padding is preserved. Sequence-slot headroom follows the topology-based disaggregated attention-DP overlap policy already on
mainvia #17282.Why the failure surfaced
This was a latent lifecycle bug exposed by the high-concurrency Kimi disaggregated workload. The transactional ADP handling introduced by #16279 was scoped to DeepSeek-V4, so Kimi continued to use the legacy path. At concurrency 4096, a fleet-wide scheduling rejection can leave a tentative dummy alive long enough for repeated iterations to expose the broken active-request invariant.
Failure mechanism
The scheduler admits only requests within its configured state window and readiness rules. The legacy active-request count could still treat transfer, wait, or terminal requests outside that window as schedulable. A rank holding only those requests could omit a required dummy while peer ADP ranks allocated tentative dummies.
If the TP-wide queue decision rejected the iteration, 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 dummy requests. A sticky context/generation role also allowed transfer-only requests to retain a stale dummy shape while a cell was draining.Targeted validation then exposed a second ownership bug in the V1 cache path:
add_dummy_requestspre-registered a context dummy's sequence, and normal context resource preparation attempted to register the same sequence again, triggering the C++emplaceDoneassertion.Fix
ENCODER_INITboundary for encoder-decoder models.none/generation/context intent every iteration and combine it with one scalar TPMAXreduction. A transfer-only rank adds no dummy when no peer has real forward work; otherwise it pads with the peer's compute role.This PR makes no independent sequence-slot sizing change. On the rebased base, normal non-overlap configurations use
max_batch_size, PP usespp_size * max_batch_size, and non-PP disaggregated attention-DP overlap uses the2 * max_batch_sizeheadroom supplied by #17282.Verification
The exact NVBUG workload passed on the pre-rebase PR head
d0d14ef2417877b12a89de62b1fd89acf421a2fe. Current-head CI is pending for rebased head7c9c56733a5afab08fd5a2253c0a789800492a8d.GB300-20_GPUs-5_Nodes-PyTorch-Disagg-PerfSanity-CTX1-NODE1-GPU4-GEN1-NODE4-GPU16-Post-Merge-2.perf/test_perf_sanity.py::test_e2e[disagg_upload-e2e-gb300_kimi-k25-thinking-fp4_8k1k_con4096_ctx1_dep4_gen1_dep16_eplb0_mtp0_ccb-NIXL].2901721completed with exit code0.emplaceDoneassertion, executor restart error, HangDetector/MPI-abort chain, or KV-transfer/Python-transceiver timeout errors.7c9c5673: Python syntax compilation andgit diff --checkpassed; all formatting/lint pre-commit hooks passed; both test-list validators passed with Python 3.13 (2,063 unique entries). Local pytest remains unavailable because this host lackstorch.Full merge-gate pipeline #52601 on pre-rebase head
d0d14efcompleted with unrelated Gemma4 timeout, GB200 performance-threshold, Jenkins, and Slurm flakes; it did not expose an ADP dummy-lifecycle regression. A fresh full run was started for rebased head7c9c5673from the trigger comment: PR_Github #65097 launched L0_MergeRequest_PR #52898, whose live console checked out the exact rebased SHA.The exact Kimi test is already unwaived on main by #16920; this PR makes no waiver or timeout change.
Scope and related work
This PR intentionally does not:
#17278, already merged and included in this PR's base, makes overlap pad-dummy surplus fail-soft instead of crashing at the assertion. #17282, also included in the rebased base, supplies topology-based sequence-slot headroom for disaggregated attention-DP overlap. This PR preserves both protections while fixing the transactional leak and scheduler/ownership invariants within the scope above. Earlier Python-transceiver diagnostics are documented in #16918, and #16920 owns the test unwaiving.