Fix CohereAsr training-loss double-shift (train against labels, not labels[..., 1:])
Author: eliaghazal
Pull request: yes

<!-- ci-dashboard-badge:start -->
[![CI](https://transformers-ci.lor-e.huggingface.cool/badge/pr?pr=47090)](https://transformers-ci.lor-e.huggingface.cool/d/pytest-observability-by-pr/pytest-observability-branch?var-pr=47090)
<!-- ci-dashboard-badge:end -->

# What does this PR do?

`CohereAsrForConditionalGeneration.forward` right-shifts `labels` into `decoder_input_ids` via `shift_tokens_right`, then computes the loss with `self.loss_function` (`ForCausalLMLoss`), which shifts the labels a second time internally. The model therefore trains against `labels[..., 1:]` instead of `labels`.

Verified on main (`b70d02fc72`) with a tiny random-init model:

```
model loss:            4.990426
aligned CE (correct):  4.985212
double-shift CE (bug): 4.990426
```

This is the same bug fixed for Moonshine in #46784. CohereAsr inherits from Moonshine but overrides `forward`, so that fix didn't propagate.

The fix keeps `self.loss_function` and passes the (already aligned) labels as `shift_labels`, so `ForCausalLMLoss` skips its internal shift while preserving the `num_items_in_batch` handling needed for correct loss reduction under gradient accumulation — same pattern as Florence2 after #46898. Changed in `modular_cohere_asr.py` and regenerated `modeling_cohere_asr.py` with the modular converter.

Also adds the same regression test as Moonshine's (`test_training_loss_no_double_shift`, with and without `-100` padding), extended with an assertion for the `num_items_in_batch` (summed) reduction path. It fails on current main and passes with this fix; the full `tests/models/cohere_asr` file passes (124 passed, 138 skipped).

Fixes #46894
Closes #46895, closes #46958 (earlier PRs for the same issue)

## Before submitting
- [ ] This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
- [x] Did you read the [contributor guideline](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md#create-a-pull-request), Pull Request section?
- [x] Was this discussed/approved via a Github issue or the forum? Please add a link to it if that's the case. → #46894
- [ ] Did you make sure to update the documentation with your changes?
- [x] Did you write any new necessary tests?

## Who can review?

@eustlb @vasqu


Comments:
Can we add closes #... for the other PRs so we focus on only this one? 

@vasqu good catch on the gradient accumulation — dropping `self.loss_function` did lose the `num_items_in_batch` handling. I've updated the PR (4629fa3) to keep `self.loss_function` and pass the labels as `shift_labels` instead: forward already right-shifts them into `decoder_input_ids`, so they're aligned with the logits and `ForCausalLMLoss` can skip its internal shift while still summing and dividing by `num_items_in_batch`. This is the same pattern Florence2 uses since #46898. The regression test now also covers the `num_items_in_batch` reduction path.

On fixing collectively — from a sweep of models mixing the whisper-style label shift with `ForCausalLMLoss`:

- **Florence2** — already fixed with the `shift_labels` pattern (#46898)
- **Moonshine** — fixed in #46784, but with a plain `CrossEntropyLoss`, so it has the same grad-accum gap you raised here; happy to open a follow-up PR moving it to the `shift_labels` pattern
- **PPFormulaNet** — builds `decoder_input_ids` by shifting `input_ids` while the loss shifts `labels` internally, so it looks affected too (depends on the intended labels convention); can check it in the same follow-up

I'd keep this PR scoped to CohereAsr and do the Moonshine/PPFormulaNet pass in a follow-up, unless you'd prefer it all in one.

Also added the closes-references for the earlier duplicate PRs (#46895, #46958) to the description.


Fair point 😅 — reading through #46895, the review there converged on this same `shift_labels` approach before my update did, so credit to @sharmax-vikas. No attachment to which PR lands — happy to close this one in favor of #46895 if that's the cleaner path.

Whichever one you pick, two bits from here may be worth carrying over: the regression test also asserts the `num_items_in_batch` (summed) reduction path, and the change is made in `modular_cohere_asr.py` with `modeling_cohere_asr.py` regenerated by the converter.

Either way I'll follow up on the Moonshine/PPFormulaNet sweep discussed above once this settles.


Yep, not on you. Imo it's just about coordination now and imo probably best to sweep all affected models at once

Happy to do the sweep in this PR then. I'd extend it to move Moonshine off the plain CrossEntropyLoss (it has the same num_items_in_batch gap) and check/fix PPFormulaNet, each with the same regression test including the grad accumulation reduction path. If you'd rather do it in #46895 with @sharmax-vikas driving that's fine too, just say the word and I'll share what I have.


Let's wait on @SunMarc and @eustlb first, also happy with either :hugs: expecting sometime next week to handle this properly then. 

Then maybe we can merge this https://github.com/huggingface/transformers/pull/46895 and @eliaghazal, if you are willing, maybe you can propagate this change to other models so that the have the same behavior across all models that have this isssue ? 

@SunMarc done — this PR now carries the propagation:

- **Moonshine**: #46784 fixed the double-shift but left a plain `CrossEntropyLoss`, which drops `num_items_in_batch` — so loss reduction under gradient accumulation is wrong. Moved it to `self.loss_function(..., shift_labels=labels)` like the rest.
- **PPFormulaNet**: inherits Florence2 but overrides `forward`, so it missed #46898 and still double-shifts (also reported in #47317). Same `shift_labels` fix.
- Regression tests for both exercise the grad-accumulation path with a global token count that differs from the per-step count, so the summed path is distinguishable from a plain mean.
- Audited the rest of the codebase for models that right-shift labels into `decoder_input_ids` and call `self.loss_function` without `shift_labels`: the only other hit is T5Gemma, which is fine — it uses `loss_type="ForMaskedLM"`, which doesn't shift.

Coordination so we don't trip over each other: if #46895 lands first I'll rebase and drop the CohereASR commits here; and #46903 (@OmkumarSolanki) also covers PPFormulaNet with a config-gated common test — happy to drop my PPFormulaNet half in deference, whichever way you prefer to slice it.


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

run-slow: cohere_asr, moonshine, moonshine_streaming, pp_formulanet

<!-- ci-dashboard-recap:start -->

---

### CI recap

**Dashboard:** [View test results in Grafana](https://transformers-ci.lor-e.huggingface.cool/d/pytest-observability-by-pr/pytest-observability-branch?var-pr=47090)
**Latest run:** [29611022127:2](https://github.com/huggingface/transformers/actions/runs/29611703077)
**Result:** `success` | **Jobs:** 5 | **Tests:** 867 | **Failures:** 1 | **Duration:** 4m 58s

<!-- ci-dashboard-recap:end -->