Skip to content

Fix DeepSeek4 crafted template#25414

Open
pwilkin wants to merge 4 commits into
ggml-org:masterfrom
pwilkin:ds4-template-fix
Open

Fix DeepSeek4 crafted template#25414
pwilkin wants to merge 4 commits into
ggml-org:masterfrom
pwilkin:ds4-template-fix

Conversation

@pwilkin

@pwilkin pwilkin commented Jul 7, 2026

Copy link
Copy Markdown
Member

Overview

It seems like I've been a bit lazy implementing the DS4 template. I used the standard behavior (retain last reasoning trace). However, the reference behavior is different. DS4 will retain all reasoning traces in one of two cases:

  • if drop_reasoning is set to false
  • in case the message history contains at least one tool call

I've patched the template and added a test to verify this.

Additional information

Note that under the current behavior llama.cpp defaults to setting drop_reasoning to true, so if you want it set to false you'll have to add --preserve-reasoning - not that it matters that much because for all agentic cases, DS4 will still force the retain of traces due to the presence of tool calls.

Requirements

  • I have read and agree with the contributing guidelines
  • AI usage disclosure: Yes, a bit of GLM-5.2 and Claude to make the tests (funny thing: GLM-5.2 tripped on reading stuff that contained <think> and </think> :))

@pwilkin pwilkin requested a review from CISC as a code owner July 7, 2026 17:45
@pwilkin pwilkin requested review from CISC, aldehir, am17an and ngxson and removed request for CISC July 7, 2026 17:45
@github-actions github-actions Bot added testing Everything test related jinja parser Issues related to the jinja parser labels Jul 7, 2026

@aldehir aldehir left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we want to start testing templates, probably best to move those to a separate test suite as this one is already pretty large.

IMO Python tests are more appropriate for the Jinja templates.

@pwilkin

pwilkin commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

Yeah, I'll probably move to refactor test-chat.cpp in a followup PR.

Personally, I'm against Python tests because the C++ tests double down as a check on the template rendering / message passing, which Python won't do - I prefer the tests to run inside the same engine as the actual template processing.

@tarruda

tarruda commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

As I had explained in the reddit thread, this is not the only issue and I had to opt out of the autoparser in order to properly run DSV4. In particular, parallel tool calling is not working with the autoparser and while running on pi I was getting random stops with tool calls within the reasoning block.

Everything done to fix are contained in these two commits sent to fairydreaming's lightining indexer branch: fairydreaming#4

@pwilkin

pwilkin commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

@tarruda yeah, working on the parser extension next :)

@pwilkin pwilkin requested a review from a team as a code owner July 7, 2026 19:06
@aldehir

aldehir commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

@tarruda can you please include your response from Reddit for visibility purposes. Also because the link appears broken on my end.

@pwilkin

pwilkin commented Jul 7, 2026

Copy link
Copy Markdown
Member Author

This is basically pretty similar to @tarruda 's branch :) a few style differences.

@tarruda technically your approach to the template flag isn't in line with the reference: they require reasoning traces not if tools are declared, but if any tool calls are actually present (I know, practically it doesn't really matter too much, but just nitpicking :))

@tarruda

tarruda commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

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.

3 participants