[fix bug] logits's shape different from label's shape in preprocess_logits_for_metrics

[wiserxin]

What does this PR do?

After update my tranformers 4.39.3 --> 4.41.2, training LLM with deepspeed DDP method, something wrong when it goes to my preprocess_logits_for_metrics function: logits's shape is different from label's shape.

Then i discover that in latest trainer code, before passing to preprocess_logits_for_metrics, logits are padded by function pad_across_processes but labels are not.

It seems like a bug unexpectedly introduced .

Before submitting

• This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
• Did you read the contributor guideline,
  Pull Request section?
• Was this discussed/approved via a Github issue or the forum? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes? Here are the
  documentation guidelines, and
  here are tips on formatting docstrings.
• Did you write any new necessary tests?

Who can review?

Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.

[amyeroberts]

cc @muellerzr @SunMarc

[SunMarc]

Hi @wiserxin, this bug was indeed introduced by this PR #30267. The padding of the labels happens after preprocess_logits_for_metrics, hence the issue that you are experiencing. @qubvel, is it safe to put it back where it was ?

[qubvel]

Oh, that's true. @SunMarc yes, should be safe. Alternatively, we can just move the "if" block with labels padding before the "if" block with logits.

[SunMarc]

Alternatively, we can just move the "if" block with labels padding before the "if" block with logits

Yeah, I think that's would be better for readability + it was the previous behavior ! Could you make the change @wiserxin ?

[wiserxin]

Alternatively, we can just move the "if" block with labels padding before the "if" block with logits

Yeah, I think that's would be better for readability + it was the previous behavior ! Could you make the change @wiserxin ?

sure

[SunMarc]

LGTM ! Thanks for the PR @wiserxin !

[qubvel]

@SunMarc @wiserxin let's maybe add a comment about the order of this "if" block to avoid the same bug in the future. :)

[HuggingFaceDocBuilderDev]

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.

[amyeroberts]

Thanks for fixing!

+1 on @qubvel's comment

[wiserxin]

@SunMarc @wiserxin let's maybe add a comment about the order of this "if" block to avoid the same bug in the future. :)

good idea

[muellerzr]

Thanks a bunch!