Accept processor in get_training_chat_template#5560
Accept processor in get_training_chat_template#5560qgallouedec wants to merge 20 commits intomainfrom
get_training_chat_template#5560Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa61a92ea2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 6700975. Configure here.
Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com>
albertvillanova
left a comment
There was a problem hiding this comment.
CI is red:
AttributeError: 'NoneType' object has no attribute 'chat_template'

get_training_chat_templatealready works on bothPreTrainedTokenizerandProcessorMixin. The type hint, parameter name, and docstring were misleading by saying "tokenizer" only. Call sites (e.g.GRPOTrainer,SFTTrainer) already passprocessing_class.Note
Low Risk
Mostly an API/typing/doc clarification with a backwards-compatible alias and minimal logic changes; risk is limited to edge cases around argument validation/warning behavior.
Overview
get_training_chat_templatenow takes aprocessing_class(tokenizer orProcessorMixin) and updates its docs/types accordingly, aligning the public API with existing processor-based call sites.For backward compatibility it keeps a deprecated
tokenizeralias, emitting aFutureWarning, validating that only one of the two args is passed, and improving the error message wording when patching is unsupported.Reviewed by Cursor Bugbot for commit e0fb2e1. Bugbot is set up for automated code reviews on this repo. Configure here.