Fix double-shifted training loss in GitForCausalLM by Yigtwxx · Pull Request #47395 · huggingface/transformers

Yigtwxx · 2026-07-18T08:05:05Z

What does this PR do?

GitForCausalLM.forward shifts the labels manually and then passes them positionally to self.loss_function, so they bind to labels and shift_labels stays None. ForCausalLMLoss (loss_utils.py:61-64) then shifts a second time, and position t is trained to predict token t + 2.

Because the manual shift flattens the labels to 1-D before the call, the internal pad-and-slice keeps the shape consistent (N → N+1 → N), so nothing raises — and each batch row's final target is silently pulled in from the next row.

This regressed in #35875, which swapped CrossEntropyLoss (no shift) for self.loss_function (shifts) while leaving the manual shift in place. First released in v4.49.0. Same mechanism as the Moonshine fix in #46784.

Unlike Moonshine, GIT cannot just drop the manual shift: logits carries num_image_tokens leading image positions that must be sliced off regardless. So this PR passes shift_labels explicitly to suppress the internal shift, following the convention already used by csm (modeling_csm.py:619-621), and keeps the tensors 2-D since ForCausalLMLoss flattens them itself — which also removes the cross-row leak.

Verified with the repro from the issue (CPU, no pretrained weights). Before:

model loss            : 4.584834575653076
aligned CE (expected) : 4.646134853363037
matches aligned       : False
matches double-shift  : True
row 0 correct targets : [28, 71, 8, 58, 61, 15]
row 0 actual targets  : [71, 8, 58, 61, 15, 19]
row 1 labels          : [19, 42, 30, 58, 94, 67]

After:

model loss            : 4.646134853363037
aligned CE (expected) : 4.646134853363037
matches aligned       : True
matches double-shift  : False

Also checked that the num_items_in_batch path used by Trainer under gradient accumulation still matches the mean reduction when all targets are valid.

Per review, no model-level regression test is added here — the common test coming from #46903 is expected to cover this. The change is source-only.

$ pytest tests/models/git/
189 passed, 297 skipped, 2208 subtests passed

ruff check and ruff format --check (pinned 0.14.10) are clean. No # Copied from blocks reference this code and GIT has no modular file, so nothing needs regenerating.

Fixes #47394

Disclosure: this fix was implemented with the assistance of a code agent, per my proposal in the linked issue, and reviewed and validated by me.

Before submitting

This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
Did you read the contributor guideline and the
Pull Request checks?
Was this discussed/approved via a Github issue? GitForCausalLM double-shifts the training loss and leaks targets across batch rows (regression from #35875) #47394
Did you make sure to update the documentation with your changes according to the guidelines?
Did you write any new necessary tests?

Who can review?

@zucchini-nlp

Sanjays2402 · 2026-07-19T07:24:15Z

            loss = self.loss_function(
-                shifted_logits.view(-1, self.config.vocab_size),
-                labels.view(-1),
+                logits=shifted_logits,


since forward now pre-shifts and passes shift_labels with labels=None, the shift_labels[..., 1:] step inside ForCausalLMLoss is skipped, which is the fix. but the manual shifted_logits[:, :-1] + labels[:, 1:] pattern here predates that helper. worth checking Git-derived models (e.g. GitForSequenceClassification path or any copy) for the same double-shift, or is git the only one hitting it?

Good question. I checked, and git is the only model hitting this specific bug.

Git-derived models: there are none. modeling_git.py defines only GitVisionModel, GitModel and GitForCausalLM — there is no GitForSequenceClassification, and no modular_*.py file imports from models/git, so there is no copy to keep in sync.

The manual pattern elsewhere is safe. Models that still do shift_logits = logits[..., :-1, :] / shift_labels = labels[..., 1:] (gpt2, imagegpt, openai, blip_2, clvp, mamba, falcon_mamba, xlstm, gemma3, llama4, qwen2_audio, granite_speech, modernbert_decoder, ...) pass the result to a plain nn.CrossEntropyLoss / fixed_cross_entropy, not to self.loss_function. ForCausalLMLoss is never involved, so there is no second shift. Those are pre-loss_function leftovers, not bugs.

One other model does hit it: moshi. MoshiForCausalLM.forward (src/transformers/models/moshi/modeling_moshi.py#L1003-L1014) pre-shifts, flattens, and then calls self.loss_function(shift_logits, shift_labels, vocab_size=...) positionally — shift_labels= is never passed as a keyword, so ForCausalLMLoss pads and shifts again (loss/loss_utils.py#L61-L64). Because the tensors are already flattened, the shapes stay valid and it fails silently, exactly like git did.

I left that out of this PR to keep the diff scoped to git; happy to open a separate PR for moshi (or fold it in here if a maintainer prefers).

zucchini-nlp

I'd prefer to keep a sinlge PR to fix all models and add a proper tests. A contrib was working on it in #46903 (will nudge them again) so let's close this PR

Fixing and reviewing the same issue in all models one-by-one takes too much time for maintainers and for contribs

Yigtwxx · 2026-07-20T18:28:46Z

Thanks for the review, and understood on wanting a single consolidated PR — that makes sense from a maintenance standpoint.

One thing worth flagging before this gets folded into #46903: the two cases have different root paths, and I don't think #46903 as currently written would cover GIT.

#46903 targets encoder-decoder models, where decoder_input_ids is built via shift_tokens_right so the logits are already aligned with labels. Its regression test (test_encoder_decoder_loss_no_double_shift) is gated on config.is_encoder_decoder, so it skips GIT entirely.

GIT is decoder-only. The double shift here comes from GitForCausalLM.forward shifting manually — which it has to do anyway, because logits carries num_image_tokens leading image positions that must be sliced off — and then passing the result positionally, so it binds to labels and ForCausalLMLoss shifts again. Dropping the manual shift isn't an option the way it was for Moonshine (#46784); the fix is passing shift_labels explicitly.

So a genuinely model-agnostic PR would need to cover both the encoder-decoder path and the decoder-only-with-prefix-tokens path, with a test that isn't gated on is_encoder_decoder.

Happy to go either way:

close this and port the GIT case + an ungated test into Fix loss alignment and Trainer token counting for encoder decoder models #46903 (coordinating with @OmkumarSolanki), or
leave this open if you'd rather keep the decoder-only variant separate.

Just let me know which you prefer and I'll act on it.

zucchini-nlp · 2026-07-21T11:09:13Z

Oh sorry, indeed the issue here is that we passed manually shifted labels as raw labels. Seems to be rather a miss when converging into unified self.loss since GIT is special of itself

Can you delete the test, I dont think it is needed at model level. The common test for encoder-decoder models from another PR should be fine imo, and let's merge

forward already shifts logits and labels by one before calling self.loss_function, but ForCausalLMLoss shifts again when shift_labels is not supplied, so position t was trained to predict token t + 2. The labels were also flattened before the call, so the internal pad-and-slice kept the shape valid and pulled each batch row's final target in from the next row instead of raising. Pass shift_labels explicitly and keep the tensors 2-D, following the convention already used by csm. Co-Authored-By: Claude Opus 4.8 <[email protected]>

Yigtwxx · 2026-07-21T14:33:17Z

Done — the model-level test is removed, so tests/models/git/test_modeling_git.py is now identical to main and the PR is source-only (4 insertions, 3 deletions in modeling_git.py). Updated the description accordingly.

Locally: pytest tests/models/git/ → 189 passed, 297 skipped, 2208 subtests passed; ruff check and ruff format --check clean.

Ready to merge from my side. Note that the common test in #46903 is currently gated on config.is_encoder_decoder, so it won't exercise GIT as written — worth ungating it there so this path stays covered.

github-actions · 2026-07-21T14:34:08Z

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

run-slow: git

github-actions · 2026-07-21T14:37:43Z

CI recap

Dashboard: View test results in Grafana
Latest run: 29637186287:2
Result: success | Jobs: 5 | Tests: 485 | Failures: 0 | Duration: 2m 33s

HuggingFaceDocBuilderDev · 2026-07-21T15:39:16Z

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.

* upstream/main: (39 commits) Remove deprecated training args and `is_fast` property (huggingface#46917) Consistent output shape from `get_image_features` (huggingface#46405) Fix multi-device mxfp4 dequantization race in `_convert_moe_packed_tensors` (huggingface#47423) fix failed test cases for qwen3_omni_moe model (huggingface#47449) Fix Hunyuan-VL PIL image resize parity with reference preprocessing (huggingface#47233) Move `value` padding into the attention interfaces that need it (huggingface#47451) Simplify function dispatch for linear attention (huggingface#47450) [cache] Allow sliding window layers to be roll-backed for speculative decoding (huggingface#47447) Fix double-shifted training loss in GitForCausalLM (huggingface#47395) Fix CohereASR training-loss double-shift (same as Moonshine fix huggingface#46784) (huggingface#46895) Warn when `group_by_length` is silently ignored for iterable datasets (huggingface#47379) Update bug report list (huggingface#46607) Fix shape mismatch in KyutaiSpeechToText `generate()` last window (huggingface#46952) Optimize flash attention max seqlen computation in vision attention (huggingface#47170) fix: remove unreachable return in special token builder (huggingface#47420) Add Harry to slow CI (huggingface#47454) BLT: vectorize patch length processing (huggingface#47385) Fix `TrackioCallback` fails to log evaluation metrics after training ends (huggingface#46935) [Kimi] add integration tests (huggingface#47383) Fix typo in `MusicgenForCausalLM.generate()` (huggingface#46974) ...

Yigtwxx force-pushed the fix-git-causal-lm-double-shift-loss branch from fa9fc1f to 9456864 Compare July 18, 2026 08:16

Sanjays2402 reviewed Jul 19, 2026

View reviewed changes

zucchini-nlp reviewed Jul 20, 2026

View reviewed changes

Yigtwxx force-pushed the fix-git-causal-lm-double-shift-loss branch from 9456864 to dde8e72 Compare July 21, 2026 14:32

zucchini-nlp approved these changes Jul 21, 2026

View reviewed changes

zucchini-nlp enabled auto-merge July 21, 2026 15:27

zucchini-nlp added this pull request to the merge queue Jul 21, 2026

Merged via the queue into huggingface:main with commit 9ed46fb Jul 21, 2026
36 checks passed

Yigtwxx deleted the fix-git-causal-lm-double-shift-loss branch July 21, 2026 20:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix double-shifted training loss in GitForCausalLM#47395

Fix double-shifted training loss in GitForCausalLM#47395
zucchini-nlp merged 1 commit into
huggingface:mainfrom
Yigtwxx:fix-git-causal-lm-double-shift-loss

Yigtwxx commented Jul 18, 2026 •

edited

Loading

Uh oh!

Sanjays2402 Jul 19, 2026

Uh oh!

Yigtwxx Jul 19, 2026

Uh oh!

zucchini-nlp left a comment

Uh oh!

Yigtwxx commented Jul 20, 2026

Uh oh!

zucchini-nlp commented Jul 21, 2026

Uh oh!

Yigtwxx commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

Uh oh!

HuggingFaceDocBuilderDev commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Yigtwxx commented Jul 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Before submitting

Who can review?

Uh oh!

Sanjays2402 Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

Yigtwxx Jul 19, 2026

Choose a reason for hiding this comment

Uh oh!

zucchini-nlp left a comment

Choose a reason for hiding this comment

Uh oh!

Yigtwxx commented Jul 20, 2026

Uh oh!

zucchini-nlp commented Jul 21, 2026

Uh oh!

Yigtwxx commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

Uh oh!

github-actions Bot commented Jul 21, 2026

CI recap

Uh oh!

Uh oh!

HuggingFaceDocBuilderDev commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Yigtwxx commented Jul 18, 2026 •

edited

Loading