group_by_length in Seq2SeqTrainer
Author: maximzubkov
Pull request: no

### System Info

Hey, huggingface team!

- `transformers` version: 4.24.0
- Platform: Linux-5.4.0-132-generic-x86_64-with-glibc2.31
- Python version: 3.9.16
- Huggingface_hub version: 0.11.1
- PyTorch version (GPU?): 1.11.0+cu113 (True)
- Tensorflow version (GPU?): not installed (NA)
- Flax version (CPU?/GPU?/TPU?): not installed (NA)
- Jax version: not installed
- JaxLib version: not installed
- Using GPU in script?: Yes, but error is does not depend on it
- Using distributed or parallel set-up in script?: Yes, but error is does not depend on it

### Who can help?

@ArthurZucker @younesbelkada @sg

### Information

- [ ] The official example scripts
- [X] My own modified scripts

### Tasks

- [ ] An officially supported task in the `examples` folder (such as GLUE/SQuAD, ...)
- [X] My own task or dataset (give details below)

### Reproduction

I was working on a project and tried `group_by_length` in [Seq2SeqTrainer](https://huggingface.co/docs/transformers/main_classes/trainer#transformers.Seq2SeqTrainer) and observed spikes in the loss function. I attribute it to the implementation of the `get_length_grouped_indices` function. This function groups samples in `megabatches` of size `mega_batch_mult` and reorders samples w.r.t. to length inside of the `megabatche`. Here is an example of why it may not be the right way to do that:

```
import numpy as np
import torch
from transformers.trainer_pt_utils import get_length_grouped_indices

lengths = np.random.permutation(list(range(20))).tolist()
batch_size = 2

ids = get_length_grouped_indices(lengths=lengths, mega_batch_mult=3, batch_size=batch_size)
[lengths[i] for i in ids]
```
The output is like
```
[19, 14, 11, 10, 4, 2, 17, 13, 12, 8, 7, 5, 18, 16, 9, 6, 1, 0, 15, 3]
```
And after sequential batching:
```
batches = [
    [19, 14],
    [11, 10], 
    [4, 2],
    [17, 13], 
    [12, 8], 
    [7, 5],
    [18, 16], 
    [9, 6], 
    [1, 0],
    [15, 3]
]
```
So after tokenization, we will have a very spiky `max_length` of the batch. In the example: `[19, 11, 4, 17, 12, 7, 18, 9, 1, 15]`. On a bigger scale, it will result in spikes in the loss function (`mega_batch_mult` has a default value of 50, so `max_length` of batch gradually decreases for 50 steps, and so the spike happens every 50 steps, example in the comments)  

### Expected behavior

Maybe one more additional shuffling inside of the `megabatch` is required

Comments:
![image](https://user-images.githubusercontent.com/47659865/208266592-1338018c-cec5-42fb-a90b-f605f792225d.jpeg)

Spiking loss example 

I am not too sure where the bugs lies here. The training loss with samples of varying length will always be noisy. If we had a shuffle you will see random spikes instead of regular ones, but it won't make this noisy behavior disappear.

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the [contributing guidelines](https://github.com/huggingface/transformers/blob/main/CONTRIBUTING.md) are likely to be ignored.

I met the same issue. I think we should reopen this issue