GLM-5.2-FP8 with DSA enablement - #1777
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The sparse indexer may return partially uninitialized top-k index buffers (risking out-of-range cache gathers) and the sparse decode softmax can produce NaNs for fully-masked rows.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Enables GLM-5.2 FP8 execution with DSA (Dynamic Sparse Attention) on Intel Gaudi HPU by wiring an HPU BF16-based sparse-attention indexer into the DeepSeek/GLM MLA path and adding a sparse MLA decode implementation.
Changes:
- Adds an HPU SparseAttnIndexer implementation that scores Q·K in BF16/FP32 and selects top-K cache slots via
torch.topk. - Routes DeepSeek/GLM indexer and MLA attention to use the HPU sparse path (including BF16 indexer cache handling and SparseAttnIndexer dispatch).
- Extends the HPU MLA attention backend to support sparse decode and updates MLA KV-cache binding to handle HPU’s per-layer KV-cache tuple.
File summaries
| File | Description |
|---|---|
vllm_gaudi/platform.py |
Enables sparse+MLA backend selection logging and adds an HPU platform hook for runner KV-cache behavior. |
vllm_gaudi/ops/hpu_sparse_attn_indexer.py |
Introduces the HPU BF16 sparse-attention indexer that produces top-K physical slot indices per request. |
vllm_gaudi/models/deepseek_v2.py |
Monkey-patches DeepSeek indexer cache/forward and SparseAttnIndexer to use the HPU indexer implementation. |
vllm_gaudi/attention/oot_mla.py |
Enables sparse routing for MLA decode and overrides KV-cache binding to keep the HPU tuple intact. |
vllm_gaudi/attention/backends/hpu_attn.py |
Adds forward_mqa_sparse to perform sparse decode attention over selected cache slots. |
Review details
Suppressed comments (1)
vllm_gaudi/ops/hpu_sparse_attn_indexer.py:54
- In the
seq_lens is Nonefallback, the code only fills:topkcolumns oftopk_indices_bufferand returns without initializing/padding the remaining columns. The sparse attention decode gathers using the full buffer width, so any uninitialized columns can contain stale/out-of-range slot indices and lead to invalid cache reads or indexing errors. Pad the remaining columns with a valid slot index (e.g., last valid slot) whentopk < self.topk_tokens.
topk = min(self.topk_tokens, all_slots.shape[0])
self.topk_indices_buffer[:batch_size, :topk] = (
all_slots[:topk].unsqueeze(0).expand(batch_size, -1))
return self.topk_indices_buffer
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
pawel-olejniczak
left a comment
There was a problem hiding this comment.
Reviewed at 867254b against VLLM_STABLE_COMMIT 2cf0a691.
The main thing to resolve before this lands: I do not think the sparse path can execute on this branch. There are three independent gates, any one of which routes every DSA decode back to dense MLA - see the inline comments on oot_mla.py:168, hpu_sparse_attn_indexer.py:27 and hpu_sparse_attn_indexer.py:47. The first is that this backport is missing the hpu_model_runner.py change that the main-branch sibling carries; the other five files here are byte-identical to it.
That also affects how I read the validation numbers. Dense MLA is exact for sequences up to index_topk tokens, so GSM8K and AIME agreeing with the model card is consistent with the sparse path never running, and does not by itself show that it did. Something that separates the two paths would help, for example a log line or counter proving forward_mqa_sparse was entered, or an eval at a context length well past index_topk.
Two other asks:
- Performance numbers. DSA is a latency and throughput feature, and the description only reports accuracy, while the dense-fallback PR it supersedes reported median TTFT 1.82 s / TPOT 88.5 ms / 353.7 tok/s. Once the scoring path is actually reached it is a Python loop over the batch with a per-request
int(seq_lens[i].item())host sync, inside@torch.compiler.disable. Could you post TPOT and output throughput at a long context next to that dense baseline? pre-commitwill fail as is: the pinned yapf 0.43.0 rewrites 12 hunks across four of the five files. Pinned ruff 0.11.7 is clean.
| block_size = attn_metadata.block_size | ||
| slot_mapping = attn_metadata.slot_mapping.flatten() | ||
|
|
||
| if kv_cache is None or kv_cache.numel() == 0: |
There was a problem hiding this comment.
On this branch I believe this early return is the only path this function ever takes. get_kv_cache_spec in vllm_gaudi/v1/worker/hpu_model_runner.py:1717 handles only MambaBase, Attention and MLAAttention, and DeepseekV32IndexerCache is none of those - it is a plain AttentionLayerBase (upstream vllm/model_executor/models/deepseek_v2.py:604). So the indexer cache layer never gets a spec, never gets an allocation, and self.k_cache.kv_cache keeps the torch.tensor([]) it is initialised with at deepseek_v2.py:609. kv_cache.numel() == 0 is then true on every step: the index_copy_ below never inserts K, and the returned indices are always 0..topk_tokens-1.
The main-branch sibling (#1760) adds exactly the missing branch to get_kv_cache_spec, and this PR is otherwise byte-identical to it in all five shared files. The new check_runner_kv_caches_multi_layer no-op in platform.py only has an effect once the indexer cache is in kv_caches, which also points at that change being intended here.
Please cherry-pick the elif isinstance(attn_module, AttentionLayerBase) branch from #1760. One thing worth confirming when you do: it adds a second entry per DSA layer to runner_kv_caches, which is the case upstream's own TODO at vllm/v1/worker/utils.py:529-541 describes as unanalyzed - have you checked that nothing on the HPU side indexes runner_kv_caches positionally?
There was a problem hiding this comment.
runner_kv_caches is not consumed by any function / read by any other variable.
kv_caches is also binded to model_runner.kv_Caches, which is consumed by defragmenter. I switched off the defrag for this model (and cont pa) in features.py - we don't want it to manipulate indexer cache. model_runner.kv_caches is also used in hpu_worker.py:L531-550, but there only first item from kv cache is taken and it's not indexer cache.
|
@jkaniecki do we need README and examples update? |
Enable GLM-5.2 with DSA (DeepSeek Sparse Attention) on HPU. What changed: - hpu_sparse_attn_indexer.py (new): per-request Q.K BF16 scoring + torch.topk to select top-2048 KV cache slots per decode step. Rewritten to derive per-request valid slots via block_groups/block_usage masking instead of a fragile block_offset walk that assumed block_list was an unpadded per-request concatenation (breaks under contiguous PA, which scatters/reorders blocks by physical block id). Padding now uses the upstream -1 sentinel convention instead of repeating the last valid slot or leaving stale buffer contents. - hpu_attn.py: forward_mqa_sparse - gathers top-K latent KV, decompresses, runs masked MLA decode attention. Masking is now driven directly by the -1 sentinel in topk_indices instead of seq_lens_tensor/context_lens_tensor, which are always None on HPU decode for models without mamba-like layers (the previous fallback silently handed every request in the batch the same first topk_tokens slots). Uses torch.finfo(dtype).min instead of -inf for masking to avoid NaN on fully-padded rows, and zeroes fully padded rows' output with a dtype- and shape-correct multiply (previously promoted bf16 attn to fp32 before a bf16 matmul, and used a 3D mask that only broadcast correctly by coincidence when num_heads == batch_size). HPUMLAImpl.__init__ now stores topk_indices_buffer (previously swallowed by **kwargs and never read) and sets is_sparse per-instance, which is what actually makes forward_mqa_sparse reachable. - oot_mla.py: dispatches to forward_mqa_sparse when use_sparse=True, reading topk_indices_buffer from self.impl (where HPUMLAImpl now stores it) instead of the wrapper module (where it was never set). - deepseek_v2.py: BF16 indexer cache, HPU-safe Indexer.forward, SparseAttnIndexer dispatch. - platform.py / hpu_model_runner.py: DSA routing + indexer cache layer index fix. Ports the get_kv_cache_spec AttentionLayerBase branch from the main-branch sibling (vllm-project#1760), which this backport was missing - without it DeepseekV32IndexerCache never gets a KV cache allocation, so kv_cache.numel() is always 0 and the indexer never runs. Addresses review feedback from Pawel Olejniczak and Copilot on PR vllm-project#1777: - Sparse decode path is now actually reachable (three independent gates previously routed every DSA decode back to dense MLA). - Fixed NaN-producing masked_fill(-inf) on fully-masked rows. - Fixed uninitialized/stale topk_indices_buffer columns. - Fixed bf16->fp32 dtype promotion before a bf16 matmul. - Removed unused _orig_forward_native and an orphaned comment. - Added missing -> None annotation on check_runner_kv_caches_multi_layer. - Applied yapf 0.43.0 (column_limit=120) formatting; ruff 0.11.7 clean. Still open (not addressed here, needs checkpoint inspection on the pod): whether the validated GLM-5.2-FP8 checkpoint still ships indexers_proj weights now that the load_weights filter for them has been dropped. Validated on 8x Gaudi3 (GLM-5.2-FP8, TP=8): GSM8K 5-shot (1319 samples): 93.6% exact match Signed-off-by: Jan Kaniecki <jkaniecki@habana.ai>
867254b to
7f9a844
Compare
…ejniczak) - get_kv_cache_spec (already present): DSA indexer cache gets a spec so kv_cache.numel() != 0 and the sparse path is actually reachable - hpu_sparse_attn_indexer.py: derive per-request valid cache slots from block_groups/block_usage masking instead of an always-None seq_lens_tensor/context_lens_tensor fallback and a block_offset walk that assumed an unpadded per-request block_list layout; adopt upstream's -1 sentinel for padding instead of repeating a valid slot - forward_mqa_sparse: mask via the -1 sentinel directly instead of the unreliable seq_lens derivation; use finfo(dtype).min instead of -inf to avoid NaN on fully-masked rows; fix a bf16->fp32 dtype promotion bug in the empty-row zero-mask multiply - HPUMLAImpl: store topk_indices_buffer (previously swallowed by **kwargs) and set is_sparse per-instance so forward_mqa_sparse is actually reachable - oot_mla.py: read topk_indices_buffer from self.impl (where it is actually stored) instead of the wrapper; drop an orphaned comment - deepseek_v2.py: drop an unused _orig_forward_native assignment - platform.py: add -> None annotation to check_runner_kv_caches_multi_layer Signed-off-by: Jan Kaniecki <jkaniecki@habana.ai>
…g for GLM DSA
- forward_mqa_sparse: empty_mask needs a 4D [B,1,1,1] view to align with
attn's [B,H,1,T] shape. A 3D [B,1,1] view broadcasts against attn's last
3 dims [H,1,T] instead, silently applying the wrong mask whenever
batch_size != num_heads (previously masked by a test where they happened
to be equal).
- features.py: disable use_contiguous_pa (and therefore defrag, which is
defined as Enabled('use_contiguous_pa')) for glm_moe_dsa. Per Pawel
Olejniczak's PR vllm-project#1777 review question about whether anything indexes
runner_kv_caches positionally: CacheSwapUtils.forward swaps every
kv_caches entry uniformly, including the DSA indexer's K-only cache,
which is a different shape/purpose than a standard attention KV cache
and has not been validated under block remapping. Disable contiguous PA
and defrag for this model until that path is verified.
Signed-off-by: Jan Kaniecki <jkaniecki@habana.ai>
@pawel-olejniczak Thanks for the complex review, I've changed the code to enable DSA properly, confirmed it is now launching. About testing - for sure GSM8K was only a sanity check, not showing real DSA capabilities. AIME 2026 is a bit different - some samples overcome this 2k topk threshold and switch on the DSA, so it's really working. I've also ran tests up to 128k context to see long-context capabilities using "needle" type task - if was successful in all runs. About perf numbers - it's only a functional enablement for now, so not sharing perf results, I haven't focus on them. The MLA fallback PR may have some perf numbers (even better than DSA version due to switching t.compile off) but in fact it enables different model - GLM5.2 was trained and should be executed with DSA. Without it, we cannot be sure about it's accuracy for samples larger than top_k. Dense MLA version can also show good acc at multiple sample ranges, but it cannot be said that it's the same acc that GLM5.2 should have then. |
Enable GLM-5.2 with DSA (DeepSeek Sparse Attention) on HPU
What changed:
hpu_sparse_attn_indexer.py (new): per-request Q·K BF16 scoring + torch.topk to select top-2048 KV cache slots per decode step
hpu_attn.py: forward_mqa_sparse — gathers top-K latent KV, decompresses, runs masked MLA decode attention
oot_mla.py: dispatches to forward_mqa_sparse when use_sparse=True
deepseek_v2.py: BF16 indexer cache, HPU-safe Indexer.forward, SparseAttnIndexer dispatch
platform.py / hpu_model_runner.py: DSA routing + indexer cache layer index fix
Validated on 8× Gaudi3 (GLM-5.2-FP8, TP=8):
GSM8K 5-shot (1319 samples): 93.6% exact match
AIME 2026 - 100% - on pair with model card's target (99,2 % mean for multiple reruns)