feat(dcp, flashinfer): support FULL_DECODE_ONLY for flashinfer with DCP>1 - #36503
feat(dcp, flashinfer): support FULL_DECODE_ONLY for flashinfer with DCP>1#36503pisceskkk wants to merge 6 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for FULL_DECODE_ONLY in FlashInfer with DCP by allocating persistent buffers for DCP-related variables. The changes involve initializing these buffers in the FlashInferBackend and using them in the forward pass to avoid on-the-fly memory allocations. While the buffer management logic appears sound, I've identified a critical issue in vllm/v1/attention/backends/flashinfer.py where a variable is calculated and then immediately overwritten with a different, simpler logic. This is not only confusing and inefficient but also potentially a bug as the new calculation ignores DCP-specific parameters, which could lead to incorrect behavior. I've recommended refactoring this part to make the logic explicit and correct.
73b42dc to
aab57d7
Compare
I think #36070 (flashattn counterpart of this PR) is relevant as well for SM90. It seems to work for short inputs (like gsm8k), but for long inputs I still see infinite repetition / gibberish. |
Sorry for the late reply. I initially suspected there might be an issue with chunked prefill, but testing with chunked prefill didn’t show noticeable drop in accuracy. Could you provide a reproducible script and output samples? test results with There is indeed a slight precision drop, but it appears to be an edge case. Looking into it. |
I think there was a miscommunication - what I've mentioned before is for FA3 as my main computing pool is H100 and the default attention backend for H100 is FA3. I'd like to check the FI backend on H100 in any case, it would be great if DCP is back to functioning well with FI. Will come back with some benchmark results. |
|
fix some bugs and now the accuracy with 'fully' chunked prefill looks better: BTW, I noticed FlashInfer's LSE uses base-2, but |
LucasWilkinson
left a comment
There was a problem hiding this comment.
overall looks pretty good to me! thanks for the contribution! i have a couple questions
| if self.use_dcp: | ||
| assert seq_lens_cpu is not None | ||
| # Keep the shared metadata immutable for mixed decode/prefill batches. | ||
| seq_lens_cpu = seq_lens_cpu.clone() |
There was a problem hiding this comment.
why does it need to be cloned, i see it says it needs to be immutable but im confused where the mutation is coming from, async scheduling?
There was a problem hiding this comment.
Looks like that was leftover code from debugging the accuracy issue. I just confirmed that removing it doesn't change anything. The real cause for the accuracy issue was the LSE base value.
| Tasks | Version | Filter | n-shot | Metric | Value | Stderr | ||
|---|---|---|---|---|---|---|---|---|
| gsm8k | 3 | flexible-extract | 5 | exact_match | ↑ | 0.600 | ± | 0.0155 |
| strict-match | 5 | exact_match | ↑ | 0.546 | ± | 0.0158 |
There was a problem hiding this comment.
can we remove it then please?
There was a problem hiding this comment.
I remember deleting this line of code, but it might still be there due to a rebase error. deleted
| (decode_query.size(0), decode_query.size(1)), | ||
| dtype=torch.float32, | ||
| device=decode_query.device, | ||
| decode_query_buffer.copy_( |
There was a problem hiding this comment.
why is this copy necessary?
There was a problem hiding this comment.
These buffers need to be assigned in-place. Do you mean using the decode_query_buffer[:] = get_dcp_group(...) syntax?
There was a problem hiding this comment.
i guess i dont understand why we can't use the output of get_dcp_group().all_gather directly?
There was a problem hiding this comment.
Ah, yes, I had some misunderstandings about the persistence of tensors in CUDAGraph. This value indeed does not need to be persisted. It has been corrected.
There was a problem hiding this comment.
Pull request overview
Adds persistent buffer allocation to enable FULL_DECODE_ONLY when using FlashInfer with decode_context_parallel_size > 1, and adjusts LSE handling for downstream attention-state merging.
Changes:
- Allocate reusable decode buffers via the workspace manager for DCP all-gathered queries / outputs / LSE.
- Convert FlashInfer LSE to the expected log base for
merge_attn_states. - Update DCP/CP interleave sizing and adjust how CPU seq-lens are materialized for DCP/cascade paths.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logger = init_logger(__name__) | ||
|
|
||
| trtllm_gen_workspace_buffer = None | ||
| LOG2_E_SCALE = math.log(2.0) |
There was a problem hiding this comment.
LOG2_E_SCALE is ambiguous/misleading: math.log(2.0) is ln(2), which is specifically a base-conversion factor (log2 -> ln). Consider renaming to something unambiguous like LN_2 or LOG2_TO_LN to make the intent (and direction) clear and avoid future mistakes.
| self.dcp_world_size = 1 | ||
| self.dcp_rank = 0 | ||
| self.dcp_kv_cache_interleave_size = 1 | ||
| self.cp_kv_cache_interleave_size = 1 |
There was a problem hiding this comment.
The naming switches from dcp_* to cp_* while the surrounding fields remain dcp_world_size, dcp_rank, use_dcp, etc. This inconsistency makes it harder to reason about whether the interleave size is tied to DCP specifically or a broader CP concept. Either align the field name with the DCP terminology used in this class, or add a short clarifying comment explaining why the config uses cp_* while the runtime concept is DCP.
Signed-off-by: QiuChunshuo <qiuchunshuo@huawei.com>
| if seq_lens_np is not None | ||
| else None | ||
| seq_lens_cpu = ( | ||
| common_attn_metadata.seq_lens.cpu() if needs_seq_lens_cpu else None |
There was a problem hiding this comment.
why is this change needed? if its just refactoring I think we should break that into a separate PR
There was a problem hiding this comment.
I moved seq_lens_np to the bottom because the code snippet below might modify seq_lens_cpu, so I need to assign a value to seq_lens_np after that. However, this modification really shouldn't be included in this PR. I'll open a new PR to complete this modification.
There was a problem hiding this comment.
Open a new PR for this modifcation: #42205
Signed-off-by: QiuChunshuo <qiuchunshuo@huawei.com>
fc81fb2 to
7b65dea
Compare
Signed-off-by: QiuChunshuo <qiuchunshuo@huawei.com>
|
Hi @pisceskkk, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
|
Hi @pisceskkk, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, Tip Is
|
| if self.sinks is not None and self.sinks.dtype != torch.float32: | ||
| self.sinks = self.sinks.to(torch.float32) | ||
|
|
||
| def _get_dcp_decode_buffers( |
There was a problem hiding this comment.
can we just inline this?
There was a problem hiding this comment.
yes, have changed.
| num_decode_tokens: int, | ||
| ) -> tuple[torch.Tensor, torch.Tensor]: | ||
| assert self.dcp_world_size > 1 | ||
| total_num_heads = self.num_heads * self.dcp_world_size |
There was a problem hiding this comment.
is this always true? what about say TP=8 and DCP=2; our is this not truely total_num_heads just total_num_heads within DCP group?
There was a problem hiding this comment.
I think this is correct, right? The output should always maintain the same shape as decode_query.
decode_query = get_dcp_group().all_gather(
decode_query.contiguous(), dim=-2
)
There was a problem hiding this comment.
our is this not truely total_num_heads just total_num_heads within DCP group?
Sorry, I misunderstood the meaning. This variable indeed should not be called "total_num_heads." I have changed the name to ensure it aligns with its intended meaning.
Signed-off-by: QiuChunshuo <qiuchunshuo@huawei.com>
Signed-off-by: QiuChunshuo <qiuchunshuo@huawei.com>
Purpose
Allocate persistent buffers for DCP-related variables to enable FULL_DECODE_ONLY support.
Test Plan
Test Result
TP4DCP1
TP4DCP2(4ba38c8)
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.