Skip to content

fix: adapt Whisper models to use self.loss_function with num_items_in_batch support - #46068

Closed
Ashutosh-177 wants to merge 4 commits into
huggingface:mainfrom
Ashutosh-177:fix/whisper-loss-function-36119
Closed

fix: adapt Whisper models to use self.loss_function with num_items_in_batch support#46068
Ashutosh-177 wants to merge 4 commits into
huggingface:mainfrom
Ashutosh-177:fix/whisper-loss-function-36119

Conversation

@Ashutosh-177

@Ashutosh-177 Ashutosh-177 commented May 19, 2026

Copy link
Copy Markdown

Summary

  • Fixes gradient accumulation for Whisper by replacing the manual CrossEntropyLoss instantiation with self.loss_function, which supports the num_items_in_batch parameter introduced in Fix Gradient Accumulation issue #34191.
  • Adds an explicit num_items_in_batch parameter to both WhisperForConditionalGeneration.forward() and WhisperForCausalLM.forward().
  • Sets self.loss_type = "ForMaskedLM" in both Whisper class __init__ methods so the correct (no-shift) loss is used without touching the global LOSS_MAPPING.

Changes

src/transformers/models/whisper/modeling_whisper.py

  • WhisperForConditionalGeneration.__init__: sets self.loss_type = "ForMaskedLM". Encoder-decoder models receive labels already aligned with decoder logits — no token shifting is needed.
  • WhisperForConditionalGeneration.forward(): adds num_items_in_batch parameter; replaces manual CrossEntropyLoss block with self.loss_function(...).
  • WhisperForCausalLM.__init__: same self.loss_type = "ForMaskedLM" override, preserving the original no-shift behavior.
  • WhisperForCausalLM.forward(): same treatment.

src/transformers/loss/loss_utils.py

  • No global mapping changes — LOSS_MAPPING["ForConditionalGeneration"] remains ForCausalLMLoss to avoid affecting decoder-only models like LlavaForConditionalGeneration and PaliGemmaForConditionalGeneration that rely on label shifting.

Behavior

  • When num_items_in_batch=None (default): loss is identical to the previous CrossEntropyLoss(reduction="mean") call — no behavior change.
  • When num_items_in_batch is provided by the Trainer: loss is normalized correctly across gradient accumulation steps, fixing the GA bug.

Test plan

  • Verify loss is unchanged when num_items_in_batch=None
  • Verify loss is correctly normalized when num_items_in_batch is supplied
  • Run existing Whisper tests: pytest tests/models/whisper/ -x

Closes #36119

Fixes gradient accumulation for Whisper by replacing the manual
CrossEntropyLoss instantiation with self.loss_function, which supports
the num_items_in_batch parameter introduced in PR huggingface#34191.

Key changes:
- loss_utils.py: ForConditionalGeneration now maps to ForMaskedLMLoss
  instead of ForCausalLMLoss. Encoder-decoder models have labels already
  aligned with logits (no token shift needed), matching the masked-LM
  loss pattern.
- modeling_whisper.py: WhisperForConditionalGeneration and
  WhisperForCausalLM both gain an explicit num_items_in_batch parameter
  and delegate loss computation to self.loss_function.

Closes huggingface#36119
Copilot AI review requested due to automatic review settings May 19, 2026 06:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates Whisper’s forward() implementations to use the unified self.loss_function(...) interface (including num_items_in_batch support) to fix gradient-accumulation loss scaling, and adjusts the global loss dispatch for ForConditionalGeneration.

Changes:

  • Add num_items_in_batch to WhisperForConditionalGeneration.forward() and WhisperForCausalLM.forward() and route loss computation through self.loss_function(...).
  • Update LOSS_MAPPING["ForConditionalGeneration"] to point to ForMaskedLMLoss instead of ForCausalLMLoss.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
src/transformers/models/whisper/modeling_whisper.py Switch Whisper LM losses to self.loss_function(...) and accept num_items_in_batch for correct GA normalization.
src/transformers/loss/loss_utils.py Changes the default loss implementation used for classes matching ForConditionalGeneration.
Comments suppressed due to low confidence (1)

src/transformers/models/whisper/modeling_whisper.py:1224

  • WhisperForCausalLM previously computed loss directly against labels (no shifting). Switching to self.loss_function will use ForCausalLMLoss by default, which shifts labels internally; this is a behavior change and contradicts the PR description claim of “no behavior change when num_items_in_batch is None”. If Whisper’s decoder labels are intended to be already aligned with logits (as in many encoder-decoder decoders), consider setting self.loss_type = "ForMaskedLM" for this class or passing an explicit shift_labels to preserve current semantics.
        logits = self.proj_out(outputs[0])

        loss = None
        if labels is not None:
            loss = self.loss_function(
                logits=logits, labels=labels, vocab_size=self.config.vocab_size, num_items_in_batch=num_items_in_batch
            )

Comment thread src/transformers/loss/loss_utils.py Outdated
Comment thread src/transformers/models/whisper/modeling_whisper.py
Revert the LOSS_MAPPING["ForConditionalGeneration"] change — it
was too broad and would break decoder-only models that match the
same key (LlavaForConditionalGeneration, PaliGemmaForConditionalGeneration,
etc.) which rely on ForCausalLMLoss label-shifting behavior.

Instead, set self.loss_type = "ForMaskedLM" explicitly in both
WhisperForConditionalGeneration.__init__ and WhisperForCausalLM.__init__.
This overrides the per-instance loss resolution in PreTrainedModel
without touching the global mapping, ensuring:

- All other *ForConditionalGeneration models remain unaffected
- WhisperForConditionalGeneration and WhisperForCausalLM both use
  ForMaskedLMLoss (no label shift), matching the original
  CrossEntropyLoss behavior and preserving correctness
@Ashutosh-177
Ashutosh-177 force-pushed the fix/whisper-loss-function-36119 branch from 1e377ce to 08d7337 Compare May 19, 2026 07:04
Ashutosh-177 and others added 2 commits May 19, 2026 12:52
Both WhisperForConditionalGeneration.forward() and
WhisperForCausalLM.forward() were missing docstring entries for the
new num_items_in_batch parameter, causing check_docstrings to fail.
@github-actions

Copy link
Copy Markdown
Contributor

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

run-slow: whisper

@Ashutosh-177

Copy link
Copy Markdown
Author

Opened PR #46068 that fixes this. All CI checks are passing — would appreciate a review.

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.

Adapting Whisper to the new loss_function attribute

3 participants