Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open an issue so this work is tracked and we don't forget?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short answer is that because the
chat_template
arg can also be the name of a template in the template dict, as well as a full template string, we still snifftokenizer.default_chat_template
even if we receive achat_template
arg, to check if the chat template matches one of the dict keys.This is definitely suboptimal, but I'll open an issue and clean it up once we can get rid of default chat templates, which have been causing other problems too!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
eek - that's not good. That also means anyone is going to get this error if they pass in a proper template. This should really be addressed at the moment, rather than waiting two months for the deprecation cycle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right - I was banking on that being an uncommon path, but I'll try to make a proper fix in this PR!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@amyeroberts should be fixed now! I moved the warnings out of the default templates and into the
apply_chat_template
logic itself, so it should now only fire when appropriate. This means theTODO
is no longer necessary either.