Conversation
--use-replayssm selects the RecoverSSM path on Kimi-K3. That path cannot start today: it raises during cudagraph capture, and once that is fixed it raises again on the first execute_model. Both are unbound reads. 1. GDNAttentionMetadataBuilder.__init__ does not call AttentionMetadataBuilder.__init__, where layer_names is stored, and assigns its fields by hand -- vllm_config, compilation_config, speculative_config, kv_cache_spec. layer_names was left out. KimiK3KDAMetadataBuilder passes it up correctly; nothing receives it. Only _get_recoverssm_context reads the attribute, so no other path has noticed. 2. The checkpoint block guards on 'spec_sequence_masks_cpu is not None' while active_non_spec_mask_cpu is bound only in the num_spec_decodes > 0 branch; the sibling has_initial_state use guards on the latter. The two agree without RecoverSSM, because a step whose draft tokens sum to zero clears the mask -- but that clearing carries 'not self.use_recoverssm', so under RecoverSSM the mask survives with a zero count and the read is unbound. num_spec_decodes > 0 is also the correct alignment. checkpoint_offsets is consumed by _store_cache_checkpoints_kernel over a grid of its own numel, where seq_idx indexes non_spec_query_start_loc; when num_spec_decodes == 0 that tensor is the full query_start_loc, so request_rows must be all rows, which is what the fallback already builds. Signed-off-by: misunp <misunp@nvidia.com>
mispa-ms
marked this pull request as draft
August 31, 2026 03:31
Review feedback. self.device was missing for the same reason layer_names was: the builder never calls AttentionMetadataBuilder.__init__ and assigns its fields by hand. Call super() instead of adding a second field -- it restores all four the base stores, matches flash_attn / mamba_attn / triton_attn, and is two lines shorter. Two regression tests. The existing recoverssm tests set builder.recoverssm_context = Mock(), so _get_recoverssm_context returns early and the layer_names read is never executed; that is why CI is green on a path that cannot start. The first test asserts the base fields directly. The second mirrors test_internal_checkpoint_metadata_targets_last_aligned_boundary with use_recoverssm=True on a prefill-only batch, which is the real trigger: num_decode_draft_tokens is filled with -1 and written only on spec-decode rows, so the >= 0 mask is all False -- non-None with num_spec_decodes == 0. Signed-off-by: misunp <misunp@nvidia.com>
Review feedback: the test passed against the unfixed code, so it guarded nothing. build() takes num_decode_draft_tokens_cpu=None by default, and the first classification branch is 'not self.use_spec_decode or num_decode_draft_tokens_cpu is None', which sets spec_sequence_masks_cpu to None. The old guard was then False and active_non_spec_mask_cpu was never read. The docstring described the all-False mask correctly while the body never built one. Pass the -1 fill the runner uses on a prefill-only step, as test_recoverssm_distinguishes_draftless_decode_from_one_token_prefill already does. The mask is then a non-None all-False tensor with num_spec_decodes == 0: the old guard reads an unbound local, the new one takes request_rows = [0, 1]. Expectations are unchanged. Signed-off-by: misunp <misunp@nvidia.com>
mispa-ms
marked this pull request as ready for review
August 31, 2026 04:24
mispa-ms
requested review from
AndreasKaratzas,
DarkLight1337 and
ywang96
as code owners
August 31, 2026 04:24
Review feedback: the test did not run when its subject changed. The Kimi K3 suite triggers on vllm/models/kimi_k3/ and tests/models/kimi_k3/; the V1 Attention suite triggers on vllm/v1/attention/ and tests/v1/attention/. Neither list contains the other, so a later change to gdn_attn.py that drops layer_names or device again would not run the assert written to catch it -- restoring exactly the condition under which crash 1 survived. tests/v1/attention/test_gdn_metadata_builder.py already has a CPU DEVICE, no CUDA gate and a _create_gdn_builder() helper that passes layer_names and device, so the test is four lines there and exercises the shared builder directly rather than through the Kimi subclass. It also stops being pinned to b200-k8s for what is a pure CPU check. The checkpoint test stays in tests/models/kimi_k3/: its guard lives in vllm/models/kimi_k3/nvidia/kda_metadata.py and the trigger already matches. Signed-off-by: misunp <misunp@nvidia.com>
It described assigning fields instead of calling super() as current behaviour, sitting next to the super() call this PR adds. The next reader would take it as saying not calling super() is fine here. Signed-off-by: misunp <misunp@nvidia.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
--use-replayssmselects the RecoverSSM path on Kimi-K3. It cannot start: itraises during cudagraph capture, and after that is fixed it raises again on the
first
execute_model.1.
GDNAttentionMetadataBuildernever calls its base__init__.AttentionMetadataBuilder.__init__storeskv_cache_spec,layer_names,vllm_configanddevice. This builder calls none of it and assigns fourfields by hand, leaving out
layer_namesanddevice.layer_nameshas onereader,
_get_recoverssm_context;devicehas none yet. Callingsuper()restores both and matches
flash_attn,mamba_attnandtriton_attn.2. The checkpoint block's guard does not match the one that binds its
variable.
active_non_spec_mask_cpuis bound only undernum_spec_decodes > 0, which iswhat the sibling
has_initial_stateuse guards on. The checkpoint block guardson
spec_sequence_masks_cpu is not None.The reachable case is an all-False mask, not a zero draft sum.
num_decode_draft_tokensis filled with-1and written only on spec-decoderows, so on a prefill-only step
num_decode_draft_tokens_cpu >= 0is entirelyFalse. Without RecoverSSM the classification clears that mask to
None; theclearing carries
not self.use_recoverssm, so under RecoverSSM a non-Noneall-False mask survives with
num_spec_decodes == 0. This is therefore everyprefill step under
alignwith checkpoint blocks, not a rare corner, which iswhy it fires on the first
execute_model.The root cause is a dropped normalization clause. This file states it
"intentionally mirror[s] GDNAttentionMetadataBuilder", and the shared builder
keeps
spec_sequence_masks_cpu is not Noneequivalent tonum_spec_decodes > 0at the source:
That invariant is why guarding on the mask is safe there. The Kimi copy dropped
the
num_spec_decodes == 0disjunct and moved the count into theelse, andadding
not self.use_recoverssmthen made the gap reachable. Restoring theclause is the other possible fix; it is left as a follow-up because it would
make the mask
Noneagain and so stop the regression test below fromdistinguishing the two guards.
num_spec_decodes > 0is also the correct guard, not just the bound one:checkpoint_offsetsis consumed by_store_cache_checkpoints_kernelon a gridof its own
numel(), withseq_idxindexingnon_spec_query_start_loc. Atnum_spec_decodes == 0that tensor is the fullquery_start_loc, sorequest_rowsmust be every row — the fallback value. Masking there would haveshortened
request_rowsagainst a full-length start-loc array.Not a duplicate
active_non_spec_mask_cpureturns no results in this repo's issues or PRs. TheRecoverSSMhits are #51855 (closed, added this path), #52506 and #54255(FlashInfer backends), #54103 (ROCm). None touches either line.
Test
Two regression tests added. The existing RecoverSSM tests set
builder.recoverssm_context = Mock(), so_get_recoverssm_contextreturnsearly and the
layer_namesread never executes -- which is why CI is green ona path that cannot start.
Kimi-K3-MXFP4, 16×GB300, aggregated, TP16 × DCP8 × PP1, DSpark
num_speculative_tokens=4,mamba_cache_mode=align,mamba-backend=triton,VLLM_USE_V2_MODEL_RUNNER=1, 3600 s per point. Three arms one flag apart.--use-replayssm+3.2% at c64 and +4.5% at c128 over the triton control, in ITL.
Limitations
ignore_eos, so a stalerecurrent state would not show in them. GSM8K with
rejection_sample_method=standard, 1319 questions, 5 shots, same config oneflag apart: 0.9530 off / 0.9545 on, invalid 0.08% / 0.15%. Two questions
on 1319, and in the wrong direction for a staleness bug.
has no RecoverSSM or checkpoint path and does not read either field, so it is
unaffected.
AI assistance
Written with Claude Code. Both crashes were reproduced on hardware and the
tracebacks are verbatim.