Skip to content

MiroThinker tool call parser#20624

Open
hksdpc255 wants to merge 12 commits intoggml-org:masterfrom
hksdpc255:patch-2
Open

MiroThinker tool call parser#20624
hksdpc255 wants to merge 12 commits intoggml-org:masterfrom
hksdpc255:patch-2

Conversation

@hksdpc255
Copy link
Copy Markdown
Contributor

@hksdpc255 hksdpc255 commented Mar 16, 2026

The MiroThinker series v1.0–v1.7 (and likely every version before v2.0) uses an MCP-style tool call:

<use_mcp_tool>
<server_name>{server_name}</server_name>
<tool_name>{tool_name}</tool_name>
<arguments>
{json_args}
</arguments>
...
</use_mcp_tool>

It requires the MCP server name to be included in the system prompt, which makes it impossible for the autoparser to work with it.

@hksdpc255 hksdpc255 changed the title Implement MiroThinker tool call parser MiroThinker tool call parser Mar 16, 2026
@hksdpc255 hksdpc255 marked this pull request as ready for review March 16, 2026 10:03
@hksdpc255 hksdpc255 requested a review from a team as a code owner March 16, 2026 10:03
Comment thread models/templates/MiroThinker-v1.jinja Outdated
Comment thread models/templates/MiroThinker-v1.jinja Outdated
Copy link
Copy Markdown
Member

@pwilkin pwilkin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to be any support for structured outputs.

Comment thread common/chat.cpp
@hksdpc255
Copy link
Copy Markdown
Contributor Author

There doesn't seem to be any support for structured outputs.

@pwilkin What does structured outputs means? Is there any examples to implement that?

@hksdpc255
Copy link
Copy Markdown
Contributor Author

@pwilkin Actually, I have another idea. I could further improve the chat template to recognize formatted tool names from MCP servers (e.g., mcp__<server_name>__<tool_name> used by claude-code), which would make the server name more meaningful.

However, this would require, as an example, mapping
<server_name>name_part_1</server_name>[spaces]<tool_name>name_part_2</tool_name>
to mcp__name_part_1__name_part_2.

How can I implement this kind of custom transformation using the new PEG parser?

Comment thread models/templates/MiroThinker-v1.jinja Outdated
Comment on lines +274 to +285

{#- ========== Workaround for llama.cpp crashing ========== #}
{%- for message in messages %}
{%- if message.role == "assistant" %}
{%- if message.tool_calls | length == 0 %}
{%- set fake_function = namespace(name='fake_name', arguments='{}') %}
{%- set fake_function = namespace(function=fake_function) %}
{%- set message.tool_calls = [fake_function, fake_function] %}
{%- endif %}
{%- endif %}
{%- endfor %}
{#- ========== Workaround for llama.cpp crashing ========== #}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this fixed? If not, someone should look at it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part add fake functions to assistant messages and it will prevent llama-server from crash.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The crash seems to occur here:

[&](bool success, value & messages, value & tools) {
if (!success) {
return; // Nothing can be inferred
}
auto & tool_name = tools->at(0)->at("function")->at("name");
caps_print_stats(tool_name, "tools[0].function.name");
caps_print_stats(tools, "tools");
if (!tool_name->stats.used) {
result.supports_tools = false;
}
auto & tool_calls = messages->at(1)->at("tool_calls");;
caps_print_stats(tool_calls, "messages[1].tool_calls");
if (!tool_calls->stats.used) {
result.supports_tool_calls = false;
return;
}
auto & tool_arg = tool_calls->at(0)->at("function")->at("arguments")->at("arg");
caps_print_stats(tool_arg, "messages[1].tool_calls[0].function.arguments.arg");
if (tool_arg->stats.used) {
result.supports_object_arguments = true;
}
}

It assumes that the message list passed into the chat template is immutable. However, in llama.cpp’s Jinja engine, a reference is passed to the template rather than a copy, which makes it effectively mutable.

Copy link
Copy Markdown
Member

@CISC CISC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise the template LGTM (even though it will not work in immutable sandbox; does not matter for us), remaining changes and approval up to @pwilkin

@aldehir
Copy link
Copy Markdown
Contributor

aldehir commented Mar 17, 2026

How can I implement this kind of custom transformation using the new PEG parser?

You can create a custom mapper and add another chat format. See chat-peg-parser.cpp, you can likely inherit the one there.

Definitely an interesting model...

Is it not possible to hardcode the server name to something like "localhost" for better compatibility?

@hksdpc255
Copy link
Copy Markdown
Contributor Author

hksdpc255 commented Mar 17, 2026

Definitely an interesting model...

Is it not possible to hardcode the server name to something like "localhost" for better compatibility?

Yes, that is possible. However, this model is fine-tuned to use specific server names such as tool-python, search_and_scrape_webpage, and jina_scrape_llm_summary for tasks like Python/shell execution, web search, and fetching web content.

Mapping everything to a generic server would in pratically require additional reasoning tokens during inference, and the results would not be as good as when using the original format.

@pwilkin
Copy link
Copy Markdown
Member

pwilkin commented Mar 17, 2026

How can I implement this kind of custom transformation using the new PEG parser?

You can create a custom mapper and add another chat format. See chat-peg-parser.cpp, you can likely inherit the one there.

Don't even need to create a custom mapper since for the analysis I made a tagged mapper that can be used out-of-the-box for this :)

See the parser usages in chat-diff-analyzer.cpp for examples of extracting fragments.

@pwilkin
Copy link
Copy Markdown
Member

pwilkin commented Mar 17, 2026

There doesn't seem to be any support for structured outputs.

@pwilkin What does structured outputs means? Is there any examples to implement that?

Basically this:

        if (has_response_format) {
            auto response_format = p.rule("response-format", p.content(p.schema(p.json(), "response-format-schema", inputs.json_schema)));
            return ctx.reasoning_parser + p.space() + p.choice({
                p.literal("```json") + p.space() + response_format + p.space() + p.literal("```"),
                response_format
            }) + p.end();
        }

@hksdpc255
Copy link
Copy Markdown
Contributor Author

There doesn't seem to be any support for structured outputs.

@pwilkin What does structured outputs means? Is there any examples to implement that?

Basically this:

        if (has_response_format) {
            auto response_format = p.rule("response-format", p.content(p.schema(p.json(), "response-format-schema", inputs.json_schema)));
            return ctx.reasoning_parser + p.space() + p.choice({
                p.literal("```json") + p.space() + response_format + p.space() + p.literal("```"),
                response_format
            }) + p.end();
        }

Oh... I know what you means. I'll implement it later. Let me convert this PR to draft before I fully implement it.

@hksdpc255 hksdpc255 marked this pull request as draft March 17, 2026 10:50
@hksdpc255 hksdpc255 marked this pull request as ready for review March 21, 2026 13:40
@hksdpc255
Copy link
Copy Markdown
Contributor Author

@pwilkin Mind taking another look?

@hksdpc255
Copy link
Copy Markdown
Contributor Author

@pwilkin Is current implementation good for merge now?

@pwilkin
Copy link
Copy Markdown
Member

pwilkin commented Mar 27, 2026

Yeah, almost good - please add proper tests to test-chat.cpp.

@hksdpc255
Copy link
Copy Markdown
Contributor Author

@pwilkin Done

@github-actions github-actions Bot added the testing Everything test related label Apr 1, 2026
@pwilkin
Copy link
Copy Markdown
Member

pwilkin commented Apr 1, 2026

@aldehir care to take a look?

@pwilkin
Copy link
Copy Markdown
Member

pwilkin commented Apr 1, 2026

Aight going to run CI and merge if green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants