Skip to content

server : guard chat-template thinking probe against apply-time jinja errors#24093

Merged
pwilkin merged 4 commits into
ggml-org:masterfrom
palios-taey:fix/chat-template-thinking-guard
Jul 9, 2026
Merged

server : guard chat-template thinking probe against apply-time jinja errors#24093
pwilkin merged 4 commits into
ggml-org:masterfrom
palios-taey:fix/chat-template-thinking-guard

Conversation

@palios-taey

Copy link
Copy Markdown
Contributor

Hit a SIGABRT in llama-cli --jinja loading a model whose chat template parses at init but fails parser generation at apply time (one using {% call %}). The throw — std::invalid_argument from common_chat_templates_support_enable_thinking() — escapes because that call sits just outside the try/catch guarding common_chat_templates_init(), so it is uncaught and the process aborts instead of failing to load.

Moved the thinking probe inside that try/catch. Apply-time errors now fail load the same way init parse errors do — chat template parsing error, clean return. llama-server already degraded gracefully here; this fixes the llama-cli path and keeps both calls behind one guard.

Verified before/after on a model with such a template: SIGABRT → clean rc 1. Normal model still loads and runs under --jinja (CLI + server).

AI usage disclosure: AI-assisted diagnosing the abort and drafting the fix. Reviewed it, own it, verified the before/after myself.

@palios-taey
palios-taey requested a review from a team as a code owner June 3, 2026 22:41
@ggml-gh-bot

ggml-gh-bot Bot commented Jun 3, 2026

Copy link
Copy Markdown

Hi @palios-taey, thanks for your contribution!

Per our contribution guidelines, the automated PR checker found the following issue(s) that need your attention:

  • AI-generated content: This project does not accept PRs, descriptions or commit messages that are fully or predominantly AI-generated. If you have used AI to assist you in writing code, please make sure to disclose that explicitly.

Please note that maintainers reserve the right to make final decisions on PRs. If you believe there is a mistake, please comment below.

@ngxson
ngxson requested a review from pwilkin June 4, 2026 09:20

@pwilkin pwilkin left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yeah, change looks fine, thanks.

A model whose chat template parses at init but fails parser generation
at apply time (e.g. uses {% call %}) throws std::invalid_argument from
common_chat_templates_support_enable_thinking(), which ran outside the
try/catch guarding common_chat_templates_init(). The throw was uncaught
and llama-cli aborted (SIGABRT) instead of failing to load. Moved the
probe inside that try/catch so an apply-time error fails load the same
way an init parse error does.

Signed-off-by: Jesse LaRose <jesse@taey.ai>
@palios-taey
palios-taey force-pushed the fix/chat-template-thinking-guard branch from 5f0afe7 to c8ebb07 Compare June 22, 2026 17:56
@palios-taey

Copy link
Copy Markdown
Contributor Author

Rebased on master and resolved the conflict in tools/server/server-context.cpp — the change itself is unchanged (the thinking probe + log stay inside the init try/catch).

@pwilkin

pwilkin commented Jun 27, 2026

Copy link
Copy Markdown
Member

@ggml-org/maintainers could use an approval please.

@ggerganov

ggerganov commented Jun 27, 2026

Copy link
Copy Markdown
Member

@pwilkin Btw, slightly related - can we detect and print a hint for preserve_thinking when it is supported by the template? I think that this chat option (i.e. --chat-template-kwargs "{\"preserve_thinking\": true}") is a must for Qwen3.6 models, yet likely many users don't use it because it's quite obscure. Printing a hint to enable it on startup would be helpful.

Assisted-by: Codex
Signed-off-by: Jesse LaRose <jesse@taey.ai>
@palios-taey

Copy link
Copy Markdown
Contributor Author

Rebased on master to clear the conflict from the server-context split. The fix itself is unchanged — enable_thinking is declared before the try and the template_supports_thinking probe moved inside it, same as before, just re-applied on top of the refactored code (kept the SRV_TRC line). Still +7/-7 in the one file. llama-server builds clean. Ready for a re-approval whenever you have a moment, @pwilkin.

@palios-taey

Copy link
Copy Markdown
Contributor Author

Friendly bump — this is green and mergeable, and I think it's just stuck on a technicality: both approvals (@pwilkin on 5f0afe7, @forforever73 on d36fc59) predate the 6/28 rebase, so GitHub counts them as stale against the current head dc30a3a and still shows "review required". The rebase was only to clear the conflict from the server-context split — the change itself is unchanged, +7/−7 in one file. A re-approval of the current head would let it land. Thanks for the reviews.

@pwilkin
pwilkin merged commit 82fce65 into ggml-org:master Jul 9, 2026
25 checks passed
@Moore2877

Copy link
Copy Markdown

I've been working on a custom chat template for Qwen and this morning has been a rollercoaster with this and other pushes while testing after updating llama.cpp. At first I thought I broke something but now things are working better than before yesterday. https://huggingface.co/Moore2877/Qwen-Fixed-Chat-Templates-llamacpp

fewtarius added a commit to fewtarius/CachyLLama that referenced this pull request Jul 19, 2026
…, OpenCL Q6_K/Adreno, CORS, checkpoint min-step, prompt cache refactor, MoE expert API stays)

Upstream highlights since 6be7459:
- model: DFlash speculative with KV rotation (ggml-org#25823)
- model: Hy3 (hy_v3) with MTP speculative decoding (ggml-org#25395)
- model: DeepseekV4 with fused hyper-connection ops (ggml-org#25585)
- ggml: 0.17.0, LIGHTNING_INDEXER, out_prod, f16 set_rows
- vulkan: Q2_0 support, native e2m1/e4m3 conversions, transfer-queue race fix
- CUDA: MMQ kernel config refactor (ggml-org#24127), tighter MMQ src1 buffer for fp4 (ggml-org#25613), CUDA graphs on Volta/Turing, MoE gate/up dedup, CUDA Virtual Devices
- ROCm: hexagon L2 cache rework, native fp4, FP16/INT8 coopmat on AMD
- SYCL: Battlemage flash attention via oneDNN XMX, XIELU op, fp16 conv2d_dw
- OpenCL: Q6_K GEMM/GEMV fix, ragged-tile MoE prefill FP16, Adreno vectorized LD/ST, A7x optimizations, ABS op
- kleidiai: SME2 f32 kernel, SME vs SME2 dispatch
- server: refactor prompt cache state ownership (ggml-org#25649) - new server_prompt_cache_state separates prompt metadata from KV data
- server: evict checkpoints within min-step (ggml-org#25472)
- server: text-only slot save/restore with mtmd (ggml-org#25076)
- server: --cors-* options (ggml-org#25655)
- server: refactored server_stream (ggml-org#25541)
- server: respect min-step when splitting prompt batches (ggml-org#25420)
- server: move chat-template thinking probe inside init try/catch (ggml-org#24093)
- common: auto-download dflash/eagle3 HF sidecars (ggml-org#25811), drop --stdin mutual-exclusion, align tokenize usage
- conversion: BitNetForCausalLM, dflash tokenizer fix, split MTP export for HY V3
- llama-quant: exclude i32 ffn_gate_tid2eid routing table, allow manual tensor types with --pure
- llama-batch: fix allowed decreasing pos in a seq (ggml-org#25449), n_keep_tail in split_equal for recurrent
- llama: refactor fused ops (ggml-org#24646), TP fix for Phi3/Bert/Plamo2/3/ChatGLM
- ui: agentic content UX, reasoning effort on mobile add sheet, MCP panel fixes, thinking menu fix
- vendor: BoringSSL 0.20250713.0
- tests: actually exercise test-recurrent-state-rollback, ds_v4_hc sentinel init, export-graph-ops graceful exit

CachyLLama preservation work (conflict resolution):

1. tools/server/server-task.h: Accept upstream's server_prompt refactor (no data member, clear() method).
   Move our t_last_used field from server_prompt to server_prompt_cache_state (where it now lives
   after the refactor). server_prompt_cache_state already has the size() method, so our old
   size() on server_prompt is no longer needed.

2. tools/server/server-context.cpp (create_checkpoint): Take upstream's min-step eviction
   pre-filter as the FIRST pass, then keep our existing highest-pos_min eviction as the
   capacity overflow fallback. These are complementary: min-step removes redundant checkpoints
   from the same task; highest-pos_min keeps the rec-window-friendly checkpoints when at cap.

3. tools/server/server-context.cpp (handle_completions_impl): Keep our std::vector<server_task>
   tasks batching for multi-prompt requests and per-user concurrency check, AND take upstream's
   res->set_req(&req) for spipe ownership transfer.

4. tools/server/server-task.cpp: Fix references to entry.tokens -> entry.prompt.tokens,
   entry.checkpoints -> entry.prompt.checkpoints, entry.n_tokens() -> entry.prompt.n_tokens().
   Update find_eviction_candidate return type from list<server_prompt>::iterator to
   list<server_prompt_cache_state>::iterator.

5. ggml/src/ggml-cuda/mmq.cuh + new mmq-config-rdna3_5.cuh: Upstream's massive MMQ refactor
   moved per-architecture config into separate files but did NOT add RDNA3.5 (gfx1150/1/2/3,
   Strix Halo). Create mmq-config-rdna3_5.cuh (231 CASE entries) derived from rdna2 with
   nthreads=128 (4 warps) and I=48 (smaller X tile) matching our original Strix Halo tuning.
   Wire into both host and device dispatch paths before the RDNA4 / RDNA2 fallback.

6. README.md and AGENTS.md: Keep CachyLLama-specific links and project context where upstream
   added parallel content.

Verified:
- cmake --build builds clean (Release, CPU-only)
- llama-server starts, --help shows all CachyLLama flags preserved:
  --cache-ssd-hot-ram, --cache-ssd-warm-ram, --cache-ssd-system-prompts,
  --cache-ssd-system-max-days, --cache-ssd-no-fsync, --cache-ssd-max-conversations,
  --max-concurrent-per-user
- /expert-stats and /expert-tracking endpoints preserved
- 55/58 tests pass; 3 failures unrelated to merge:
  - test-tokenizers-ggml-vocabs: missing model downloads
  - test-jinja-py: missing jinja2 Python module
  - test-quant-type-selection: snapshot mismatch on upstream's new MXFP4_MOE heuristic

Custom CachyLLama files untouched (no upstream conflicts):
- common/kv-ssd-cache.{cpp,h}, common/kv-ssd-posix.h, common/kv-ssd-system-cache.{cpp,h}
- common/kv_page_manager.{cpp,h}
- tools/server/server-context-page-manager.{cpp,h}
- tools/server/server-context-ssd-cache.{cpp,h}
- test_kv_page_manager.cpp, tests/test-ssd-cache-caps.cpp
- STRIX_HALO_NOTES.md, docs/development/user-isolation-design.md
- .github/workflows/build-cpu.yml, build-cuda-windows.yml, build-vulkan.yml
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants