fix: map GGUF architecture to built-in chat template when embedded is missing - #9440
Conversation
0d4757c to
9187455
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d4757cb16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(("gemma", ChatTemplateFallbackWarning::KnownArchitecture)) | ||
| } | ||
| "gemma4" => Some(("gemma", ChatTemplateFallbackWarning::Gemma4Unsupported)), | ||
| "llama" | "llama2" => Some(("llama2", ChatTemplateFallbackWarning::KnownArchitecture)), |
There was a problem hiding this comment.
Avoid treating every llama GGUF as Llama 2
When a GGUF is missing tokenizer.chat_template, this maps the general.architecture value llama to the Llama 2 template, but that metadata value is shared by newer Llama-family GGUFs as well (the local provider’s default model is Llama 3.2). In that fallback path, Llama 3.x models without an embedded template will be prompted with [INST]...[/INST] instead of the Llama 3 header/eot format, leading to malformed prompts and poor or broken responses; the fallback needs more specific model metadata/name handling before choosing llama2.
Useful? React with 👍 / 👎.
9187455 to
3a663d2
Compare
|
Thanks very much for the work here. For now we have gone a slightly different path (#9442), I wrote a bit about the reasoning in #9110 (comment) — very open to discussion on this as I know the stricter path may make some GGUF conversions (that lack embedded chat templates) a little harder to use, but at least there is an escape hatch for using them now. |
Summary
Maps the GGUF model architecture metadata to a built-in chat template when the model does not ship an embedded template, instead of unconditionally using chatml. gemma/llama/qwen/phi families now get an architecture-appropriate fallback with a specific warning level per case.
Why this matters
Issue #9110 reported that local-inference models without an embedded chat template silently defaulted to chatml, which produces wrong outputs for gemma/llama families. The old llamacpp path issued
warn!("Model has no embedded chat template, falling back to chatml")and continued. For a Gemma-family model that is the wrong template; the response degrades badly.Changes
select_chat_template()inlocal_inference.rsreturns eitherEmbedded(t)when the embedded template is present orBuiltIn(BuiltInChatTemplateFallback)when it is notGemma4Unsupportedwarning, llama/llama2 -> "llama2", llama3 -> "llama3", qwen2 -> "chatml", phi3 -> "phi3"UnknownArchitecturewarningMissingArchitecturewarningLlamaCppBackend::loadreads the model arch viameta_val_str("general.architecture")and routes throughselect_chat_templatelog_chat_template_fallbackemits info vs warn based on the fallback variantTesting
cargo test --features local-inference -p goose --lib local_inference_tests-- 6/6 pass:KnownArchitecturewarningGemma4UnsupportedwarnMissingArchitecturewarnFixes #9110