-
Notifications
You must be signed in to change notification settings - Fork 14.7k
common : convert string contents to arrays if template requires typed content #19156
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
base: master
Are you sure you want to change the base?
Conversation
… content Signed-off-by: Weizhao Ouyang <[email protected]>
ngxson
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.
This may not be a good fix. Formatting can fail for multiple reasons, so assuming it fails due to typed content will break other templates.
Instead, we should have a system to enable a cap, then retry the formatting to verify if it actually works. I'll have a look on that later
|
Yeah, this is a quick fix that I've validated locally, we should be cautious about template formatting. Feel free to modify my patch. |
|
Funnily enough it's not illegal to pass a string to a for loop in Edit: Reasonable as in every single character of the string, which hopefully no chat template will use/expect to work. |
|
Yes, I think the current error message is reasonable. I'm just thinking about how to make llama-cli more flexible (and stable) in accepting template, rather than attempting to update the original template. |
|
It's a templating problem. I'm working on it in the autoparser branch. Basically, for some templates we need to vary whether we treat an unquoted value in a field as a string or as a JSON object based on the schema for the tool call, i.e. is a string when baz is typed as a string and should be a JSON array (not a string "["foo"]") when it's typed as an array. |
|
Oh, sorry, I misread, this is about converting content, not tool call args. Well, the general problem is the same: we need to be able to determine which one we should parse. The key thing is when streaming, you cannot just "change your mind" at some point because the delta will be invalid, so you have to actually know in advance whether the content you'll be getting is an array or a string. |
I encountered an error while testing #18825 (on the latest master branch):
The original chat template is at https://huggingface.co/PaddlePaddle/PaddleOCR-VL/blob/main/chat_template.jinja
I use git bisect to find this commit (6df686b) introduced a change which cleaned up the chatml fallback and it is the first bad commit. I think the root cause is that jinja templates will fail when they receive plain strings during the content array indexing (for llama-cli case). So this PR will detect templates that expect typed/array-style message content and convert string contents into a typed-content array if requires_typed_content.