Have seq2seq just use gather
Author: muellerzr
Pull request: yes

# What does this PR do?

In the case of using `Seq2Seq`, we don't want `gather_for_metrics` to use its magic and we just want to do `.gather()` (since otherwise it will drop samples in the former case as accelerate will drop "duplicates" based on the batch size, which leads to a bug).

This PR sets a new `gather_function` in the `Trainer` which by default is `gather_for_metrics`, but if a particular `Trainer` needs to modify it (such as `Seq2SeqTrainer`), then it can be specified. 

Fixes https://github.com/huggingface/transformers/issues/25231


## 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#start-contributing-pull-requests),
      Pull Request section?
- [ ] Was this discussed/approved via a Github issue or the [forum](https://discuss.huggingface.co/)? 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](https://github.com/huggingface/transformers/tree/main/docs), and
      [here are tips on formatting docstrings](https://github.com/huggingface/transformers/tree/main/docs#writing-source-documentation).
- [ ] 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.

@ArthurZucker @younesbelkada


Comments:
_The documentation is not available anymore as the PR was closed or merged._


@ArthurZucker any thoughts on what could fix this? I saw negligible time differences between main and my branch when running locally on CPU (~72s)


Looks like it's all passing now!

@amyeroberts:

>     Am I right in understanding this should only be applied to cases when evaluating generations from seq2seq models and the generation config specifies num_return_sequences > 1?
Correct, otherwise we will drop samples. Technically we can avoid this entirely I think by just using `gather`, and the test seems to show that will indeed work fine. As a result, I'll simplify this to just use `.gather`()

>     What happens and what should happen if I call evaluate with a generation config with num_return_sequences > 1 and then call a second time with num_return_sequences==1?

Per your recommendation of the test, I tried this, and it worked as it should (because `gather_for_metrics` doesn't do anything for a bs of 1 really).



@amyeroberts:

> Why does using gather_metrics drop samples?

It's some logic in Accelerate, `gather_for_metrics` will drop samples if we think they've been duplicated (such as filling in the last batch of data if data has been duplicated to make DDP efficient), however here is an edge case where just using `gather` is better

> Is this only true for Seq2SeqTrainer. If not, why not just use gather everywhere?

Yes, just Seq2Seq. Otherwise `gather_for_metrics` should be used always

Hello @muellerzr , I think `.gather` will cause the last batch containing duplicated samples, and it will eventually lead to take these  duplicated samples into computing the in `compute_metrics` , which is unexpected.
![image](https://github.com/huggingface/transformers/assets/62004255/7edb68ff-2d5e-4902-8700-f94c6a87e442)