Skip to content

Conversation

@grf53
Copy link
Contributor

@grf53 grf53 commented May 30, 2025

Description

Due to uses of nullptr to test templates which are caps_.requires_non_null_content == true, the capabilities of that template are being mis-estimated.
In the case I saw, even though the template covers 'tool call' and 'tool response', caps_.supports_tool_calls and caps_.supports_tool_responses were both false, so the tool calls and tool responses are rendered with the polyfills.

Currently in main, the non-null content requirement is being checked(caps_.requires_non_null_content), but it is difficult to find any meaningful use cases.
It seems desirable that this be used to suppress the above situation.

@grf53
Copy link
Contributor Author

grf53 commented Jun 18, 2025

With the Qwen3 model's chat template being improved, it seems that the change in this PR is no longer mandatory for Qwen3 template.

However, it may still be meaningful for other templates that require non-null content.

@yichunk
Copy link
Contributor

yichunk commented Jul 7, 2025

Could we merge this PR?
Though the qwen3 template is updated, this update is still useful.

Copy link
Contributor

@ochafik ochafik left a comment

Choose a reason for hiding this comment

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

Thanks @grf53 for sending this out, and sorry for the delay!!

@ochafik ochafik merged commit da98a14 into google:main Jul 10, 2025
3 of 11 checks passed
@yichunk
Copy link
Contributor

yichunk commented Jul 10, 2025

This PR broke the chat-template tests?
./scripts/tests.sh results in all test cases failed with the error like

460/547 Test #471: test-supported-template-Qwen-Qwen2.5-1.5B-Instruct-tool_use .....................................................***Failed    0.24 sec
# Testing template:
# ./build/bin/test-supported-template ["minja/build/tests/Qwen-Qwen2.5-1.5B-Instruct.jinja","minja/build/tests/Qwen-Qwen2.5-1.5B-Instruct.caps.json","minja/tests/contexts/tool_use.json","minja/build/tests/Qwen-Qwen2.5-1.5B-Instruct-tool_use.txt"]
Test failed: basic_string: construction from null is not valid

@yichunk
Copy link
Contributor

yichunk commented Jul 10, 2025

Please check the fix #75

@yatarkan
Copy link
Contributor

This PR broke the chat-template tests?

That is true, tests were broken by this PR, see https://github.com/google/minja/actions/runs/15346704885/job/43184463260#step:9:7818

ochafik added a commit that referenced this pull request Aug 7, 2025
@ochafik ochafik mentioned this pull request Aug 7, 2025
ochafik added a commit that referenced this pull request Aug 7, 2025
* Port content handling of #72 to python test logic

* Pin dependency versions

* add clangd/ to .gitignore

* disable QwQ-32B test
ochafik pushed a commit to ochafik/minja that referenced this pull request Nov 2, 2025
…ontent (google#72)

fix: use empty string instead of nullptr for templates requiring non null
ochafik added a commit to ochafik/minja that referenced this pull request Nov 2, 2025
* Port content handling of google#72 to python test logic

* Pin dependency versions

* add clangd/ to .gitignore

* disable QwQ-32B test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants