-
Notifications
You must be signed in to change notification settings - Fork 543
Explicit check for existence of chat_template #1725
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
`conversational` does not always imply chat_template. Example: https://huggingface.co/facebook/blenderbot-400M-distill/blob/main/README.md See #1722
| `model = ${info.auto_model}.from_pretrained("${model.id}"` + remote_code_snippet + ")" | ||
| ); | ||
| if (model.tags.includes("conversational")) { | ||
| if (model.tags.includes("conversational") && model.config?.tokenizer_config?.chat_template) { |
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.
I'm not sure if this is populated if i.e. chat_template.jinja exists but it's not in tokenizer_config.json
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.
(I think it's not)
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 correct way to check if a chat template is set is:
(this.config?.tokenizer_config?.chat_template !== undefined ||
this.config?.processor_config?.chat_template !== undefined ||
this.config?.chat_template_jinja !== undefined(taken from internal repo)
Note that this.config?.chat_template_jinja is the most future-proof field (but is not populated for all models with a chat template)
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.
agree^
Notably, check for jinja field.
packages/tasks/src/tokenizer-data.ts
Outdated
| export interface ProcessorConfig { | ||
| chat_template?: string | Array<{ name: string; template: string }>; | ||
| } |
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.
hmm, in moon-landing, this one is simply typed (and validated...) like this:
processor_config?: {
chat_template?: string;
};
chat_template_jinja?: string;any reason to differ here?
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.
No reason at all, I was trying to mimic what I saw in the other fields but didn't check moon-landing. Changing!
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.
^ much simpler this way, thanks!
julien-c
left a comment
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.
lgtm with my suggestion
Co-authored-by: Julien Chaumond <julien@huggingface.co>
|
Waiting in case @Wauplin has time to take a look, otherwise will merge in a day or so. |
Wauplin
left a comment
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.
Thanks for taking care of it @pcuenca!
julien-c
left a comment
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.
awesome 👍
conversationaldoes not always imply chat_template. Example: https://huggingface.co/facebook/blenderbot-400M-distill/blob/main/README.mdIn our internal codebase we auto-set
conversationalif a chat template exists, but the opposite is not always true. The model above has an explicitconversationaltag, but no chat template.See #1722