Fix inter-chunk recurrence in the Zamba2 / Nemotron-H Mamba2 slow path - #47250
Fix inter-chunk recurrence in the Zamba2 / Nemotron-H Mamba2 slow path#47250Jeronymous wants to merge 4 commits into
Conversation
…on-H Mamba2 slow path
The pure-PyTorch (no-kernel) path of the Mamba2 mixer summed the
inter-chunk recurrence over the wrong dimension, reducing
new_state[j] = sum_i decay_chunk[j, i] * state[i]
to
new_state[q] = state[q] * sum_p decay_chunk[p, q]
so the per-chunk states were no longer mixed across chunk boundaries.
Align the implementation with mamba2 (transpose(1, 3) + sum over dim=1),
matching the fix made for mamba2 in huggingface#35154.
This affects Zamba2 and Nemotron-H (which inherits torch_forward from
Zamba2MambaMixer). The bug is masked for a single chunk with an empty
cache, but produces wrong outputs and wrong cached SSM states for
sequences longer than chunk_size or when continuing generation from a
populated cache.
b5b0aad to
4415fa5
Compare
vasqu
left a comment
There was a problem hiding this comment.
Thanks! Can we add test(s) similar to create_and_check_mamba2_slow_vs_fast_forward in the other PR that fixed this
Add a slow-path multi-chunk regression test for Zamba2 and Nemotron-H that checks a single chunked `torch_forward` over a multi-chunk sequence matches a token-by-token recurrent decode. A large-magnitude input is used so the SSM state is O(1) and the inter-chunk term is observable; with the previous reduction the two disagree by orders of magnitude. Runs on CPU without the fast-path kernels. Also add the Mamba2 slow-vs-fast (kernel vs torch) consistency test to Zamba2, mirroring the existing mamba2 and nemotron_h tests.
vasqu
left a comment
There was a problem hiding this comment.
This is more about the tests (design), the fix itself is good!
| config_and_inputs = self.model_tester.prepare_config_and_inputs() | ||
| self.model_tester.create_and_check_mamba2_slow_vs_fast_forward(*config_and_inputs) | ||
|
|
||
| def test_mamba2_slow_path_multi_chunk(self): |
There was a problem hiding this comment.
Hmm was create_and_check_mamba2_slow_vs_fast_forward not enough? Or why the whole new test design?
There was a problem hiding this comment.
Thanks. I reworked into a create_and_check_* helper driven by prepare_config_and_inputs.
| config = NemotronHConfig( | ||
| vocab_size=99, | ||
| hidden_size=32, | ||
| mamba_num_heads=8, | ||
| mamba_head_dim=8, | ||
| ssm_state_size=16, | ||
| n_groups=1, | ||
| mamba_chunk_size=8, | ||
| num_attention_heads=2, | ||
| num_key_value_heads=2, | ||
| head_dim=8, | ||
| intermediate_size=32, | ||
| use_mamba_kernels=False, | ||
| layers_block_type=["mamba"], | ||
| ) | ||
| torch.manual_seed(0) | ||
| mixer = NemotronHModel(config).eval().to(torch_device).layers[0].mixer | ||
|
|
||
| seq_len = 5 * config.chunk_size + 3 | ||
| hidden_states = 50.0 * torch.randn(1, seq_len, config.hidden_size, device=torch_device) |
There was a problem hiding this comment.
I would like to use prepare config and inputs if possible (if this test is still really needed after my comment above)
| msg=f"Max diff: {(ref_first - under_test_first).abs().max().item():.6f}", | ||
| ) | ||
|
|
||
| def create_and_check_zamba2_slow_vs_fast_forward(self, config, input_ids, *args): |
There was a problem hiding this comment.
I think it changes a bit on main re mamba2 especially re decorators can you check
There was a problem hiding this comment.
Zamba2 slow_vs_fast now gated with @require_torch_accelerator + @require_kernels, helper trimmed to match mamba2.
Wire the multi-chunk regression tests through prepare_config_and_inputs as create_and_check_* helpers, and gate the Zamba2 slow-vs-fast test with require_torch_accelerator + require_kernels to match mamba2.
CI recapDashboard: View test results in Grafana |
vasqu
left a comment
There was a problem hiding this comment.
Test design questions again
| seq_len = 4 * config.chunk_size + 1 | ||
| torch.manual_seed(0) | ||
| hidden_states = 100.0 * torch.randn(1, seq_len, config.hidden_size, device=torch_device) |
There was a problem hiding this comment.
I mean we are still manually creating here, no? The idea was to directly use the created input ids
There was a problem hiding this comment.
OK, thanks for clarifying. It should be fixed now.
feeds model.embeddings(input_ids) (the prepare_config_and_inputs ids) instead of a manual tensor. It's only rescaled so the SSM state is O(1), otherwise the inter-chunk term is ~1e-7 and invisible.
| msg=f"Max diff: {(ref_first - under_test_first).abs().max().item():.6f}", | ||
| ) | ||
|
|
||
| def create_and_check_nemotron_h_slow_path_multi_chunk(self, config, input_ids, *args): |
There was a problem hiding this comment.
But I'm still a bit confused - why do we have this new test and not just the slow vs fast path test? (which should catch the same regression)
There was a problem hiding this comment.
slow_vs_fast can't catch this one: it runs seq_length=7 < chunk_size, i.e. a single chunk with an empty cache, so the inter-chunk recurrence is never reached (previous_states=0, nothing to mix). Buggy and fixed give identical outputs there. It passes unchanged on the buggy code. The regression only shows with more than 2 chunks, hence the separate test.
| """ | ||
| config = copy.deepcopy(config) | ||
| config.chunk_size = 8 | ||
| model = NemotronHModel(config).eval().to(torch_device) |
There was a problem hiding this comment.
I mean we need to guarantee that this uses the slow path and here it could result into the fast path. Wouldnt it make more sense to just force cpu here?
There was a problem hiding this comment.
Done. The model is now .to("cpu") so it always takes the torch_forward slow path regardless of kernel availability.
Drive the multi-chunk regression tests from the prepare_config_and_inputs input_ids and force CPU so the torch (slow) path is exercised.
|
Thank you for your contribution 🤗! CI Security Gate — automatic approval blockedThis PR was not automatically approved for CI because the security gate failed. Possible reasons:
See the workflow run for the exact violations. A maintainer can review and manually approve CI if a finding is a false positive. |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: nemotron_h, zamba2 |
vasqu
left a comment
There was a problem hiding this comment.
Gotcha thanks, I will check tomorrow again. Could you ping me then? I think we are mostly fine to merge
|
run-slow: nemotron_h, zamba2 |
|
This comment contains models: ["models/nemotron_h", "models/zamba2"] |
CI ResultsCommit Info
The test failure analysis could not be completed. Please check the workflow run for details. |
|
@vasqu is there anything still blocking this? It's approved, the slow tests were triggered (run-slow: nemotron_h, zamba2). Also: the closed #47513 flagged a related slow-path issue in the same mixer (dt is clamped one-sided instead of two-sided against time_step_limit, unlike the kernel path). Want me to fold that fix into this PR so it fully closes #47246, or keep it separate? |
|
Hey @Jeronymous 👋 sorry I refactored all the mamba / lin attn models in #47630 so they all should work now, could you recheck? I lost track and was out of office for a bit sorry about that |
|
Rechecked. Looks good. I close this PR since the fix is redundant now. One thing worth keeping though: the test_mamba2_slow_path_multi_chunk regression test I added here : it catches exactly this bug (multi-chunk slow-path forward vs. token-by-token recurrent decode, which the single-chunk slow_vs_fast test misses). |
|
Hey, I don't mind to add the test into e.g. only mamba2 as a baseline wdyt? |
|
Yes @vasqu it sounds good |
What does this PR do?
Fixes an incorrect reduction dimension in the inter-chunk SSM recurrence of the pure-PyTorch (no-kernel)
torch_forwardpath shared byZamba2MambaMixerandNemotronHMamba2Mixer.The current code
sums over the target-chunk axis while
statesis broadcast over it, sostatesfactors out of the sum. This collapses the intended recurrenceinto
i.e. the per-chunk states are no longer propagated across chunk boundaries.
This is the same bug that was reported for
mamba2in #34817 and fixed in #35154; the fix was never propagated tozamba2, and therefore tonemotron_h, whose mixer inheritstorch_forwardfromZamba2MambaMixer. All other Mamba2 descendants (bamba,falcon_h1,granitemoehybrid) already use the corrected form. The change aligns these two models withmamba2:It was applied to
modular_zamba2.pyand mirrored into the generatedmodeling_zamba2.pyandmodeling_nemotron_h.py(identical lines, no name substitution), socheck_modular_conversionstays green.Scope of impact
Only the slow path is affected (no
mamba-ssm/causal-conv1dkernels — e.g. CPU, oruse_mamba_kernels=False); the CUDA kernel path is correct. The bug is masked for a single chunk with an empty cache (seq_len <= chunk_size, fresh prefill), which is why existing short-sequence tests pass. It produces wrong outputs and wrong cached SSM states when:chunk_size(prefill spanning ≥ 2 chunks), orReproduction
Standalone numerical check (no model download)
Output:
Fixes #47246
Who can review?
@vasqu (fixed the equivalent
mamba2bug in #35154)