[glm-moe-dsa] Indexer uses interleaved rope - #46842
Conversation
|
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. |
|
Testing confirms the issue. I'll apply the fix to modular. |
|
This is ready for review now. |
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
Thanks, let's avoid having that flag if its set in stone that DSv32 use interleaved and dsa does not
The property exists in GLM models. I will hardcode for now, we can revisit if/when more models use DSA. |
ArthurZucker
left a comment
ArthurZucker
left a comment
There was a problem hiding this comment.
Ty! if we just re-define properly outside should work? or is this an naming issue that modular does not catch well? (the functions have the same name)
|
|
||
| # The indexer uses NON-interleaved (half-split) RoPE — unlike the main MLA attention | ||
| q_rot, k_rot = apply_rotary_pos_emb(q_rot, k_rot, cos, sin, unsqueeze_dim=2) | ||
| q_rot, k_rot = self.apply_indexer_rotary_pos_emb(q_rot, k_rot, cos, sin) |
There was a problem hiding this comment.
unsure I understand the need for the self function here!
|
|
||
| # The indexer uses NON-interleaved (half-split) RoPE — unlike the main MLA attention | ||
| q_rot, k_rot = apply_rotary_pos_emb(q_rot, k_rot, cos, sin, unsqueeze_dim=2) | ||
| q_rot, k_rot = self.apply_indexer_rotary_pos_emb(q_rot, k_rot, cos, sin) |
There was a problem hiding this comment.
same here! let' s leave it as defined outside no?
|
[For maintainers] Suggested jobs to run (before merge) run-slow: glm_moe_dsa |
CI recapDashboard: View test results in Grafana |
|
Thanks a lot for the patience @ArthurZucker, merging now then! |
pcuenca
changed the title
* [glm-mode-dsa] Indexer uses interleaved rope * Apply to modular, read from config * Remove from config * Copy and modify forward implementation
3 participants
What does this PR do?
This is a proof-of-concept for discussion, the real fix will be more general (read below).History as I understand it:
indexer_rope_interleaveto the config, but it was not implemented (we defaulted to full attention).Another data point:
How this manifests in real-life
I'm testing again to exclude the transformers branch as a confounder; I think the result is a not-obvious quality degradation for long sequences.
Confirmed: generation degrades as sequence length increases.
Before this PR, I observed subtle degradation in long-context generation, but not obvious signals such as corrupted text. For example, asking to generate a space invaders game I saw syntax errors like in the following line (0.0.4is not a number):this.tone(120, 0.0.4, 'sawtooth', 0.1, -80);These errors did not happen with the fix in this PR. But I can't still rule out the reason might have been different (my "bad output" test was run before the on-going FP8/parallel fixes).Real Fix
If this is indeed a bug, I think the real fix should be to read
indexer_rope_interleavein the DS implementation and act accordingly.Code Agent Policy
Before submitting
Pull Request checks?
to it if that's the case.