Skip to content

Add workaround for templates requiring non-null content #19056

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.

Sign up for GitHub

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

Closed
pwilkin wants to merge 5 commits into from
Closed

Add workaround for templates requiring non-null content #19056

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
2393b17
Add workaround for templates requiring non-null content
pwilkin Jan 23, 2026
ede9586
Fix bad typo
pwilkin Jan 23, 2026
7a54851
Fix sanitizer warnings
pwilkin Jan 23, 2026
8570683
Make call IDs nine-character
pwilkin Jan 23, 2026
20db3d4
Revert caps detection, use workaround for all tool-calling templates
pwilkin Jan 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions common/chat.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2780,6 +2780,15 @@ static void system_message_not_supported(json & messages) {
}
}

static void requires_non_null_content(json & messages) {
GGML_ASSERT(messages.is_array());
for (auto & message : messages) {
if (message.contains("tool_calls") && !message.contains("content")) {
message["content"] = "";
}
}
}

static void func_args_not_string(json & messages) {
GGML_ASSERT(messages.is_array());
for (auto & message : messages) {
Expand Down Expand Up @@ -2885,6 +2894,13 @@ static common_chat_params common_chat_templates_apply_jinja(
workaround::system_message_not_supported(params.messages);
}

if (tmpl.original_caps().supports_tool_calls) {
// some templates will require the content field in tool call messages
// to still be non-null, this puts an empty string everywhere where the
// content field is null
workaround::requires_non_null_content(params.messages);
}
Comment on lines +2897 to +2902
Copy link
Contributor

@ngxson ngxson Jan 24, 2026

Choose a reason for hiding this comment

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

I'm not 100% sure if this is expected by all templates. in theory, there can be regression in these cases:

  • template check: if content is not none: (do something) else (do other things)
  • template that wraps content inside special tokens, example: <message><tool>...</tool><content></content></message> --> in such case, content maybe expected to be none

but that's just in theory, for now I can't think of a way to verify it.

Copy link
Contributor Author

@pwilkin pwilkin Jan 24, 2026

Choose a reason for hiding this comment

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

I thought the same, but @aldehir talked me out of it 😀 can always add the cap code if we change our mind.

Copy link
Contributor

@aldehir aldehir Jan 24, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Trying to recall, but I believe this is where simply checking "non-null" is insufficient:

llama.cpp/models/templates/Qwen-Qwen3-0.6B.jinja

Lines 40 to 42 in bb02f74

{%- if loop.last or (not loop.last and reasoning_content) %}
{{- '<|im_start|>' + message.role + '\n<think>\n' + reasoning_content.strip('\n') + '\n</think>\n\n' + content.lstrip('\n') }}
{%- else %}

This branch is only explored with a tool call + reasoning content. The Qwen3 template was not triggering the requires non-null check in Minja, so we were passing null content and it was throwing an exception. I think most templates are defensive against empty content, more than null content. A default of an empty string seems more reasonable to me. We are already passing in an empty string as of #18485. The capability checks do not align with this.


params.extra_context = json::object();
for (auto el : inputs.chat_template_kwargs) {
params.extra_context[el.first] = json::parse(el.second);
Expand Down
8 changes: 4 additions & 4 deletions common/jinja/caps.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ caps caps_get(jinja::program & prog) {
{"content", "Assistant message"},
{"tool_calls", json::array({
{
{"id", "call1"},
{"id", "call00001"},
Copy link
Contributor

@ngxson ngxson Jan 24, 2026

Choose a reason for hiding this comment

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

probably useful to leave a comment header explaining why the ID must be at a certain length (IIRC there was one or two templates enforces this?)

just in case we may come up with a better way in the future, we can come back and test the problematic template

Copy link
Contributor Author

@pwilkin pwilkin Jan 24, 2026

Choose a reason for hiding this comment

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

Right, I'll add.

{"type", "function"},
{"function", {
{"name", "tool1"},
Expand All @@ -170,10 +170,10 @@ caps caps_get(jinja::program & prog) {
}}
},
{
{"id", "call2"},
{"id", "call00002"},
{"type", "function"},
{"function", {
{"name", "tool2"},
{"name", "tool1"},
{"arguments", {
{"arg", "value"}
}}
Expand All @@ -194,7 +194,7 @@ caps caps_get(jinja::program & prog) {
{"name", "tool"},
{"type", "function"},
{"function", {
{"name", "tool"},
{"name", "tool1"},
{"description", "Tool description"},
{"parameters", {
{"type", "object"},
Expand Down
13 changes: 13 additions & 0 deletions tests/test-jinja.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -949,6 +949,19 @@ static void test_tests(testing & t) {
{{"x", {{"a", 1}}}},
"yes"
);

test_template(t, "something in undefined",
"{% if x in y %}yes{% else %}no{% endif %}",
{{"x", 1}},
"no"
);

test_template(t, "null is undefined",
"{% if null is not defined %}yes{% else %}no{% endif %}",
json::object(),
"yes"
);

}
Comment on lines +958 to 965
Copy link
Member

@CISC CISC Jan 24, 2026

Choose a reason for hiding this comment

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

Suggested change
test_template(t, "null is undefined",
"{% if null is not defined %}yes{% else %}no{% endif %}",
json::object(),
"yes"
);
}
}

What are you trying to test here, we already have this test (null is a JSON thing), you can change y to null if you want:

llama.cpp/tests/test-jinja.cpp

Lines 189 to 193 in 20db3d4

test_template(t, "is not defined",
"{% if y is not defined %}yes{% else %}no{% endif %}",
json::object(),
"yes"
);

Copy link
Contributor Author

@pwilkin pwilkin Jan 24, 2026

Choose a reason for hiding this comment

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

Yeah, I wanted to do the explicit "null constant is alias for undefined" check.


static void test_string_methods(testing & t) {
Expand Down
Loading