cc @SunMarc

Hey @jiosephlee, I don't mind adding this but did you manage to do an experiment where this leads to better training overall ?

Hey @SunMarc ! Sequential sampling in and of itself wouldn't improve training, but yes, I'm doing research currently where a well-designed curriculum can lead to better performance. There are also prior works that support this e.g. https://arxiv.org/abs/2405.07490. A common way of organizing data in this field is from easy to hard: https://dl.acm.org/doi/abs/10.1145/3589335.3641257

Maybe we can first gauge the interest from the community before merging this ? TrainingArgs is already super bloated so I prefer to not add any args that have low usage. Could you create an issue so that we see if folks are interested in ?

@SunMarc, that totally makes sense. I just created an issue, and I can re-bump this PR once it's garnered some interest.

I know folks like @qgallouedec have wanted this as well, if this helps clarify the need for this feature (correct me if I'm wrong, Quentin!)

Thznks!

Yes, to give more context:

Currently, if I want to sample my data in order, I can't. The PR in the current state is a solution, but in my opinion, sequential sampling should be the default.

If we agree on this, I see two options

1. Add a parameter such as random_sampling (False by default) that would enable random sampling. But that would add another argument to TrainingArguments.

2. (Better, in my opinion) Simply replace the random sampler with a sequential sampler, and if the user wants random sampling, they can simply shuffle their dataset beforehand, which amounts to the same thing. No new arg.

In both cases it would change the default behavior of the trainer

@qgallouedec I will +1 on the default behavior change. The default behavior is likely unexpected for many, especially for those coming from PyTorch, which also has the shuffle off as default https://docs.pytorch.org/docs/stable/data.html#torch.utils.data.DataLoader

Concerning (2), there is an issue that would arise from forgoing RandomSampler. RandomSampler has a nice property where you can reset the shuffle at each epoch, and it appears to be used in Trainer.

Otherwise, it seems difficult to implement epoch-level shuffling outside of Trainer.

Currently, if I want to sample my data in order, I can't. The PR in the current state is a solution, but in my opinion, sequential sampling should be the default.

Can you explain why ? For the training dataset, it should be better to have have random sampling in general no ?

Maybe as a first set, we can create a new arg called train_sampler and we remove that the same time group_by_length arg. In this new arg, we will have three choices random (default), group_by_length and sequential wdyt ? Since v5 is not out yet, if we are quick enough, we won't have to do dep cycle regarding group_by_length arg. cc @jiosephlee

Happy new year btw ;)

@SunMarc happy new year :)

And that's a pretty clean suggestion! I took a stab at refactoring it based on what you said. I took the liberty of changing one of the ValueErrors to a warning because the IterableDataset incompatibility seems to be an issue for any Sampler, afaik, which is why samplers are entirely skipped in line 1037.

Yes, that sounds good to me too. With one small reservation about the name. Because this argument would not take a sampler, but rather a strategy. So maybe sampling_strategy?

Can you explain why ? For the training dataset, it should be better to have random sampling in general no ?

Yes indeed. The motivation was mainly to avoid the surprise caused by the fact that the PyTorch loader is sequential by default, but shuffling by default is also a good default.

Moved this to new PR #43138