Fix AIM row alignment across batch and head boundaries - #2
Open
ybochkov wants to merge 1 commit into
Open
Conversation
get_aim_states and get_aim_star_states flattened (batch, head) rows and detected word boundaries with roll() and a global cumsum(). Word ids touching a row boundary (or the wrap-around between the tensor's ends) could collide, merging word pairs from unrelated rows or dropping final words. Teacher and student AIM states are built independently, so any merged or dropped row shifted every following row and broke their one-to-one correspondence in the loss. Assign pair ids per (batch row, head) slice instead, and return an empty state tensor when a batch contains no complete word pair rather than crashing on max() of an empty tensor. Add regression tests that keep identical word ids separate across batch rows and across attention heads for both AIM and AIM*. Co-Authored-By: Claude Fable 5 <noreply@anthropic.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.
Summary
get_aim_statesandget_aim_star_statesflatten the (batch, head) structure and detect word boundaries withroll()and a globalcumsum(). Adjacent rows in the flattened tensor can then leak into each other: whenever the word ids touching a row boundary (or the wrap-around between the tensor's first and last elements) happen to be equal, word pairs from unrelated rows or heads are merged into one pair row or dropped entirely. Becauserepeat_interleaveplaces the per-head copies of the same row next to each other, a colliding row collides at every head boundary at once.Teacher and student AIM states are built independently, so one merged or dropped row shifts every following row and breaks their one-to-one correspondence in the loss:
common_stepskips the batch with a dummy loss;valid_word_ids.max()of an empty tensor.Severity depends on the tokenizer. When every sequence starts with a special token mapped to
-100(Llama/Gemma/Mistral-style BOS), the leading-100acts as a separator at each flattened row boundary, and randomized synthetic batches (right-padded, longest row unpadded, 1–4 heads) show no divergence from the fixed implementation — past runs with such models were most likely unaffected. For tokenizers without a leading special token (e.g. Qwen3, which this repo ships an adapter for), the same randomized stress test produced wrong AIM/AIM* states in roughly 10–15% of batches and crashed in 4%.Fix
Assign pair ids per (batch row, head) slice: the new
causal_word_pair_idshelper keeps the row/head structure until the very end, so boundary collisions are impossible by construction, andget_aim_statesreturns an empty state tensor when a batch contains no complete word pair instead of crashing. A sharedlast_token_maskhelper replaces the roll-based last-token detection in both AIM and AIM*.Behavior is preserved where the previous code was correct: on randomized BOS-style batches the old and new implementations agree exactly (200/200 trials).
Note: this removes the implementation-induced pair mismatches, not every possible "Word states size mismatch" — teacher and student are tokenized independently, so truncation can still produce genuinely different word sets; that remains the guard's job.
Tests
tests/test_aim_impl.pyadds regression tests that keep identical word ids separate across batch rows and across attention heads, for both AIM and AIM*. All four fail on the previous implementation (one via the empty-tensor crash) and pass with the fix.🤖 Generated with Claude Code