Skip to content

common : default content to an empty string#18485

Merged
aldehir merged 2 commits intoggml-org:masterfrom
aldehir:fix-null-content
Dec 30, 2025
Merged

common : default content to an empty string#18485
aldehir merged 2 commits intoggml-org:masterfrom
aldehir:fix-null-content

Conversation

@aldehir
Copy link
Collaborator

@aldehir aldehir commented Dec 30, 2025

Fixes #18463

This issue was also spotted here: #18342 (comment)

Templates receive null for content even when the client sends an empty string "". Most templates handle this defensively, but the Qwen3 templates do not. Even the latest https://huggingface.co/Qwen/Qwen3-14B fails when both reasoning_content and tool_calls are present.

This PR ensures content is always passed as an empty string if it's missing, null, or empty. The only template this breaks is the llama.cpp-specific Deepseek R1 template, which I've patched here.

This also affects the Minja polyfill, since content will now always be present in a polyfilled assistant response. I don't see this as a problem given that the polyfill mechanism is already a workaround by nature.

@aldehir aldehir changed the title Fix null content common : default content to an empty string Dec 30, 2025
@github-actions github-actions bot added the testing Everything test related label Dec 30, 2025
Copy link
Contributor

@ngxson ngxson left a comment

Choose a reason for hiding this comment

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

This PR make me think about an idea for #18462 . Currently, I can already have a "workaround" system that automatically detect which kind of workarounds should be used.

Because I can now hook into operators like something is not none, I can probably check if the template ever call is not none on message["content"] and only deactivates the workaround (so that None is preserved ; for other models not checking this, content ="" is kept as-is)

Do you think it's something worth implemented? (It means we will be able to revert this change once the new engine is ready)

@aldehir
Copy link
Collaborator Author

aldehir commented Dec 30, 2025

This PR make me think about an idea for #18462 . Currently, I can already have a "workaround" system that automatically detect which kind of workarounds should be used.

This is a cool idea, but I think a more robust solution would be static type checking. This is done similarly to how you execute the template, but instead of producing values you produce types instead. Then you can statically verify if a type is supported.

content: String | None
lstrip(input: String, strip_chars: String): String

{{ content | lstrip("\n") }} # Error, since lstrip does not accept None

{% if content is not None %}  # Type guard asserts that content : String
{{ content | lstrip("\n') }} # Good
{% endif %}

Of course, it would be a more complex implementation but a fun thought.

@pwilkin
Copy link
Contributor

pwilkin commented Dec 30, 2025

@ngxson I think the core idea is nice, but generally I believe that we should enforce proper semantics instead of adding workarounds.

One of the reasons the code in the chat parser is so messy is that there are a lot of workarounds that are basically useless from a practical perspective - they were added to patch i.e. toolcalling capacity for models that don't really properly support tool calling anyway. The result is a gimmick that allows a 3B model to maybe possibly once call a get_weather tool in a demo chat, but is absolutely useless in any real-life scenario.

I think we should focus on supporting proper templates for models that are otherwise verified to be working and possibly drop polyfills that in most cases don't even accomplish what they set out to do.

@ngxson
Copy link
Contributor

ngxson commented Dec 30, 2025

I think the core idea is nice, but generally I believe that we should enforce proper semantics instead of adding workarounds.

Hmm yeah that make sense. I think what we can do is to allow a fixed number of schema that can be used. For model with broken template, we can maintain a llama.cpp-specific version (IIUC this is what we are doing in models/templates)

@aldehir Just to clarify that my core idea is to return more data from jinja rendering, then it's up to another logic to decide what to do with the returned result.

The current problem is that most (if not all) jinja engines simply return either a string or an exception which is not enough to (programmatically) know what's wrong / what's working.

My engine already return a list of string "parts" to mark input text. It will be trivial to also return a list of operations being applied to a specific input. In your case, the engine can return:

  • is not none was checked against content
  • lstrip filter was used on content

But I'm not entirely sure if this info is useful enough and/or the engine may skip checks as they are hidden under an if..else branch.

@aldehir aldehir merged commit 0f89d2e into ggml-org:master Dec 30, 2025
66 of 71 checks passed
srogmann pushed a commit to srogmann/llama.cpp that referenced this pull request Jan 1, 2026
* common : default content to an empty string

* common : fix tests that break when content != null
blime4 referenced this pull request in blime4/llama.cpp Feb 5, 2026
* common : default content to an empty string

* common : fix tests that break when content != null
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.

Eval bug: Qwen3 fails to parse tool response (Value is not an array or object: null at row 22, column 40)

4 participants