This is my first PR, so I'm not too familiar with this process, but there's a natural consequence that this would outdate a lot of documentation. Should I go ahead and update all previous mentions of group_by_length?

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.

@jiosephlee @SunMarc
Should we instead make the argument to accept a sampler function in addition to a strategy? That will enable users integrate their own samplers. WDYT?

@romitjain I agree that this will make the Trainer more flexible. There are, however, general hesitations to adding arguments and bloating Trainer even further.

Thoughts on adding an additional argument called train_sampler_fn that overrides the strategy when provided? @SunMarc? Also, all the docs that referenced group_by_length have been updated to use the new arg

View the CircleCI Test Summary for this PR:

https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=43138&sha=6d44c2

@SunMarc should I merge?

@ArthurZucker I would appreciate your opinion on this
#43138 (comment)

Pls merge @ArthurZucker

@romitjain I agree that this will make the Trainer more flexible. There are, however, general hesitations to adding arguments and bloating Trainer even further.

Thoughts on adding an additional argument called train_sampler_fn that overrides the strategy when provided? @SunMarc? Also, all the docs that referenced group_by_length have been updated to use the new arg

Maybe in the future when users request this ! They can also overwrite the _get_train_sampler directly !

plz merge @ArthurZucker

@ArthurZucker should be ready to merge again

@SunMarc Did we not want a deprecation period/notice on removing this from TraningArgs?

Do you think this is a widely used args ? Happy to add it back otherwise. This PR was supposed to be merged before v5 but it got forgotten.