Repository navigation
Conversation
417e82c to
30a13db
Compare
|
The core fix looks right. I traced the chunk planner and the NVFP4 kernel and simulated both tests' chunk plans: starts are block-aligned on every reachable path, the shift lands on the right physical slots, and no column that is actually read ever gets clamped. The reference-vs-dequantized-cache test design is nice. A few things worth addressing: 1. The workspace hunk looks dead. 2. Would 3. 4. The DCP rejection fails late. The raise at Minor:
Review assisted by Claude Code. |
…ernel
`--kv-cache-dtype nvfp4_ds_mla` crashed in the MLA chunked-context prefill:
`_compute_prefill_context` has a branch for fp8_ds_mla and sends every other
dtype to gather_and_maybe_dequant_cache, which only knows the fp8 variants
("Unsupported fp8 kv cache data type: nvfp4_ds_mla"). Sparse MLA reaches this
path through its dense-MHA prefill whenever a prefill has cached context: the
speculative-decoding warm-up batch (so an engine with a DFlash draft never
finished starting), a chunked short prompt's continuation, prefix-cache hits.
Route nvfp4_ds_mla to cp_gather_and_upconvert_nvfp4_kv_cache. The kernel read
every request from its first token, so it gains an optional seq_starts like
its fp8 twin: the per-token read offset becomes
`out_token - workspace_starts[req] + seq_starts[req]`, and the dense dispatch
passes chunk.starts exactly as the fp8_ds_mla branch does. Other callers are
unchanged (the argument defaults to None).
The test writes random KV into an nvfp4_ds_mla cache, runs
_compute_prefill_context on it, and compares every chunk's Q/K/V bit for bit
against the same method on a BF16 cache holding the dequantized values through
the generic gather; the batch includes a chunk that continues one request and
starts another.
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Signed-off-by: Stu Cao <stu@primeintellect.ai>
The nvfp4_ds_mla chunked-context gather is not validated under DCP. Rejecting the combination in VllmConfig fails at startup instead of on the first request that carries cached context, which is when the per-layer prefill path would have noticed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Signed-off-by: Stu Cao <stu@primeintellect.ai>
fde6eb9 to
31c6021
Compare
Kernel-level check for the seq_starts argument, from #2: random cache bytes, three requests with non-block-aligned starts (3, 32, 0), and the gathered rows must match, bit for bit, the same slots gathered without starts. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Signed-off-by: Lucas Wilkinson <lwilkins@redhat.com> Signed-off-by: Stu Cao <stu@primeintellect.ai>
|
@zyongye Thank you for the review. I have addressed your comments in the new revision.
|
|
@LucasWilkinson Hi Lucas, thank you so much for the review and suggestion PR! It make complete sense to me. I incorporate the change from your PR in this PR. The Here's other changes I made based on Yongye's comment. Could you please take another look?
|
Status (2026-10-15)
Reworked after @zyongye's review (thanks — every point was right) and @LucasWilkinson's stu-cao#2, which proposed the same
seq_startschange; rebased ontomain19b0efce30. Lucas's kernel-level test (random cache bytes, non-block-aligned starts, bit-exact against the same slots gathered without starts) is included as his commit intests/kernels/attention/test_cache.py. Where this differs from #2: the BF16 workspace hunk is dropped (zyongye's point 1, it was unreachable) and DCP is rejected at startup instead of being left to fail in the generic gather (point 4).seq_startson the NVFP4 gather kernel instead of a block-table shim.cp_gather_and_upconvert_nvfp4_kv_cachegains an optionalseq_startslike its fp8 twin (token_offset = out_token - workspace_starts[req] + seq_starts[req]), and the dense dispatch passeschunk.startsexactly as thefp8_ds_mlabranch does.shift_block_table_to_starts, its alignment precondition, the clamp, the per-chunk gather and its CPU test are gone. Other callers are unchanged (the argument defaults toNone).MLACommonMetadataBuilderworkspace hunk and thefp8_querytest arm.nvfp4_ds_mlais only accepted byFlashMLASparseBackend, whose builder (SparseMLACommonMetadataBuilder) allocates its own BF16 workspace and hard-codes a BF16 prefill query, so neither was reachable.VllmConfig.validate_nvfp4_kv_cache_with_mla) instead of in the per-layer forward on the first request with context; covered bytests/test_config.py::test_nvfp4_ds_mla_rejects_decode_context_parallelism.kv_c_and_k_pe_cachepass is intentional: thenvfp4_ds_mlacache is allocated astorch.uint8. The three other gather dispatch sites that still sendnvfp4_ds_mlato the generic gather (mla_attention.pyDCP path,kimi_k3/nvidia/mla.py,sparse_mla_attention.py) are unreachable today and left for a follow-up.Verification on the rebased head (
dc6aa4ae53, B300 SM103, from-source build): 17 passed, 0 failed —test_mla_prefill_context_nvfp4.py(1, theseq_startspath on real NVFP4 rows),test_mla_context_chunks.py(14),test_config.py::test_nvfp4_ds_mla_rejects_decode_context_parallelism(2); registered schemacp_gather_and_upconvert_nvfp4_kv_cache(..., int batch_size, Tensor? seq_starts=None); torch 2.14.0+cu130,TORCH_CUDA_ARCH_LIST=10.3a.tests/kernels/attention/test_cache.py -k nvfp4: 27 passed includingtest_cp_gather_and_upconvert_nvfp4_kv_cache_with_seq_starts.Purpose
--kv-cache-dtype nvfp4_ds_mlacrashes in the MLA chunked-context prefill:MLACommonBaseImpl._compute_prefill_contextgathers each chunk's cached context KV into a BF16 workspace. It has a branch forfp8_ds_mla; every other dtype goes togather_and_maybe_dequant_cache, which only knows the fp8 variants. The NVFP4 upconvert kernel added with thenvfp4_ds_mlaformat,cp_gather_and_upconvert_nvfp4_kv_cache, is never called here.Sparse MLA reaches this path through its dense-MHA prefill: a prefill whose total length is
<= index_topkand that has cached context. In practice:nvfp4_ds_mlaKV and a DFlash draft never finishes starting;Longer prefills use the sparse FlashMLA kernels, which already handle
nvfp4_ds_mla.Fix:
nvfp4_ds_mlatocp_gather_and_upconvert_nvfp4_kv_cache.seq_starts(csrc + bindings +_custom_ops), mirroringcp_gather_and_upconvert_fp8_kv_cache: a continuation chunk reads from its context start instead of the request's first token. Without it, every chunk after the first would silently re-read the request's first tokens.nvfp4_ds_mlawith decode context parallelism at config validation; that combination is not validated.Other dtypes are unaffected. I found no existing issue or PR for this.
Test Plan
New tests:
tests/v1/attention/test_mla_prefill_context_nvfp4.py(SM100). Writes random KV into annvfp4_ds_mlacache withconcat_and_cache_mla, then runs_compute_prefill_contexton it. The reference is the same method on a BF16 cache holding the dequantized values, through the generic gather. Every chunk must see bit-identical Q/K/V; the batch includes a chunk that continues one request and starts another.tests/kernels/attention/test_cache.py::test_cp_gather_and_upconvert_nvfp4_kv_cache_with_seq_starts(SM100): the kernel'sseq_startswith non-block-aligned starts, bit-exact against a full gather.tests/test_config.py::test_nvfp4_ds_mla_rejects_decode_context_parallelism(CPU).pytest tests/v1/attention/test_mla_prefill_context_nvfp4.py tests/v1/attention/test_mla_context_chunks.py tests/kernels/attention/test_cache.py tests/test_config.py -k "nvfp4 or context_chunks"End-to-end on 1 × GB200 node (4 GPUs), run with the first revision of this change (block-table shift; same gather semantics):
FLASHMLA_SPARSE,--kv-cache-dtype nvfp4_ds_mla,--block-size 1024,--max-model-len 1048576, prefix caching on.--max-model-len 262144, since fp8 cannot fit one 1M-token request on TP4.--max-num-seqs 8and an 8192-token budget so some are necessarily split and their continuation runs this path, compared with the same prompts served unsplit; draft acceptance over 3 × 400 tokens on a short and a ~10K-token prompt.Test Result
Unit tests: see Status above for the current head. The first revision, on GB200 against
v0.30.1rc0: the SM100 test failed before the change (Unsupported fp8 kv cache data type: nvfp4_ds_mla) and passed with it.End-to-end (first revision):
AI assistance (Claude) was used to investigate, write the change and tests, and run the tests above. I reviewed every changed line.
🤖 Generated with Claude Code