Fix Llama left-padding position IDs during training - #48946
lindicaphxag-tech wants to merge 2 commits into
Conversation
sylvesterkaczmarek
left a comment
There was a problem hiding this comment.
This branch is unreachable in the normal training call used by the new test. use_cache comes from the config and defaults to true, so an empty DynamicCache is created before the past_key_values is None check. The test can still pass because the two position schemes differ by a constant shift per row, which RoPE makes output-equivalent. Can we make the condition handle the empty-cache training case and add a test that actually proves this position-id branch ran?
|
[For maintainers] Suggested jobs to run (before merge) run-slow: llama |
CI recapDashboard: View test results in Grafana |
|
I updated the condition in 7dadbd2 to handle the empty DynamicCache created by the normal training call ( |
|
Rechecked 7dadbd2. The empty-DynamicCache training path now reaches the left-padding position-id branch, and the revised regression inspects the actual position_ids passed into rotary_emb, so the concern I raised is resolved. One separate merge blocker remains in CI: Check repository consistency is failing because this Llama modeling change has not been propagated to the generated derivative model files (Apertus, Arcee, Aria, Axk1, BitNet, Cohere, DeepSeek variants, GLM variants, Mistral4, Olmo variants, Youtu, etc.). The modular/generated-model sync needs to be run and committed before the consistency gate can pass. |
Closes #33095
When a Llama model is trained with left-padded inputs, leaving
position_idsunset currently gives every token positions based on its padded index. The resulting positions differ from the positions implied by the attention mask.This updates the training path to derive position IDs from a 2-D attention mask when there is no cache. Padded entries keep position 0, and non-padded entries use their cumulative token position. Generation and cached forwards keep the existing behavior.
The regression test compares the implicit position IDs with the equivalent explicit position IDs for a left-padded batch.
Tests: