🧸 Fix unset tokenizer pad_token

[LeonEricsson]

What does this PR do?

Fixes #3287

This quietly solves the issue where an auto initialized tokenizer lacks a padding token, by setting it to the EOS token. I considered adding a pad_token as an input parameter to GRPOTrainer but at that point you may as well initialize the tokenizer yourself and pass it to GRPOTrainer as processing_class=.

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? Please add a link
  to it if that's the case.
• Did you make sure to update the documentation with your changes?
• 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.

[LeonEricsson]

Should we warn/inform the user of this default behaviour?

[qgallouedec]

For the record, setting the pad_token when it's not set doesn't seem to me to be good practice. For more context, see #3200.
Nevertheless, I can't think of a better solution here. But as you point out, the user must be informed. The best thing to do is to add a line to the processing_class argument doc, informing that if the processing class has no pad_token, then it will be set to eos.
Let's avoid warning, as it can be the desired behaviour

[LeonEricsson]

on second thought how about matching the approach of SFTTrainer and defining it as a config parameter?

trl/trl/trainer/sft_config.py

Lines 52 to 54 in 1e61f6c

	pad_token (`int` or `None`, *optional*, defaults to `None`):
	Token used for padding. If `None`, it defaults to `processing_class.pad_token`, or if that is also `None`,
	it falls back to `processing_class.eos_token`.

this makes the default behaviour very explicit, i fear it would get drowned out in the processing_class doc.

my only gripe with the above is stuffing the config with too many parameters, how common is it for a user to want to use a custom padding token.

[qgallouedec]

on second thought how about matching the approach of SFTTrainer and defining it as a config parameter?

Yes, that's an option as well. But this one would require to change the way we pad the inputs. Currently we rely on the tokenizer for padding.

my only gripe with the above is stuffing the config with too many parameters, how common is it for a user to want to use a custom padding token.

Hardly never I would guess.

I'm good with both options

[LeonEricsson]

Chose to document the default behavior of pad_token = eos_token in the processing_class docstring. As you mentioned, the use case for a custom padding token doesn't seem common enough to justify adding a dedicated parameter.

[qgallouedec]

Suggested change

	if processing_class.pad_token is None:
	processing_class.pad_token = processing_class.eos_token
	if processing_class.pad_token is None:
	processing_class.pad_token = processing_class.eos_token

[qgallouedec]

We also want to run this when the processing class is passed, right?

[LeonEricsson]

Yes, agreed. But the more I think about it, the less I like the idea of silently setting the padding token.... even if it's documented.

[qgallouedec]

LGTM!

[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.