Skip to content

Fix Llama left-padding position IDs during training - #48946

Closed
lindicaphxag-tech wants to merge 2 commits into
huggingface:mainfrom
lindicaphxag-tech:contrib/llama-left-padding-position-ids-33095
Closed

lindicaphxag-tech wants to merge 2 commits into
huggingface:mainfrom
lindicaphxag-tech:contrib/llama-left-padding-position-ids-33095

Conversation

@lindicaphxag-tech

@lindicaphxag-tech lindicaphxag-tech commented Sep 19, 2026

Copy link
Copy Markdown

CPU CI GPU run-slow

Closes #33095

When a Llama model is trained with left-padded inputs, leaving position_ids unset 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:

PYTHONPATH=src python -m pytest tests/models/llama/test_modeling_llama.py -k left_padding_position_ids --maxfail=1 -q
1 passed

@sylvesterkaczmarek sylvesterkaczmarek left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@github-actions

Copy link
Copy Markdown
Contributor

[For maintainers] Suggested jobs to run (before merge)

run-slow: llama

@github-actions

Copy link
Copy Markdown
Contributor

CI recap

Dashboard: View test results in Grafana
Latest run: 35415856799:2
Result: failure | Jobs: 16 | Tests: 190,592 | Failures: 1 | Duration: 17h 17m

@lindicaphxag-tech

Copy link
Copy Markdown
Author

I updated the condition in 7dadbd2 to handle the empty DynamicCache created by the normal training call (get_seq_length() == 0). The test now spies on the rotary embedding call and compares the actual position IDs with the left-padding-aware values, so it fails if the new branch is not used. The focused test passes locally.

@sylvesterkaczmarek

Copy link
Copy Markdown

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

llama3 position_ids error with left padding

3 participants