[Core] Preallocate sampler logits workspace during memory profiling#37518
[Core] Preallocate sampler logits workspace during memory profiling#37518glaziermag wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
The pull request correctly identifies and addresses a critical Out-of-Memory (OOM) issue in the sampler by introducing a pre-allocated workspace and using copy_() for in-place casting. This is a good architectural fix for the identified problem. However, a critical sizing mismatch exists in the allocation of the sampler_workspace, which could lead to an index out-of-bounds error during runtime. The workspace is currently sized based on the maximum number of sequences (max_num_seqs), but it needs to accommodate the maximum number of batched tokens (max_num_batched_tokens) to prevent errors during prefill or large decode steps.
| logits_fp32 = self.sampler_workspace[: logits.size(0)] | ||
| logits_fp32.copy_(logits) | ||
| logits = logits_fp32 |
There was a problem hiding this comment.
The logits.size(0) represents the total number of tokens in the current batch (total_num_scheduled_tokens), which can be as large as self.scheduler_config.max_num_batched_tokens. However, self.sampler_workspace is allocated in gpu_model_runner.py with a size based on max_num_seqs (maximum number of sequences).
This mismatch means that if logits.size(0) exceeds max_num_seqs (which is common during prefill or large decode steps), accessing self.sampler_workspace[: logits.size(0)] will result in an IndexError due to out-of-bounds access. The sampler_workspace must be large enough to hold the maximum possible number of tokens in a batch.
| max_batch_size = self.scheduler_config.max_num_seqs | ||
| self.sampler.sampler_workspace = torch.empty( |
There was a problem hiding this comment.
The sampler_workspace is allocated using max_batch_size = self.scheduler_config.max_num_seqs. As noted in the sampler.py review, the sampler's forward method will attempt to slice this workspace using logits.size(0), which can be up to self.scheduler_config.max_num_batched_tokens.
This allocation should use self.scheduler_config.max_num_batched_tokens (or self.max_num_tokens) instead of max_num_seqs to ensure the workspace is sufficiently large for all possible batch sizes, preventing an IndexError during runtime.
| max_batch_size = self.scheduler_config.max_num_seqs | |
| self.sampler.sampler_workspace = torch.empty( | |
| max_batch_size = self.scheduler_config.max_num_batched_tokens |
88c1cc9 to
4f00d37
Compare
|
@gemini-code-assist Thanks for the review. We actually don't need to expand this workspace to The assertion that we need The only logits that reach I have pushed a revision that reverts the memory bounds back to track the actual required target sequence limits. This keeps the |
|
Thank you for the detailed explanation and for clarifying the memory allocation requirements for |
ac225be to
7e2047b
Compare
|
This pull request has merge conflicts that must be resolved before it can be |
|
Independent validation addendum (2026-05-05) I reran the memory behavior validation on GCP against the exact base and PR head commits:
L4 results:
A100 results:
Classification: UNSAFE_CLAIM as originally worded, with underlying evidence TARGETED. I amended the PR title/body to narrow the claim. I did not find a complete fix for a 24 GB |
|
Follow-up pushed: What changed:
Validation of the follow-up:
Classification remains TARGETED, not ACTUAL. The original L4 24 GB |
ab70bb5 to
1d3dec3
Compare
1d3dec3 to
6360e3b
Compare
Summary
Classification:
PARTIAL_MITIGATION / TARGETED sampler workspace accounting improvement.This PR preallocates a float32 sampler logits workspace during GPU memory profiling and reuses that workspace in
Sampler.forward()when it is compatible with the current logits tensor. If the workspace is absent, too small, on the wrong device, or the logits are already float32, the sampler uses the normal safe path instead of blindly slicing the workspace.The intended benefit is narrow: account for the sampler fp16-to-fp32 logits workspace when vLLM sizes the KV cache, reducing an unprofiled allocation spike during sampler warmup/serving.
Change
sampler_workspaceas a static float32 tensor sized to maximum live requests and vocabulary size, only on the last pipeline rank of generation runners, during model memory accounting.Sampler.forward(), reuse that workspace only when dtype, device, row capacity, and vocab dimension match the current logits.logits.to(torch.float32)when the preallocated workspace cannot safely satisfy the current call.Validation Scope
Independent GCP validation was run on 2026-05-05 against the original PR head before the final workspace-hardening follow-up:
be12afd284f3f09991a7fcf506553375dc58be364d580373668c3441c16dde174c1af603d0bb3c40ab70bb5249ef4f93f6958b23aba9641591ca43a6Qwen/Qwen2.5-0.5Bg2-standard-32, NVIDIA L4, driver 580.126.20, CUDA 13.0a2-highgpu-1g, CUDA 13.0Results on L4 24 GB:
gpu_memory_utilization=0.99: failed during CUDA graph capture with CUDA OOM.gpu_memory_utilization=0.99: still failed before serving with CUDA OOM while trying to allocate 2 MiB.gpu_memory_utilization=0.95: booted and completed repeated greedy generation probes.0.95with prompt logprobs plus chunked prefill: booted and probe passed.0.95withmax_num_seqs=2andmax_num_batched_tokens=128: booted and boundary probe passed.0.95with ngram speculative decoding: booted and probe passed.Results on A100:
gpu_memory_utilization=0.99both failed early because free startup memory was below the requested 0.99 reservation.0.95passed generation, prompt-logprobs/chunked-prefill, max-seqs/batched-token boundary, and ngram speculative probes.Current rebase and validation (2026-07-12)
Rebased onto upstream
mainat27c3e579f0e5f345a86e512e26e1231d3689931f.The current head corrects the prior over-reservation and reuse race:
Sampler.forward()receives one next/bonus logits row per live request; speculative target rows are handled separately byRejectionSampler, so the persistent workspace ismax_num_reqs × vocab_size, notmax_num_reqs × (1 + num_spec_tokens) × vocab_size.Current focused validation:
pytest -q tests/v1/sample/test_sampler.py -k "workspace or full_processed" --confcutdir=tests/v1/sampleon CPU ->4 passed, 36 deselectedpytest -q tests/v1/worker/test_gpu_model_runner.py -k "sampler_workspace_allocation" --confcutdir=tests/v1/workeron CPU ->4 passed, 34 deselectedruff format --check,ruff check,python -m py_compile, andgit diff --checkon all four changed files -> passedThe earlier L4/A100 runtime results remain historical validation of the targeted accounting approach. This follow-up was not rerun on GPU and does not strengthen the original OOM claim.
What This Does Not Prove
This PR should not claim to make
gpu_memory_utilization=0.99safe on 24 GB GPUs. The L4 head run still OOMed at0.99.This PR also does not prove a fix for the closest sampler OOM issue I found, #33920. That issue uses GLM-4.7-Flash, tensor parallel size 2, 2x RTX 4090,
max_model_len=90000, and an OOM in the top-k/top-p sort path. That original issue was not reproduced by this validation.Safe Claim
Preallocates/account-profiles the sampler fp32 logits workspace to avoid one unprofiled allocation spike; full 24 GB/
0.99OOM behavior remains unresolved. The current head safely falls back when the workspace is not compatible. On L4, this validated boot/generation at0.95and prompt-logprobs, chunked-prefill, max-seqs/batched-token, and ngram speculative probes. It does not make0.99utilization safe on 24 GB GPUs and does not prove the GLM-4.7-Flash/top-k-sort issue is fixed.Review Recommendation
Review this as a targeted sampler workspace accounting improvement. If the goal is to fix a 24 GB
0.99boot OOM or #33920 specifically, that needs a separate original-condition reproduction and broader memory-sizing fix.