Improve MiroThinker chat template compatibility with the new Jinja template engine#1404
Improve MiroThinker chat template compatibility with the new Jinja template engine#1404ikawrakow merged 4 commits intoikawrakow:mainfrom
Conversation
|
fa8893d Does this fix the crash? |
|
@firecoperana Yes |
|
My fix is mostly for user who does not need the template when using completions like text completions. It also helps in your case because template is not fully compatible. |
|
Where is this template from? It's not the original one. I'd urge you not to use the pop method as that is technically not allowed in chat templates (it just happens to work in our jinja parser (for now)). |
@CISC The original template (https://github.com/ikawrakow/ik_llama.cpp/blob/afa6439ac35c5710c05583fd05e6aa866ebd0a02/models/templates/MiroThinker.jinja) crashes due to the line I also cannot find any reference to |
Anyway, I guess you don't know who made this template? |
|
No, I am the person who made the template. |
|
You want to show us how it should be done with your own PR? Assuming of course this is possible without you being banned from the |
Not a problem, however it is tricky as this is modifying the messages object, which is not ok. You will have to make a copy to be compliant. |
@CISC llama.cpp itself does not provide any traceback even with |
|
I'm trying to keep all the modifications at the top of the chat template. It seems that MiroThinker’s fine-tuned models use different chat templates but share the same MCP-style tool calling. They preserve the system-message and user-message handling from the original chat template. This way, I can simply copy and paste the top section into each model’s template and it should just work. |
BTW, I'm not sure if you're just trolling or genuinely believe this, either way a bit strange as there is ample evidence to the contrary. |
No, I'm not trolling, and I wouldn't know what evidence you are talking about. Obviously you can prove me wrong by adding a PR here. |
@hksdpc255 for one contributed here before contributing PRs that got merged in Also, I am already a contributor here, both from previous and newly submitted code (though my attributions were stripped before merge (I don't mind BTW)). |
|
@hksdpc255 The macros are a nice cleanup, though you are still modifying the object, you can test on Chat Template Editor. |
|
@CISC The Chat Template Editor seems not stable enough to use. It always shows when I enable tools. I'm currently using https://huggingface.co/spaces/huggingfacejs/chat-template-playground for debugging. |
That would be because you are not passing
I would not use this, it is not fully compliant, and will also overlook immutability issues. |
For compatibility reasons. It can be removed when upstream fix the jinja render engine bugs.
| {%- macro function_name() %} | ||
| {%- if tool.function is defined %} | ||
| {{- tool.function.name }} | ||
| {%- elif tool.name is defined and tool.description is defined %} | ||
| {{- tool.name }} | ||
| {%- endif %} | ||
| {%- endmacro %} | ||
| {%- macro function_description() %} | ||
| {%- if tool.function is defined %} | ||
| {{- tool.function.description }} | ||
| {%- elif tool.name is defined and tool.description is defined %} | ||
| {{- tool.description }} | ||
| {%- endif %} | ||
| {%- endmacro %} | ||
| {%- macro function_parameters() %} | ||
| {%- if tool.function is defined %} | ||
| {{- tool.function.parameters }} | ||
| {%- elif tool.name is defined and tool.description is defined %} | ||
| {{- tool.parameters }} | ||
| {%- endif %} | ||
| {%- endmacro %} | ||
|
|
||
| {%- macro render_tool(server_name) %} | ||
| {%- if tool.mt_visited is not defined %} | ||
| {%- if server_name != ns.last_server %} | ||
| {{- "\n## Server name: " + server_name + "\n" }} | ||
| {%- set ns.last_server = server_name %} | ||
| {%- endif %} | ||
| {{- "### Tool name: " + function_name() + "\n" }} | ||
| {{- "Description: " + function_description() + "\n" }} | ||
| {{- "Input JSON schema: " + (function_parameters() | tojson(ensure_ascii=False)) + "\n" }} | ||
| {{- "\n" }} | ||
| {%- endif %} | ||
| {%- endmacro %} | ||
|
|
||
| {%- macro render_tool_server() %} | ||
| {%- if (function_name().split('_sandbox') | length > 1) or function_name().startswith('run_') or (function_name().split('python') | length > 1) %} | ||
| {{- "tool-python" }} | ||
| {%- elif function_name().split('_search') | length > 1 %} | ||
| {{- "search_and_scrape_webpage" }} | ||
| {%- elif function_name() == 'scrape_and_extract_info' %} |
There was a problem hiding this comment.
| {%- macro function_name() %} | |
| {%- if tool.function is defined %} | |
| {{- tool.function.name }} | |
| {%- elif tool.name is defined and tool.description is defined %} | |
| {{- tool.name }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro function_description() %} | |
| {%- if tool.function is defined %} | |
| {{- tool.function.description }} | |
| {%- elif tool.name is defined and tool.description is defined %} | |
| {{- tool.description }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro function_parameters() %} | |
| {%- if tool.function is defined %} | |
| {{- tool.function.parameters }} | |
| {%- elif tool.name is defined and tool.description is defined %} | |
| {{- tool.parameters }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro render_tool(server_name) %} | |
| {%- if tool.mt_visited is not defined %} | |
| {%- if server_name != ns.last_server %} | |
| {{- "\n## Server name: " + server_name + "\n" }} | |
| {%- set ns.last_server = server_name %} | |
| {%- endif %} | |
| {{- "### Tool name: " + function_name() + "\n" }} | |
| {{- "Description: " + function_description() + "\n" }} | |
| {{- "Input JSON schema: " + (function_parameters() | tojson(ensure_ascii=False)) + "\n" }} | |
| {{- "\n" }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro render_tool_server() %} | |
| {%- if (function_name().split('_sandbox') | length > 1) or function_name().startswith('run_') or (function_name().split('python') | length > 1) %} | |
| {{- "tool-python" }} | |
| {%- elif function_name().split('_search') | length > 1 %} | |
| {{- "search_and_scrape_webpage" }} | |
| {%- elif function_name() == 'scrape_and_extract_info' %} | |
| {%- macro function_name(tool) %} | |
| {%- if tool.function is defined %} | |
| {{- tool.function.name }} | |
| {%- elif tool.name is defined and tool.description is defined %} | |
| {{- tool.name }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro function_description(tool) %} | |
| {%- if tool.function is defined %} | |
| {{- tool.function.description }} | |
| {%- elif tool.name is defined and tool.description is defined %} | |
| {{- tool.description }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro function_parameters(tool) %} | |
| {%- if tool.function is defined %} | |
| {{- tool.function.parameters }} | |
| {%- elif tool.name is defined and tool.description is defined %} | |
| {{- tool.parameters }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro render_tool(server_name, tool) %} | |
| {%- if tool.mt_visited is not defined %} | |
| {%- if server_name != ns.last_server %} | |
| {{- "\n## Server name: " + server_name + "\n" }} | |
| {%- set ns.last_server = server_name %} | |
| {%- endif %} | |
| {{- "### Tool name: " + function_name(tool) + "\n" }} | |
| {{- "Description: " + function_description(tool) + "\n" }} | |
| {{- "Input JSON schema: " + (function_parameters(tool) | tojson(ensure_ascii=False)) + "\n" }} | |
| {{- "\n" }} | |
| {%- endif %} | |
| {%- endmacro %} | |
| {%- macro render_tool_server(tool) %} | |
| {%- if (function_name(tool).split('_sandbox') | length > 1) or function_name(tool).startswith('run_') or (function_name(tool).split('python') | length > 1) %} | |
| {{- "tool-python" }} | |
| {%- elif function_name(tool).split('_search') | length > 1 %} | |
| {{- "search_and_scrape_webpage" }} | |
| {%- elif function_name(tool) == 'scrape_and_extract_info' %} |
And you need to fix all other calls similarly.
There was a problem hiding this comment.
But why? Shouldn't a macro capture the context from where it is called?
There was a problem hiding this comment.
There was a problem hiding this comment.
Additionally, for this macro:
{%- macro render_tool(server_name) %}
{%- if tool.mt_visited is not defined %}
{%- if server_name != ns.last_server %}
{{- "\n## Server name: " + server_name + "\n" }}
{%- set ns.last_server = server_name %}
{%- endif %}
{{- "### Tool name: " + function_name() + "\n" }}
{{- "Description: " + function_description() + "\n" }}
{{- "Input JSON schema: " + (function_parameters() | tojson(ensure_ascii=False)) + "\n" }}
{{- "\n" }}
{%- endif %}
{%- endmacro %}If I changed it to
{%- macro render_tool(server_name, tool, ns) %}
{%- if tool.mt_visited is not defined %}
{%- if server_name != ns.last_server %}
{{- "\n## Server name: " + server_name + "\n" }}
{%- set ns.last_server = server_name %}
{%- endif %}
{{- "### Tool name: " + function_name() + "\n" }}
{{- "Description: " + function_description() + "\n" }}
{{- "Input JSON schema: " + (function_parameters() | tojson(ensure_ascii=False)) + "\n" }}
{{- "\n" }}
{%- endif %}
{%- endmacro %}It seems {%- set ns.last_server = server_name %} will not affect the namespace outside of the macro. At least in llama.cpp.
There was a problem hiding this comment.
ns is not within the scope of the macro (it's declared later).
There was a problem hiding this comment.
Not just passed-in BTW, all arrays and dicts/objects (except namespace) are immutable.
There was a problem hiding this comment.
Seems making it fully compatible with standard Jinja2 would require a huge refactor. I’d rather leave it as is for now unless it breaks again.
There was a problem hiding this comment.
Yes, the way this template was written makes it harder, which is why I started by asking where it came from. :)
There was a problem hiding this comment.
So why {%- set message.tool_calls = [] %} or {%- set message.tool_calls = None %} will cause crash in llama.cpp?
There was a problem hiding this comment.
Not entirely sure, will require some debugging, but both should clearly be caught by your if checks.
|
MiroThinker-compat.jinja should works for now. MiroThinker.jinja should works when upstream fix the bug. (Then we can safely remove |
|
What is the status here? Did the two of you agree? |
|
I believe the version provided in the PR is good enough, though CISC may prefer the approach discussed here: discussion_r2923730444 Both of them is compatable with llama.cpp and ik_llama.cpp(with PR #1376 merged) The main issue with the template is that it incorrectly assumes the provided |
|
The version provided in discussion_r2923730444 has fewer violations of standard Jinja2. The version in the PR is more hacky, but it is completely separated from the part I would prefer not to modify. If MiroThinker’s developers release a new version of that section, I can simply copy and paste it without needing to rework my changes. |
|
Now that has been merged, I can address your comments.
This project clearly states that it is a fork of Hence, to use your own words, it is you who is trolling here. |
Well, well, well, talk about showing your true colors... I think you need to dial down that sarcasm a little and look a bit more carefully.
Sure, it is true that I have never submitted a PR here, but that is simply because I don't use ik_llama.cpp, so it's a bit out of my scope (and free time), I do try to be helpful on topics I know about though, either because it interests me in general, or because it is code I for some strange reason are quite familiar with, hmmm... I'll repeat this again though; this "trouble" you keep talking about is a pure figment of your imagination.
You sir are quite the comedian. |
At least it will no longer crash when starting llama-server.
Jinja only changes. Safe to merge.