Skip to content

Fix PPFormulaNet training-loss double-shift in encoder-decoder forward - #47495

Closed
ShamSaleem wants to merge 1 commit into
huggingface:mainfrom
ShamSaleem:fix/pp-formulanet-loss-double-shift
Closed

Fix PPFormulaNet training-loss double-shift in encoder-decoder forward#47495
ShamSaleem wants to merge 1 commit into
huggingface:mainfrom
ShamSaleem:fix/pp-formulanet-loss-double-shift

Conversation

@ShamSaleem

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #47317

PPFormulaNetForConditionalGeneration.forward() computes its loss with
self.loss_function()ForCausalLMLoss, which shifts labels left by one
(labels[..., 1:]) when shift_labels is not supplied. PPFormulaNet is an
encoder-decoder model, so its logits are already aligned with labels and
no shift is needed. The extra shift drops the last target token and trains the
model against misaligned labels.

This passes shift_labels=labels to ForCausalLMLoss, which takes the
"already-shifted" path and leaves the labels aligned with the logits.

This is the same fix already merged for Florence2 in #46898 (and Moonshine
before it). PPFormulaNet's modular model inherits from
Florence2ForConditionalGeneration but overrides forward(), so the
Florence2 fix never propagated to it — eliaghazal flagged PPFormulaNet as
likely-affected in the #47090 discussion.

Change is applied to modular_pp_formulanet.py (source of truth) and the
regenerated modeling_pp_formulanet.py.

Before submitting

Note: AI assistance was used to help locate and draft this fix.

Who can review?

@zucchini-nlp (reviewed & merged the identical Florence2 fix #46898)

PPFormulaNetForConditionalGeneration.forward() computes its loss via
self.loss_function() (ForCausalLMLoss), which left-shifts labels by one
(labels[..., 1:]). PPFormulaNet is an encoder-decoder model whose logits are
already aligned with labels, so this extra shift drops the last target token
and trains the model against misaligned labels.

Pass shift_labels=labels so ForCausalLMLoss skips its internal shift and keeps
labels aligned with logits. This is the same fix already merged for Florence2
in huggingface#46898; PPFormulaNet inherits from Florence2 but overrides forward(), so the
fix did not propagate.

Fixes huggingface#47317
@zucchini-nlp

Copy link
Copy Markdown
Member

Already being fixed on another PR, closing as duplicate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PPFormulaNet training-loss bug

2 participants