Fix GLM DCP global top-k MTP - #31
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ChangesB12X DCP top-k warmup and routing
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
vllm/v1/attention/backends/mla/b12x_mla_sparse.py (1)
683-706: 💤 Low valueNear-duplicate of
_sync_dcp_warmupinsparse_attn_indexer.py.This method is very similar to the module-level
_sync_dcp_warmup()insparse_attn_indexer.py, with minor differences:
- This passes
device_ids=[torch.cuda.current_device()]to barrier- This checks
self.device.type == "cuda"vscurrent_platform.is_cuda()Consider extracting a shared helper to avoid drift between the two implementations, or document why the differences are intentional.
🤖 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 `@vllm/v1/attention/backends/mla/b12x_mla_sparse.py` around lines 683 - 706, The _sync_dcp_warmup method in this file contains duplicate logic with a similar method in sparse_attn_indexer.py, with minor differences in how device checking is done and parameters passed to the barrier call. Extract a shared helper function in a common utility module that can be used by both implementations to maintain consistency and prevent future drift, passing any device-specific parameters as arguments to the helper. Alternatively, if the differences between the two implementations are intentional, add clear comments documenting why each implementation differs in its device checking approach and barrier parameter passing.vllm/v1/attention/backends/mla/indexer.py (1)
960-966: Extract VLLM_DCP_GLOBAL_TOPK parsing to a shared utility to prevent duplication.The
VLLM_DCP_GLOBAL_TOPKparsing at lines 963-965 duplicates the logic in_dcp_global_topk_requested()fromsparse_attn_indexer.py. A circular import exists (sparse_attn_indexer.py imports from this file), making a direct import infeasible. Extract this logic to a shared utility function inenvs.pyor a common module to avoid drift if either is updated independently.🤖 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 `@vllm/v1/attention/backends/mla/indexer.py` around lines 960 - 966, The parsing logic for the VLLM_DCP_GLOBAL_TOPK environment variable is duplicated between the keep_empty_dcp_chunk assignment in this file and the _dcp_global_topk_requested() function in sparse_attn_indexer.py. Create a new shared utility function in envs.py that encapsulates the VLLM_DCP_GLOBAL_TOPK parsing logic (checking if the retrieved value is in the set of "1", "true", "yes", "on" after lowercasing), then replace the inline os.environ.get("VLLM_DCP_GLOBAL_TOPK", "1").lower() condition in the keep_empty_dcp_chunk assignment with a call to this new utility function, and update _dcp_global_topk_requested() to use the same utility to prevent drift between implementations.
🤖 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.
Inline comments:
In `@vllm/model_executor/layers/sparse_attn_indexer.py`:
- Around line 443-446: The code directly calls dist.barrier(group=device_group)
which violates the documented pattern against using device_group for barriers
since NCCL internally creates GPU tensors that can interfere with GPU state.
Replace the dist.barrier(group=device_group) call with dcp_group.barrier()
instead, which internally uses cpu_group as the established convention dictates.
Remove the direct device_group barrier invocation and use the dcp_group barrier
method to maintain consistency with the GroupCoordinator pattern.
---
Nitpick comments:
In `@vllm/v1/attention/backends/mla/b12x_mla_sparse.py`:
- Around line 683-706: The _sync_dcp_warmup method in this file contains
duplicate logic with a similar method in sparse_attn_indexer.py, with minor
differences in how device checking is done and parameters passed to the barrier
call. Extract a shared helper function in a common utility module that can be
used by both implementations to maintain consistency and prevent future drift,
passing any device-specific parameters as arguments to the helper.
Alternatively, if the differences between the two implementations are
intentional, add clear comments documenting why each implementation differs in
its device checking approach and barrier parameter passing.
In `@vllm/v1/attention/backends/mla/indexer.py`:
- Around line 960-966: The parsing logic for the VLLM_DCP_GLOBAL_TOPK
environment variable is duplicated between the keep_empty_dcp_chunk assignment
in this file and the _dcp_global_topk_requested() function in
sparse_attn_indexer.py. Create a new shared utility function in envs.py that
encapsulates the VLLM_DCP_GLOBAL_TOPK parsing logic (checking if the retrieved
value is in the set of "1", "true", "yes", "on" after lowercasing), then replace
the inline os.environ.get("VLLM_DCP_GLOBAL_TOPK", "1").lower() condition in the
keep_empty_dcp_chunk assignment with a call to this new utility function, and
update _dcp_global_topk_requested() to use the same utility to prevent drift
between implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6b16eb0a-c3ee-4e28-ac6c-870d4f48c478
📥 Commits
Reviewing files that changed from the base of the PR and between 4e4a0b9 and df8ad3b202c84937a23cfa9d93f7a3677da8ecde.
📒 Files selected for processing (3)
vllm/model_executor/layers/sparse_attn_indexer.pyvllm/v1/attention/backends/mla/b12x_mla_sparse.pyvllm/v1/attention/backends/mla/indexer.py
df8ad3b to
000807e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@vllm/v1/attention/backends/mla/b12x_mla_sparse.py`:
- Around line 698-701: The barrier call at lines 698-701 is using dist.barrier()
with device_group parameter, which violates the established pattern and causes
NCCL internal GPU tensor allocation issues. Replace the
dist.barrier(group=device_group, device_ids=[torch.cuda.current_device()]) call
with dcp_group.barrier() which properly routes the barrier operation through the
CPU group instead, avoiding the problematic NCCL internal GPU tensor creation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 60b946da-06ad-4c19-99af-1619884fa738
📥 Commits
Reviewing files that changed from the base of the PR and between df8ad3b202c84937a23cfa9d93f7a3677da8ecde and 000807e.
📒 Files selected for processing (4)
vllm/model_executor/layers/sparse_attn_indexer.pyvllm/model_executor/models/deepseek_mtp.pyvllm/v1/attention/backends/mla/b12x_mla_sparse.pyvllm/v1/attention/backends/mla/indexer.py
🚧 Files skipped from review as they are similar to previous changes (1)
- vllm/v1/attention/backends/mla/indexer.py
Keep the B12X DCP global top-k path rank-consistent through graph warmup and MTP draft execution. The target GLM path already allocated score buffers for B12X DCP top-k, but the MTP draft layer only allocated indices. With B12X sparse indexer global-topk enabled, the draft capture either failed or fell back to non-comparable slow paths. Allocate and pass top-k score buffers for MTP when DCP and B12X sparse indexer are active. Also keep empty DCP prefill chunks participating in the global-topk merge and prewarm the actual B12X merge/extend paths so rank-sensitive graph warmup enters the same collectives on every rank.
000807e to
79f154c
Compare
Summary
This replaces the previous PR contents with one clean commit for the validated fast GLM-5.2 TP8/DCP4/MTP3 path on current
dev/dark-devotion.The fix keeps the B12X DCP global-topk path rank-consistent during graph warmup and makes the MTP draft path provide the same top-k score buffer that the target GLM path already provides.
Changes included:
-infscores and mask results back to-1.plan.bind(scratch=...), matching the vLLM eager binding rule.topk_scores_bufferfrom the MTP draft layer when DCP + B12X sparse indexer is active. Without this, the latest B12X global-topk stack failed or ran in non-comparable slow paths for MTP.No B12X source patch is included in this PR. The validated run uses B12X
5af873a7b6c81fbf533ef96bede13fbf4744ad2aunchanged.Validation
Clean image built through the standard
blackwell-llm-dockerpipeline:voipmonitor/vllm:dark-devotion-df8ad3b-b12x5af873a-mtptopkscores-cu132-20260621Build inputs:
000807e2b0e33277ac6b3ae51ae2e52d8472c9abdev/dark-devotionat4e4a0b91a73d474374e8e5da528a24bb6a16b0eb5af873a7b6c81fbf533ef96bede13fbf4744ad2a9c5ed7c194e7412780862491742fc655daaad6ac2.12.0+cu1322.30.413.4.1.2Validated runtime matching the fast reference:
--dcp-comm-backend ag_rs--attention-backend B12X_MLA_SPARSE--moe-backend b12xB12X_MOE_FORCE_A16=1VLLM_USE_B12X_SPARSE_INDEXER=1VLLM_DCP_GLOBAL_TOPK=1VLLM_DCP_SHARD_DRAFT=1--max-model-len 256000--max-num-seqs 4--max-num-batched-tokens 8192--max-cudagraph-capture-size 24Measured with
/mnt/test.py --port 5543 -L:128.18 tok/s127.91 tok/s126.17 tok/s130.73 tok/s0Reference PR30 image on the same host measured
/mnt/test.py --port 5544 -L:119.96 tok/s120.27 tok/s128.47 tok/s0Engine logs for the fixed latest stack showed MTP acceptance around
0.95 / 0.83 / 0.64-0.70, average draft acceptance about78-82%.Notes
The B12X vLLM bindings remain eager and caller-scratch-owned. This PR does not introduce workspace/arena ownership or cached workspace binding in the vLLM path.
Summary by CodeRabbit
Performance Improvements
Bug Fixes