Skip to content

Conversation

@ExtReMLapin
Copy link
Contributor

@ExtReMLapin ExtReMLapin commented Aug 11, 2025

There was this issue that we would not use tool_choice at required with reasoning because of forced tool call imposed by grammar.

Grammar now allows the model to think first.

Reasoning = big brain

Tool calling = strong arms

Now you can be very smart and very strong

Fixes #15247

@ExtReMLapin
Copy link
Contributor Author

All am I sure is that it fixes issues with Qwen3 + reasoning (enabled or disabled)+ tool calling.
I fear it also has to be implemented for others reasoning parsers (non hermes)

CC hermes2 contributor : @ochafik

@ExtReMLapin
Copy link
Contributor Author

Back at the office on tuesday, re-reading the PR i might reconsider the logic around

thinking_forced_open

@ExtReMLapin
Copy link
Contributor Author

ExtReMLapin commented Aug 25, 2025

For a future PR, the following functions needs the same kind of patch :

  • common_chat_params_init_granite : same as qwen3, but i'm not sure as the thinking tags are not always there
  • common_chat_params_init_command_r7b : "<|START_THINKING|>", "<|END_THINKING|>"
  • common_chat_params_init_deepseek_r1 : same as qwen3
  • common_chat_params_init_gpt_oss ???

ready for review @ggerganov

Not sure exactly who I should ping as ochafik seems to be busy this week

@ExtReMLapin ExtReMLapin marked this pull request as draft August 26, 2025 12:59
@ExtReMLapin ExtReMLapin marked this pull request as ready for review August 29, 2025 12:49
@ExtReMLapin
Copy link
Contributor Author

Not sure who I should ping

@slaren

@ggerganov ggerganov requested a review from ochafik September 24, 2025 07:19
@ggerganov
Copy link
Member

Same comment as #15019 (comment)

This is a smaller change, so I can take a look and merge this, but prefer if we have someone who would take over this part of the code.

@ExtReMLapin
Copy link
Contributor Author

Got it, thanks for keeping me updated !

@ExtReMLapin
Copy link
Contributor Author

If you are being held hostage, blink twice @ochafik

Copy link
Collaborator

@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 @ExtReMLapin ! Looks very promising :-)

Could you add some tests in test-chat.cpp (will need to force git add models/templates/qwen3-something.jinja, see models/templates/README.md )

@ExtReMLapin
Copy link
Contributor Author

ExtReMLapin commented Nov 4, 2025

So I added two sets of tests, one in test-chat.cpp (100% ai generated, but tested) and I'm less that meh-ly conviced of their point.

I would have much more liked some kind of grammar check that filter/accepts/prune the reasoning part.

And this is why I added another test inside test_tool_call.py of the server tool, which is in my opinion not the perfect place because chat.cpp is not only used in the server, but it's an e2e test and much more straighforward at checking if things works.

Test results before this PR :

===================================================================================================== short test summary info ======================================================================================================
FAILED unit/test_tool_call.py::test_required_tool_with_reasoning[tool0-unsloth/Qwen3-0.6B-GGUF:Q4_K_M-None-deepseek-CompletionMode.NORMAL] - AssertionError: Expected reasoning content, but got None
FAILED unit/test_tool_call.py::test_required_tool_with_reasoning[tool0-unsloth/Qwen3-0.6B-GGUF:Q4_K_M-None-deepseek-CompletionMode.STREAMED] - AssertionError: Expected reasoning content, but got None
FAILED unit/test_tool_call.py::test_required_tool_with_reasoning[tool1-unsloth/Qwen3-0.6B-GGUF:Q4_K_M-None-deepseek-CompletionMode.NORMAL] - AssertionError: Expected reasoning content, but got None
FAILED unit/test_tool_call.py::test_required_tool_with_reasoning[tool1-unsloth/Qwen3-0.6B-GGUF:Q4_K_M-None-deepseek-CompletionMode.STREAMED] - AssertionError: Expected reasoning content, but got None
================================================================================================ 4 failed, 463 deselected in 5.79s =================================================================================================
[Inferior 1 (process 936362) detached]
terminate called after throwing an instance of 'std::runtime_error'
  what():  Test failed
Abandon (core dumped)

After :

unit/test_tool_call.py ....                                                                                                                                                                                                  [100%]

================================================================================================ 4 passed, 463 deselected in 6.07s =================================================================================================

[chat] All tests passed!

@github-actions github-actions bot added testing Everything test related examples python python script changes server labels Nov 4, 2025
@ExtReMLapin ExtReMLapin requested a review from ochafik November 4, 2025 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

examples python python script changes server testing Everything test related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Misc. bug: [chat] (hermes 2) Impossible de to use both tool_choice: "required" and reasoning

3 participants