Fix use_cache with seq_len > 1 ( #46032)#46084
Conversation
|
I didn't run the cuda tests, can anybody test it? |
6bc871f to
03f48bf
Compare
vasqu
left a comment
There was a problem hiding this comment.
Some initial comments from my side, this is not about functionality (it seems correct to me) but more on style
Ideally we would propogate the changes to other mamba2 related models like bamba, falcon, zamba2 etc. But no worries let's focus on getting the base mamba2 right first
df45189 to
aac20d1
Compare
vasqu
left a comment
There was a problem hiding this comment.
Sorry that im so nitpicky but I really want to avoid multiple different versions of essentially the same. We should follow qwen3 next as close as possible where we use the precomputed states and seq len var where we need + more aligned forward
|
|
||
| # Single step calculations via cache | ||
| if cache_params is not None and cache_params.has_previous_state(self.layer_idx): | ||
| if cache_params is not None and cache_params.has_previous_state(self.layer_idx) and seq_len == 1: |
There was a problem hiding this comment.
Please let's follow Bamba and make this a var use_precomputed_states = ...
| else: | ||
| A = -torch.exp(self.A_log.float()) # (num_heads) or (intermediate_size, state_size) | ||
| dt_limit_kwargs = {} if self.time_step_limit == (0.0, float("inf")) else {"dt_limit": self.time_step_limit} | ||
| has_previous_state = cache_params is not None and cache_params.has_previous_state(self.layer_idx) |
| return_final_states=True, | ||
| dt_bias=self.dt_bias, | ||
| dt_softplus=True, | ||
| initial_states=cache_params.layers[self.layer_idx].recurrent_states if has_previous_state else None, |
There was a problem hiding this comment.
We can extract the prev states (conv and recurrent before) similar to qwen3 next
transformers/src/transformers/models/qwen3_next/modeling_qwen3_next.py
Lines 613 to 616 in 1aa495d
| ) | ||
| else: | ||
| hidden_states_B_C = causal_conv1d_fn( | ||
| if has_previous_state: |
There was a problem hiding this comment.
Would also be nice if we could follow qwen3 next more closely here
transformers/src/transformers/models/qwen3_next/modeling_qwen3_next.py
Lines 626 to 656 in 1aa495d
I just want to avoid having the same but in different ways, we should stay consistent
| cache_params is not None and cache_params.has_previous_state(self.layer_idx) and seq_len == 1 | ||
| ) | ||
| # Snapshot before any cache | ||
| has_previous_state = cache_params is not None and cache_params.has_previous_state(self.layer_idx) |
There was a problem hiding this comment.
Would avoid this extra var and just check with seq len where actually needed
|
I was working to make it look closer like bamba script. I'll rework on it today |
|
Sorry I was to eager then 😬 feel free to ping me when ready, ignoring the thread for now then |
|
Sure thing, I'll let you know |
4e08cde to
9752bab
Compare
|
Should I change |
|
I changed for now, @vasqu lmk if that's right |
vasqu
left a comment
There was a problem hiding this comment.
Smaller nits but would you be up to fixing this on any mamba2 related model? I think this is a nice opportunity to do so
See it as pretty much approved for the core structure
4e51f2f to
720ca05
Compare
|
Hey @vasqu , Got bit busy lately. I have modified based on your suggestion. Can you take look and lmk if I'm missing any. I couldn't run |
vasqu
left a comment
There was a problem hiding this comment.
Heya looks pretty good, I made a suggestion on the cuda fast path because I want this to be closer to the qwen implementation. Lmk what you think
Other than that should be ready to merge then 🤗
c769ea0 to
13efaa4
Compare
|
Made changes based on your suggestion, let me know if I miss any |
Match mamba2 (huggingface#46084) and the qwen3-next reference by prepending the full cached conv_states instead of conv_states[:, :, 1:]. The two are numerically identical: the extra oldest column only adds one warm-up conv position that the trailing [-seq_len:] slice drops. This keeps the chunked-prefill left-context consistent with the reference path. Signed-off-by: Ting Sun <suntcrick@gmail.com>
vasqu
left a comment
There was a problem hiding this comment.
The forward looks good to me now. I would only adjust the tests a bit then we can merge 🤗
|
|
||
| def create_and_check_mamba2_slow_vs_fast_forward(self, config, input_ids, *args, gradient_checkpointing=False): | ||
| model = Mamba2Model(config) | ||
| def create_and_check_mamba2_chunked_prefill(self, config, *args, device="cpu"): |
There was a problem hiding this comment.
Hmm, should we not adapt
for mamba2 instead?There was a problem hiding this comment.
just the self?
There was a problem hiding this comment.
I mean that the test in qwen 3.5 is already testing what we need, we don't need to come up with something else can just reuse the patterns there
There was a problem hiding this comment.
I changed the single token prefill and added other changes but not sure at this
…on-H
cuda_kernels_forward had the same defect as the torch path: a multi-token
forward with a primed cache (chunked prefill / speculative verification) was
routed into the single-step causal_conv1d_update / selective_state_update
kernels and crashed ("weight must have shape (dim, width)"). Gate the
single-step branch on seq_len == 1; for the multi-token cached case run the
full kernels seeded from the cache (prepend the cached conv left-context and
drop it after the conv, pass the cached recurrent state as initial_states to
mamba_chunk_scan_combined), mirroring huggingface#46084.
Signed-off-by: Ting Sun <suntcrick@gmail.com>
ffb3df6 to
d805833
Compare
Add a third dispatch path to Mamba2Mixer for the case where a prior cached state exists and seq_len > 1 (chunked prefill / speculative decode verification). Previously: - cuda_kernels_forward: .squeeze(1) crashed or corrupted tensors for seq_len > 1 when has_previous_state was True. - torch_forward: dt[:, 0, :] silently dropped all tokens after index 0, and previous_states was always zero-initialised ignoring the cache. Both paths now: - Gate the single-step kernel on seq_len == 1. - For seq_len > 1, prepend the cached conv buffer (last K-1 entries) to the chunk input, run a full causal conv, drop the prepended prefix from the output, and pass the cached recurrent_state as initial_states to mamba_chunk_scan_combined / as previous_states in the naive SSD. This PR follows the same pattern applied to GDN-based models in huggingface#45513, as discussed in the issues I added test_mamba2_chunked_prefill (CPU/torch path, always runs) and test_mamba2_chunked_prefill_cuda_path (skipped without mamba-ssm).
e744224 to
3424102
Compare
|
I think it looks clean now! imo @vasqu have look when you're free |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: mamba2 |
vasqu
left a comment
There was a problem hiding this comment.
Should be good to go now, just using slow CI in a sec to sanity check
|
run-slow: mamba2 |
|
This comment contains models: ["models/mamba2"] |
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
|
Also checked locally and seems fine, merging now thanks a lot! |
|
Thanks for your suggestions! Got many lessons on mistakes that I make. |
Reshape the regression test to match the form merged in huggingface#46084: a model-level first-token causal check (the first position of a multi-token cached forward must equal the single-token cached forward), with separate CPU and accelerator variants. Nemotron-H has no CPU variant because its MoE layers use grouped_mm, which has no CPU kernel. Signed-off-by: Ting Sun <suntcrick@gmail.com>
…eMoeHybrid These models carry their own Mamba2 mixer and gate the single-step branch on seq_len == 1, but their multi-token cached path still starts from a zero conv context and zero SSM state, so chunked-prefill / speculative verification is history-less. Seed the multi-token cached path from the cache (prepend the cached conv left-context, pass the cached recurrent state as the SSM initial state), mirroring the Zamba2/Nemotron-H fix and huggingface#46084. Add the chunked-prefill regression test to each. Signed-off-by: Ting Sun <suntcrick@gmail.com>
…n-H, Bamba, FalconH1 and GraniteMoeHybrid (huggingface#46741) * Fix Mamba2 mixer chunked-prefill/speculative decode for Zamba2 and Nemotron-H The Mamba2 mixer took the single-step decode branch whenever the cache held a previous state, ignoring seq_len. For seq_len > 1 with a non-empty cache (chunked prefill, speculative/assisted decode verification) it used only the first tokens dt plus one conv/SSM step, silently producing wrong logits for the rest of the chunk and corrupting the cached state. Gate the single-step branch on seq_len == 1; for the multi-token cached case run the full path seeded from the cache (prepend the cached conv left context, pass the cached recurrent state as the initial SSM state). Same root cause and fix as mamba2 (huggingface#46032); applied in modular_zamba2.py so the change propagates to nemotron_h, which inherits the mixer. Signed-off-by: Ting Sun <suntcrick@gmail.com> * Prepend the full cached conv_states in the Mamba2 chunked path Match mamba2 (huggingface#46084) and the qwen3-next reference by prepending the full cached conv_states instead of conv_states[:, :, 1:]. The two are numerically identical: the extra oldest column only adds one warm-up conv position that the trailing [-seq_len:] slice drops. This keeps the chunked-prefill left-context consistent with the reference path. Signed-off-by: Ting Sun <suntcrick@gmail.com> * Fix Mamba2 cuda_kernels_forward chunked-prefill for Zamba2 and Nemotron-H cuda_kernels_forward had the same defect as the torch path: a multi-token forward with a primed cache (chunked prefill / speculative verification) was routed into the single-step causal_conv1d_update / selective_state_update kernels and crashed ("weight must have shape (dim, width)"). Gate the single-step branch on seq_len == 1; for the multi-token cached case run the full kernels seeded from the cache (prepend the cached conv left-context and drop it after the conv, pass the cached recurrent state as initial_states to mamba_chunk_scan_combined), mirroring huggingface#46084. Signed-off-by: Ting Sun <suntcrick@gmail.com> * Align Zamba2/Nemotron-H chunked-prefill tests with merged mamba2 test Reshape the regression test to match the form merged in huggingface#46084: a model-level first-token causal check (the first position of a multi-token cached forward must equal the single-token cached forward), with separate CPU and accelerator variants. Nemotron-H has no CPU variant because its MoE layers use grouped_mm, which has no CPU kernel. Signed-off-by: Ting Sun <suntcrick@gmail.com> * Propagate Mamba2 chunked-prefill cache fix to Bamba, FalconH1, GraniteMoeHybrid These models carry their own Mamba2 mixer and gate the single-step branch on seq_len == 1, but their multi-token cached path still starts from a zero conv context and zero SSM state, so chunked-prefill / speculative verification is history-less. Seed the multi-token cached path from the cache (prepend the cached conv left-context, pass the cached recurrent state as the SSM initial state), mirroring the Zamba2/Nemotron-H fix and huggingface#46084. Add the chunked-prefill regression test to each. Signed-off-by: Ting Sun <suntcrick@gmail.com> * Extract cached conv/recurrent state into locals in the Mamba2 mixers Bind the cached conv_states / recurrent_states once after use_precomputed_states, matching mamba2, instead of re-reading cache_params.layers[self.layer_idx] inline in the cat / initial_states / single-step paths. Pure refactor, no behavior change. Signed-off-by: Ting SUN <suntcrick@gmail.com> * Add CPU variant for the nemotron_h chunked-prefill test NemotronH MoE experts dispatch through integrations.moe to torch._grouped_mm, which only gained a CPU kernel in torch 2.11; guard the new _cpu test with require_torch_greater_or_equal("2.11") so it runs on current CI and skips on older torch. Signed-off-by: Ting SUN <suntcrick@gmail.com> * rename a bit --------- Signed-off-by: Ting Sun <suntcrick@gmail.com> Signed-off-by: Ting SUN <suntcrick@gmail.com> Co-authored-by: vasqu <antonprogamer@gmail.com>
What does this PR do?
Add a third dispatch path to Mamba2Mixer for the case where a prior cached state exists and seq_len > 1 (chunked prefill / speculative decode verification).
Previously:
Both paths now:
This PR follows the same pattern applied to GDN-based models in #45513, as discussed in the issues I added test_mamba2_chunked_prefill (CPU/torch path, always runs) and test_mamba2_chunked_prefill_cuda_path (skipped without mamba-ssm).
Fixes #46032
Code Agent Policy
The Transformers repo is currently being overwhelmed by a large number of PRs and issue comments written by
code agents. We are currently bottlenecked by our ability to review and respond to them. As a result,
we ask that new users do not submit pure code agent PRs at this time.
You may use code agents in drafting or to help you diagnose issues. We'd also ask autonomous "OpenClaw"-like agents
not to open any PRs or issues for the moment.
PRs that appear to be fully agent-written will probably be closed without review, and we may block users who do this
repeatedly or maliciously.
This is a rapidly-evolving situation that's causing significant shockwaves in the open-source community. As a result,
this policy is likely to be updated regularly in the near future. For more information, please read
CONTRIBUTING.md.Before submitting
Pull Request section?
to it if that's the case. Mamba2Mixer: use_cache with seq_len > 1 silently produces incorrect results (both CPU and GPU paths) #46032
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.
@zucchini-nlp @Cyrilvallez 🤗@vasqu @clara2911