Refactor default chat template warnings #30551
Conversation
…erly deprecate default chat templates
There was a problem hiding this comment.
Thanks for tackling this!
Could you clarify the path that triggers this? The idefics2 processor passes a chat template directly when calling the tokenizer, and there isn't a warning for its own default chat template so I'm surprised the tokenizer's warning is triggered at all
| conversation, chat_template=chat_template, tokenize=tokenize, **kwargs | ||
| ) | ||
| with warnings.catch_warnings(): | ||
| # TODO Matt: This is a workaround to avoid annoying warnings until we properly remove default |
There was a problem hiding this comment.
Can you open an issue so this work is tracked and we don't forget?
There was a problem hiding this comment.
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 sniff tokenizer.default_chat_template even if we receive a chat_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.
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.
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.
@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 the TODO is no longer necessary either.
|
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. |
Rocketknight1
changed the title
| using_default_dict = False | ||
| else: | ||
| template_dict = self.default_chat_template | ||
| using_default_dict = True |
There was a problem hiding this comment.
Is there ever a case when template_dict = self.default_chat_template and we're not setting using_default_template = True? i.e. do we need this extra flag?
There was a problem hiding this comment.
Yes - this can happen if:
self.chat_templateisNoneself.default_chat_templateis adict- The user passes a full chat template to the chat template arg, rather than a key to the default dict
There was a problem hiding this comment.
(I also find thinking through the maze of conditionals here annoying, but rest assured it's all going away the moment I can deprecate default templates!)
There was a problem hiding this comment.
Gotcha - thanks for being patient with me 😅
* Temporarily silence warnings in apply_chat_template until we can properly deprecate default chat templates * make fixup * Move the default chat template warning into apply_chat_template itself * make fixup
The Idefics2Processor applies a chat template using its tokenizer, but there's an annoying interaction between the way it calls
tokenizer.apply_chat_template()and the way we trigger default chat template warnings, which means that even when we correctly add a chat template toprocessor_config.json, the default chat template warning is still thrown. The reason is that the tokenizer method still sniffsself.default_chat_tokenizerifself.chat_templateis not set (which it isn't, because the template is set on the parent processor instead), which results in the warning being thrown.Rather than doing a refactor of the method to fix this now, I'm going to temporarily silence the warnings for one or two more versions to avoid bad UX, and then once we properly remove default chat templates I can restore
IdeficsProcessor2, and properly refactor the tokenizerapply_chat_templatecode as well, and all of this should be resolved!I've also opened PRs to add proper chat templates to the IDEFICS2 repos.