-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Use jinja template for chat formatting (#730) #744
Conversation
@mishig25 this should be good to go! am not using it in the prod config because most of the templates fail, but tested locally with some other models and it should work well. |
async function getChatPromptRender( | ||
m: z.infer<typeof modelConfig> | ||
): Promise<ReturnType<typeof compileTemplate<ChatTemplateInput>>> { | ||
if (m.chatPromptTemplate) { |
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.
shouldnt the order of fallback be the other way around? First, try to use transfomersjs, then try to use chatPromptTemplate
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 downside of doing it this way is that for example in prod, we specify the tokenizer for token counting but we actually want to override the chat template with our own template.
I think in terms of specificity it makes sense to be custom chat template > tokenizer "default" template
wdyt?
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.
yep sounds good. And if we want to change the order, let's do it in follow-up PR. Also, if we want to change the order, we would need to handle #744 (comment) first
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.
yes agreed, overall we should push towards using tokenizers rather than chat prompt templates, but we will need to handle the edge cases before indeed 😁
src/lib/server/models.ts
Outdated
]; | ||
} | ||
|
||
const output = tokenizer.apply_chat_template(formattedMessages, { |
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.
right now, "system" role would case error on prompts that does not naturally support "system" right ?
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.
yep, I tried looking into it, but because the error messages are hardcoded in the jinja template, it's not obvious to me how to determine if the template is failing bc of a lack of system prompt or for some other reason
I guess you could retry without the "system"
message and see if it builds the prompt then? we could do it in a second PR if that sounds good to you ?
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 !
So I pulled the latest changes and was met with the exciting greeting I debugged my way to this PR introducing the error https://github.com/huggingface/chat-ui/blame/a9c711026b13672328ce93abc3865f1823524b0d/src/lib/server/models.ts#L133. What is the migration path for this breaking change? It would be nice to have such ones documented in a dedicated file. It would save a lot of time us all down the road. ✌️ |
I just ran into this as well, I'm not sure I understand why the default prompt template was removed? It seems like keeping it would remove the need for a breaking change. |
Hey, apologies for the breaking change, fixed it in #985 My reasoning was that we want people to use either a I've set a default that should be sane (ChatML) which should fix the issue, and in the future I want to add a Seems like most people are moving away from text completion APIs and moving towards chat completion anyway 👀 |
Hey @nsarrazin |
Yes it should help! As long as your model has a |
…#744) * Use jinja template for chat formatting * Add support for transformers js chat template * update to latest transformers version * Make sure to `add_generation_prompt` * unindent
Upgrade transformers version and fix the types in transformersjs.ts
Got rid of legacy parameters
userMessageToken
,userMessageEndToken
,assistantMessageToken
,assistantMessageEndToken
so this is a breaking change but we've been pushing for usingchatPromptTemplate
for a while so I feel like it makes sense.We support both
chatPromptTemplate
and just specifying thetokenizer
. Priority goeschatPromptTemplate > tokenizer
(more specific)Did not change the templates in prod yet because they don't work well with system prompts
(#730)