[GG] fix(concurrency): isolate graph state and gate B12X MoE overlap - #132
Conversation
|
Warning Review limit reached
Next review available in: 18 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughChangesB12X PCIe oneshot sizing
B12X DCP A2A graph capture
Independent workspace lanes
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Speculator
participant use_workspace_lane
participant WorkspaceManager
participant Worker
Speculator->>use_workspace_lane: select workspace lane 1
use_workspace_lane->>WorkspaceManager: scope allocation
WorkspaceManager-->>Speculator: return ubatch/lane workspace
Worker->>Speculator: run setup, warmup, capture, or sampling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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.
🧹 Nitpick comments (1)
vllm/v1/attention/ops/dcp_alltoall.py (1)
154-174: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd docstring arguments and improve type hint.
The type hint for
streamis overly generic (object); usingtorch.cuda.Streamis more precise. Also, the docstring is missing the argument descriptions. As per coding guidelines, use Google-style docstrings in Python code, withArgs:/Returns:/Raises:sections.💡 Proposed fix
`@contextmanager` def capture_b12x_dcp_a2a( cp_group: GroupCoordinator, - stream: object = None, + stream: torch.cuda.Stream | None = None, ): - """Bind each CUDA graph manager to independent B12X DCP channels.""" + """Bind each CUDA graph manager to independent B12X DCP channels. + + Args: + cp_group: The control-plane group coordinator to match. + stream: The CUDA stream to use for capturing. + """ group_id = id(cp_group.device_group) matching_pools = sorted(🤖 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/ops/dcp_alltoall.py` around lines 154 - 174, Update capture_b12x_dcp_a2a to type stream as torch.cuda.Stream, preserving its optional default, and expand the docstring with a Google-style Args section describing cp_group and stream. Add Returns or Raises sections only if applicable to the existing context-manager behavior.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 `@vllm/v1/attention/ops/dcp_alltoall.py`:
- Around line 154-174: Update capture_b12x_dcp_a2a to type stream as
torch.cuda.Stream, preserving its optional default, and expand the docstring
with a Google-style Args section describing cp_group and stream. Add Returns or
Raises sections only if applicable to the existing context-manager behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e710e54-84df-4178-be8f-c95bd609dafb
📒 Files selected for processing (6)
tests/distributed/test_b12x_fused_all_reduce.pytests/distributed/test_dcp_a2a.pyvllm/distributed/device_communicators/custom_all_reduce.pyvllm/distributed/parallel_state.pyvllm/envs.pyvllm/v1/attention/ops/dcp_alltoall.py
|
@voipmonitor I opened dependent follow-up #134 for the hybrid-specific gap in the shared-expert overlap gate. |
|
Folded the hybrid resident-grid protection from #134 into this stack (99695d4), added its parametrized regression coverage, and addressed the capture-stream type/docstring review note (3214348). Combined focused validation: 71 relevant tests passed, plus the TP4 backend-config gate passed with four visible GPUs. |
2217d94
into
local-inference-lab:dev/gilded-gnosis
Summary
Root causes
Target and MTP graph managers could alias B12X oneshot/DCP channel state and process-global vLLM workspace storage. These resources now follow graph ownership and target/speculator execution use independent workspace lanes.
A separate MoE scheduling hazard existed when shared-expert GEMM work was submitted around a routed resident-grid kernel. Resident-grid kernels require all participating CTAs to become resident. The scheduler now asks the concrete B12X launch plan whether auxiliary-stream overlap is safe, serializes unsafe plans, and submits safe shared-expert work before routed MoE.
The deterministic Xid 31 after a 250k prefill was a distinct disposable graph-pool cleanup bug and is fixed in #131. It should not be attributed to tight DCP IPC output allocation: B12X returns normal PyTorch output tensors and uses IPC only for internal staging/signals.
Validation
git diff --checkpassedDependencies
Requires local-inference-lab/sparkinfer#47. For the full v19 stack, merge #131 as well.