[GG] fix(speculation): isolate graph resources and profile MRV2 memory#149
Conversation
|
Warning Review limit reached
Next review available in: 42 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 (18)
📝 WalkthroughWalkthroughAdds B12X PCIe/DCP channel checkpointing, complete disposable CUDA graph memory profiling with cleanup and pool restoration, and sparse MLA DCP transient-memory profiling controlled by a new environment flag. ChangesB12X profiling and graph lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GPUModelRunner
participant CudaGraphManagers
participant parallel_state
participant MLAAttention
GPUModelRunner->>CudaGraphManagers: assign profiling pools
GPUModelRunner->>parallel_state: checkpoint B12X channels
GPUModelRunner->>MLAAttention: profile sparse transient memory
GPUModelRunner->>CudaGraphManagers: capture and clear disposable graphs
GPUModelRunner->>parallel_state: rollback B12X channels
GPUModelRunner->>CudaGraphManagers: restore production pools
GPUModelRunner->>GPUModelRunner: measure retained capture memory
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 |
a6cb402 to
91788aa
Compare
|
Split completed. This PR head was reduced from the former aggregate commit Extracted direct-to-GG PRs:
SparkInfer companion split:
Nothing has been silently discarded. The two SparkInfer replacement branches reconstruct #58 exactly. On the vLLM side, #151 and #152 merge first; this PR is then refreshed against GG to apply workspace lane 1 to the profiling-only speculator capture and resolve the overlapping DCP test/module hunks. After that mechanical refresh, the intended functional difference from the former aggregate #149 is only the profiler-guided MoE ordering correction documented in #150. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
tests/v1/cudagraph/test_breakable_cudagraph.py (1)
42-135: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only covers pre-existing wrappers, not ones created mid-capture.
FakeWrapperis instantiated beforeprofile_cudagraph_memory()runs, so this test only exercises wrappers that are already in_all_instancesprior to capture. It doesn't cover a wrapper being lazily created duringcapture()(e.g. aBreakableCUDAGraphWrappercreated byinit_breakable_cg_runner), which is the exact gap flagged inmodel_runner.py'sprofile_cudagraph_memoryreview comment — such a wrapper'sgraph_poolis never redirected to the disposableprofiling_pool.🤖 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 `@tests/v1/cudagraph/test_breakable_cudagraph.py` around lines 42 - 135, Extend test_memory_profile_destroys_graphs_before_restoring_pools to create a new wrapper during FakeManager.capture, register it in the relevant wrapper instance collection, and assert its graph_pool uses profile_pool before cleanup and production_pool afterward. Preserve the existing assertions for pre-existing wrappers and event ordering while covering lazy wrapper creation during capture.
🤖 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/attention/mla_attention.py`:
- Around line 784-793: In the candidate construction logic, update the projected
AG_RS branch in the dcp_project_before_merge path to add (last_ag_rs_row, True)
whenever last_ag_rs_row exceeds project_threshold, regardless of
workspace_eligible. Keep the existing row bounds and unprojected candidate logic
unchanged.
In `@vllm/v1/worker/gpu/model_runner.py`:
- Around line 876-915: Update ModelCudaGraphManager.init_breakable_cg_runner so
newly created BreakableCUDAGraphWrapper instances inherit the manager’s
currently active pool when self.pool is set, rather than always retaining the
global graph pool. Preserve the existing wrapper creation behavior and allow
normal captures to continue using the global pool when no manager pool is
configured.
---
Nitpick comments:
In `@tests/v1/cudagraph/test_breakable_cudagraph.py`:
- Around line 42-135: Extend
test_memory_profile_destroys_graphs_before_restoring_pools to create a new
wrapper during FakeManager.capture, register it in the relevant wrapper instance
collection, and assert its graph_pool uses profile_pool before cleanup and
production_pool afterward. Preserve the existing assertions for pre-existing
wrappers and event ordering while covering lazy wrapper creation during capture.
🪄 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: 8e2ea35f-a3e5-49b3-881a-039e6e063db0
📒 Files selected for processing (14)
tests/distributed/test_b12x_fused_all_reduce.pytests/distributed/test_dcp_a2a.pytests/v1/attention/test_b12x_mla_dcp_workspace.pytests/v1/cudagraph/test_breakable_cudagraph.pyvllm/distributed/device_communicators/custom_all_reduce.pyvllm/distributed/parallel_state.pyvllm/envs.pyvllm/model_executor/layers/attention/mla_attention.pyvllm/v1/attention/ops/dcp_alltoall.pyvllm/v1/worker/gpu/cudagraph_utils.pyvllm/v1/worker/gpu/model_runner.pyvllm/v1/worker/gpu/spec_decode/autoregressive/speculator.pyvllm/v1/worker/gpu/spec_decode/dflash/speculator.pyvllm/v1/worker/gpu/spec_decode/speculator.py
91788aa to
6d530ab
Compare
Summary
Establish one ownership contract for target and draft CUDA-graph resources, then use it to make MRV2 memory profiling accurate and disposable.
This is intentionally one atomic PR: MRV2 profiling captures both target and speculator graphs. That capture is only safe when their workspace views and SparkInfer communication channels have independent owners. MoE scheduling is unrelated and remains in #150.
What changes
workspace.pygives each(ubatch, execution owner)an independent reusable buffer. Target uses lane 0; speculative execution uses lane 1.Root causes and lifecycle
Previously, graph-retained workspace views could alias, communication channels could outlive the graph that created them, and MRV2 reserved no production graph footprint. Sparse MLA transient buffers were also absent from the dummy profile.
A disposable profile now follows this order:
Tests assert lane separation and restoration, independent graph channels, cleanup ordering, retained-pool accounting, and sparse-DCP workspace accounting.
Scope and dependencies
dev/gilded-gnosis; no Docker-only overlay.Validation
Release CUDA image with four visible GPUs:
tests/v1/worker/test_workspace.pytests/distributed/test_b12x_fused_all_reduce.pytests/distributed/test_dcp_a2a.pytests/v1/attention/test_b12x_mla_dcp_workspace.pytests/v1/cudagraph/test_breakable_cudagraph.pygit diff --checkpassed