we can pass shift_labels=labels into loss-fn to not sift it twice

Updated — kept self.loss_function and passed shift_labels=labels, so this avoids the second shift while preserving the existing loss path / num_items_in_batch handling.

yeah, we defi need a test. Can you make it general for all models when config.is_encoder_decoder=True, and move under test_modeling_common.py
We'd want the test to run on all existing and upcoming models, this is an "easy to miss" bug so should be tested

I prototyped the common test locally in test_modeling_common.py behind config.is_encoder_decoder. It passes Bart/MBart/LED/Pegasus and catches the same issue in Florence2, but Florence2 is already tracked in #46898 / #46897.

The current common harness also skips PPFormulaNet because its shared test inputs use decoder labels of length 1, so removing the PPFormulaNet-specific test would lose coverage. A robust common test needs non-degenerate labels and decoder length >= 2, and would be red on Florence2 until #46898 lands.

Would you prefer I keep this PR scoped to the PPFormulaNet fix + regression test and add the common test as a follow-up, or expand this PR to include the common-test design and handle the failures it exposes?

I will merge the florence today, no problem on that one, Can you push the common test and fix any other models, except florence. Lets fix it all at once that many tiny PRs

Done — pushed the common test (test_encoder_decoder_loss_no_double_shift in test_modeling_common.py) and fixed CohereASR, the only other affected model besides PPFormulaNet/Florence2. Full summary is in the top-level PR comment.

import at the begonning of file

Done — removed the local CrossEntropyLoss import after switching the model fix back to self.loss_function.

why try/except, we should be fine building config once outside the loop

Removed both try/except blocks. A class based applicability check runs first. Then prepare_config_and_inputs_for_common() runs once outside the model class loop, and the config is copied for each model class. Preparation and forward errors now fail the test normally.

doesn't really look good, I suppose you wanted to infer batch size. We can try self.model_tester.batch_size

Done. Labels now use self.model_tester.batch_size directly. The tensor based batch size inference was removed.

we can delete this block, testers with small vocab size shouldn't exit and if they do, they'll have to use bigger vocab

Done. I removed the vocabulary size skip, so small vocabulary testers no longer exit early.

also weird, why do we prepare labels and then pop to prepare again, doesn't prepare_for_class already give correct labels since Seq2Seq is a common class

I changed this to return_labels=False, so labels are no longer prepared and then replaced. Fresh labels are created once with a sequence length of at least 2, which keeps the aligned and shifted loss comparison meaningful. The tester provided decoder_input_ids and decoder_attention_mask are removed so the model exercises its target to decoder input shift path. FSMT and PPFormulaNet use their explicit model specific paths.

same, not try/except , it hides any errors

Removed. There is no try/except around the forward call anymore. Only explicit applicability skips remain, so unexpected model errors fail the test.

nit: is labels are already shifted (i.e. shift_labels), lets explicitly pass labels=None and shift_labels=labels

Done, in Moonshine and PPFormulaNet. Both now pass labels=None with the aligned targets as shift_labels.

One thing to note on the exact form. The value has to come out of kwargs first. If a caller passes its own shift_labels and the model also passes one by keyword, the call raises TypeError: transformers.loss.loss_utils.ForCausalLMLoss() got multiple values for keyword argument 'shift_labels'. In the previous revision of this PR the pop also sat after the inner model call, so a caller supplied value leaked down into it. Both are handled by popping at the top of forward.

Both modulars updated and the three modeling files regenerated.

not following here, are we adding a new test for only Moonshine? can you explain why only this model?

Folded into the existing test_training_loss_no_double_shift instead of a second test. It now runs over the plain and the padded labels.

It is Moonshine only because Moonshine is the one model here whose alignment was already correct on current main, after #46784. It computed the loss with a plain nn.CrossEntropyLoss, which ignores num_items_in_batch, so it returned the same value for any count. The generic alignment test cannot see that, and the Trainer fix does not help either: the Trainer passes a count, but the count has no effect while the model computes a plain mean. Everything else in this PR was already on self.loss_function.

same q here, this one tests num_items_in_batch with trainer. Why we test differently each model?

Removed, with the unused Trainer, TrainingArguments and tempfile imports. That file matches main again.

You are right that it was a third version of the same check. Trainer counting is not specific to PPFormulaNet and is covered once on Bart in test_num_items_in_batch_encoder_decoder. Alignment is covered by the common test.

One assertion in it was not a duplicate, so I moved it into the Moonshine loss test rather than lose it: that a caller supplied shift_labels is used as given, which is the reason for the pop above.

can you skip directly in DIa/ProphetNet testers with @unittest.skip

Done. Both are @unittest.skip overrides of the test in DiaModelTest and ProphetNetModelTest, and both model_type checks are out of the common test.

not really following here as well, can you explain? TBH not a fan of adding too many conditions in a test for each model type

All of them are gone. There is no model_type branch left in the test.

They came from letting each model build its own decoder_input_ids, which meant the test had to know how each one derives them, plus an initializer_factor reset because the T5 testers make the logits numerically uniform and the aligned and shifted loss values stop being distinguishable.

The test now builds the right shifted decoder inputs itself and passes them in, so every model runs the same path. The first token is the model's top level decoder_start_token_id when that is set and inside the vocabulary, and 0 otherwise, which several testers need since they leave it unset. That token changes the logits, but it cannot change which cross entropy the reported loss matches, so the choice is not load bearing. The range check is defensive: it keeps an id past the tester vocabulary from failing in an embedding lookup before the assertions run.

The oracle compares gradients rather than loss values. It differentiates the reported loss, the aligned cross entropy and the shifted cross entropy with respect to output.logits and asserts the reported one matches the aligned gradient and not the shifted one. Gradients keep the target positions, so they stay decisive where the loss values collapse. On the T5 tester config the logits have std 4.2e-6, both losses come out at 4.595120 and differ by 4.8e-7 so torch.allclose cannot separate them, while the gradients differ by 1.3e-2. That is what removed the initializer_factor handling.

What the test gives up is checking how each model builds its decoder inputs. It now only checks that the loss does not shift targets that are already aligned, which is the bug it is guarding.

Mutation checked both ways. Reverting the PPFormulaNet fix fails the test, and injecting a double shift into T5ForConditionalGeneration fails it too, which the old value based oracle could only do with the initializer_factor override in place. The sweep is still 38 passed, 568 skipped.