Repository navigation
Conversation
68dd0c8 to
6d0e061
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between 6cbb3c1 and 6d0e0616838f992ed1bd2ec8f5500cb443040ec3. 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 SummarySummary by CodeRabbit
WalkthroughThe Triton dequantization kernel now uses the block table’s runtime row stride. A parameterized test validates bit-exact output across request counts and non-contiguous block-table layouts. ChangesK Cache Dequantization
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to K-cache gathering now correctly addresses strided block-table views while preserving contiguous-table behavior. Regression coverage validates the corrected path, with no remaining merge-blocking risk. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
|
This pull request has merge conflicts that must be resolved before it can be |
6d0e061 to
a7bec94
Compare
a7bec94 to
1100516
Compare
d9928c4 to
8962577
Compare
`_dequantize_and_gather_k_kernel` accepts `block_table` tensor views but advances between rows using `block_table.shape[-1]`. A narrowed view of a fixed-width backing table retains the backing table's larger `stride(0)`, so requests after the first read incorrect physical block IDs and can access memory outside the K-cache allocation.
Two fixes were considered:
1. Materialize the view with `block_table.contiguous()` before launching the kernel.
2. Pass `block_table.stride(0)` to the kernel and use the physical row stride for address calculation.
We chose the stride argument because the kernel already accepts tensor views and already uses physical strides for `out` and `k_cache`. Calling `contiguous()` would repair the addressing indirectly by allocating temporary storage and copying the active block table on every invocation of this inference-path operation. Passing one scalar stride fixes the incorrect assumption at its source without adding GPU work, memory traffic, or allocator pressure.
For contiguous block tables, `stride(0) == shape[-1]`, so the generated addresses remain unchanged. For narrowed views, the kernel now advances by the backing table's actual row layout.
The production failure occurred on four 8xA100 Slurm nodes with TP16, PP2, EP16, block size 256, and compiled CUDA graphs:
```bash
#!/bin/bash
#SBATCH --nodes=4
#SBATCH --ntasks=4
#SBATCH --ntasks-per-node=1
#SBATCH --gpus-per-node=8
#SBATCH --time=02:00:00
set -euo pipefail
export MODEL=nvidia/DeepSeek-V4-Pro-0813-NVFP4
export MASTER_ADDR
MASTER_ADDR=$(scontrol show hostnames "$SLURM_JOB_NODELIST" | head -n1)
export MASTER_PORT=29501
export COMPILATION_CONFIG='{"cudagraph_capture_sizes":[1,2,4,8,16,32,64],"inductor_compile_config":{"enable_auto_functionalized_v2":true}}'
srun --nodes=4 --ntasks=4 --ntasks-per-node=1 bash -lc '
headless=()
if (( SLURM_NODEID > 0 )); then
headless=(--headless)
fi
exec vllm serve "$MODEL" \
--revision 2ff4ec53ee664e54571f670276d2c89d0fcc7b82 \
--served-model-name deepseek-v4-pro \
--host 0.0.0.0 \
--port 8000 \
--trust-remote-code \
--tokenizer-mode deepseek_v4 \
--load-format fastsafetensors \
--safetensors-load-strategy lazy \
--distributed-executor-backend mp \
--master-addr "$MASTER_ADDR" \
--master-port "$MASTER_PORT" \
--nnodes "$SLURM_NNODES" \
--node-rank "$SLURM_NODEID" \
--tensor-parallel-size 16 \
--pipeline-parallel-size 2 \
--enable-expert-parallel \
--kv-cache-dtype fp8 \
--block-size 256 \
--max-model-len 10240 \
--max-num-seqs 64 \
--max-num-batched-tokens 16384 \
--gpu-memory-utilization 0.9 \
--linear-backend auto \
--moe-backend auto \
--no-enable-prefix-caching \
--distributed-timeout-seconds 7200 \
--cpu-distributed-timeout-seconds 7200 \
--compilation-config "$COMPILATION_CONFIG" \
"${headless[@]}"
'
```
The first 64 concurrent requests compiled `_dequantize_and_gather_k_kernel` and then failed with:
```text
Triton kernel JIT compilation during inference: _dequantize_and_gather_k_kernel
CUDA error: an illegal memory access was encountered
```
Add `test_dequantize_and_gather_k_cache_triton` alongside the existing `test_dequantize_and_gather_k_cache`. The new test mirrors the existing test's cache layout and PyTorch-reference comparison but calls the Triton implementation directly with a narrowed block-table view at request counts 1, 2, 4, 8, 16, 32, 64, 128, and 256.
Direct coverage is necessary because the platform dispatcher can select another implementation and allow the dispatcher test to pass without executing the faulty Triton kernel. This is not testing a different caller interface: `dequantize_and_gather_k_cache` and `dequantize_and_gather_k_cache_triton` remain plug-compatible. The added stride is private to the Triton wrapper-to-kernel launch.
Before the fix:
```text
sm80: c1 passed; c2-c256 failed
sm86: c1 passed; c2-c256 failed
sm89: c1 passed; c2-c256 failed
A100 Compute Sanitizer: 27,983 errors
```
After the fix:
```text
sm80: c1-c256 passed
sm86: c1-c256 passed
sm89: c1-c256 passed
A100 Compute Sanitizer: 0 errors
```
Signed-off-by: Michael Gschwind <mgschwind@nvidia.com>
8962577 to
ad21c00
Compare
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Michael Gschwind <mgschwind@nvidia.com>
Co-authored-by: Codex <noreply@openai.com> Signed-off-by: Michael Gschwind <mgschwind@nvidia.com>
Purpose
_dequantize_and_gather_k_kernelacceptsblock_tabletensor views but advances between rows usingblock_table.shape[-1]. A narrowed view of a fixed-width backing table retains the backing table's largerstride(0), so requests after the first read incorrect physical block IDs and can access memory outside the K-cache allocation.Two fixes were considered:
block_table.contiguous()before launching the kernel.block_table.stride(0)to the kernel and use the physical row stride for address calculation.We chose the stride argument because the kernel already accepts tensor views and already uses physical strides for
outandk_cache. Callingcontiguous()would repair the addressing indirectly by allocating temporary storage and copying the active block table on every invocation of this inference-path operation. Passing one scalar stride fixes the incorrect assumption at its source without adding GPU work, memory traffic, or allocator pressure.For contiguous block tables,
stride(0) == shape[-1], so the generated addresses remain unchanged. For narrowed views, the kernel now advances by the backing table's actual row layout.Test Plan
Run the existing platform-dispatch test and the new direct Triton test:
Run the direct Triton test under Compute Sanitizer:
Reproduce the production configuration on four 8xA100 Slurm nodes with TP16, PP2, EP16, block size 256, and compiled CUDA graphs:
The existing
test_dequantize_and_gather_k_cachecontinues to test the platform dispatcher. The newtest_dequantize_and_gather_k_cache_tritonmirrors its cache layout and PyTorch-reference comparison but calls the Triton implementation directly with a narrowed block-table view at request counts 1, 2, 4, 8, 16, 32, 64, 128, and 256.Direct coverage is necessary because the platform dispatcher can select another implementation and allow the dispatcher test to pass without executing the faulty Triton kernel. Both Python functions retain the same caller interface; the added stride is private to the Triton wrapper-to-kernel launch.
Test Result
The production run reached the first 64 concurrent requests, compiled
_dequantize_and_gather_k_kernel, and then failed with:Before the fix, the direct test passed at request count 1 and failed at every request count from 2 through 256 on
sm80,sm86, andsm89. Compute Sanitizer on an A100 reported 27,983 invalid memory accesses.After the fix, the direct test passed at request counts 1, 2, 4, 8, 16, 32, 64, 128, and 256 on
sm80,sm86, andsm89. Compute Sanitizer on an A100 reported:Essential Elements of an Effective PR Description Checklist