Skip to content

Add workaround for templates requiring non-null content#19056

Closed
pwilkin wants to merge 5 commits intoggml-org:masterfrom
pwilkin:non-null-content
Closed

Add workaround for templates requiring non-null content#19056
pwilkin wants to merge 5 commits intoggml-org:masterfrom
pwilkin:non-null-content

Conversation

@pwilkin
Copy link
Contributor

@pwilkin pwilkin commented Jan 23, 2026

As in topic, even though OpenAI standard is null content when assistant message contains tool calls, some templates explicitly require it to be non-null or they'll fail with an error.

@aldehir
Copy link
Contributor

aldehir commented Jan 23, 2026

This was a problem with a few templates, #18485 defaults to the empty string when null. I'm guessing the problem is during capability evaluation? If so, we should just pass in "content": "" to every message, instead of a whole workaround. Seems unnecessary, IMO.

@pwilkin
Copy link
Contributor Author

pwilkin commented Jan 23, 2026

This was a problem with a few templates, #18485 defaults to the empty string when null. I'm guessing the problem is during capability evaluation? If so, we should just pass in "content": "" to every message, instead of a whole workaround. Seems unnecessary, IMO.

If I remember correctly, other templates on the other hand DO expect content to be null on tool_call messages because that's the OpenAI spec, so I would settle for a workaround.

@pwilkin
Copy link
Contributor Author

pwilkin commented Jan 23, 2026

Although if I'm wrong, then yeah, scrapping the capability check and just keeping the workaround would be fine IMO.

@aldehir
Copy link
Contributor

aldehir commented Jan 23, 2026

It's already happening, and no one has complained so far 😊

@pwilkin
Copy link
Contributor Author

pwilkin commented Jan 23, 2026

Aight, will keep just the workaround.

Comment on lines +958 to 965

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

}
Copy link
Member

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:

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

Copy link
Contributor Author

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.

{"tool_calls", json::array({
{
{"id", "call1"},
{"id", "call00001"},
Copy link
Contributor

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

Choose a reason for hiding this comment

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

Right, I'll add.

Comment on lines +2897 to +2902
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);
}
Copy link
Contributor

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

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

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:

{%- 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.

@pwilkin pwilkin closed this Mar 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

jinja parser Issues related to the jinja parser testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants