Fix double-shifted training loss in GitForCausalLM - #47395
Conversation
fa9fc1f to
9456864
Compare
zucchini-nlp
left a comment
There was a problem hiding this comment.
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
|
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 GIT is decoder-only. The double shift here comes from 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 Happy to go either way:
Just let me know which you prefer and I'll act on it. |
|
Oh sorry, indeed the issue here is that we passed manually shifted labels as raw 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 <noreply@anthropic.com>
9456864 to
dde8e72
Compare
|
Done — the model-level test is removed, so Locally: Ready to merge from my side. Note that the common test in #46903 is currently gated on |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: git |
CI recapDashboard: View test results in Grafana |
|
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) ...
What does this PR do?
GitForCausalLM.forwardshifts the labels manually and then passes them positionally toself.loss_function, so they bind tolabelsandshift_labelsstaysNone.ForCausalLMLoss(loss_utils.py:61-64) then shifts a second time, and positiontis trained to predict tokent + 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) forself.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:
logitscarriesnum_image_tokensleading image positions that must be sliced off regardless. So this PR passesshift_labelsexplicitly to suppress the internal shift, following the convention already used bycsm(modeling_csm.py:619-621), and keeps the tensors 2-D sinceForCausalLMLossflattens them itself — which also removes the cross-row leak.Verified with the repro from the issue (CPU, no pretrained weights). Before:
After:
Also checked that the
num_items_in_batchpath used byTrainerunder 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.
ruff checkandruff format --check(pinned 0.14.10) are clean. No# Copied fromblocks 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
Pull Request checks?
Who can review?
@zucchini-nlp