From b84bf1691e6c79c7c923978dbafe85dfe9809925 Mon Sep 17 00:00:00 2001 From: DevelopmentCats Date: Thu, 5 Nov 2026 19:17:37 +0000 Subject: [PATCH 1/3] model-types: move per-model_type fixes into catalogue/model-types The gpt-oss tiktoken vocab and parsers and the Qwen3.5 chat template patch were Go code in internal/resolve and internal/hf. Changing or adding one meant editing internals. They are now catalogue files keyed on the HuggingFace model_type, read by internal/modeltype, and a machine owner can add or replace one in model-types.d without a rebuild. The template fix now applies only to qwen3_5 and qwen3_5_moe repos. gpt-oss profiles carry TIKTOKEN_ENCODINGS_BASE themselves, so setup no longer writes it to config.env and doctor drops the harmony_vocab check. Existing config.env lines keep working. Adds 'model-types list' and 'model-types show'. --- AGENTS.md | 3 +- CHANGELOG.md | 21 ++ catalogue/catalogue.go | 8 +- catalogue/model-types/README.md | 12 + catalogue/model-types/gpt_oss.json | 32 +++ catalogue/model-types/qwen3_5.json | 18 ++ catalogue/model-types/qwen3_5_moe.json | 18 ++ docs/install.md | 1 - docs/manifest.json | 22 ++ docs/model-types.md | 69 +++++ docs/reference/cli.md | 1 + docs/reference/cli/model-types.md | 19 ++ docs/reference/cli/model-types/list.md | 26 ++ docs/reference/cli/model-types/show.md | 23 ++ internal/cli/detectcmds.go | 23 +- internal/cli/modeltypescmd.go | 150 ++++++++++ internal/cli/root.go | 3 + internal/cli/root_internal_test.go | 2 +- internal/cli/setup.go | 7 +- internal/hf/hf.go | 43 +-- internal/hf/hf_internal_test.go | 93 +------ internal/modeltype/local.go | 77 ++++++ internal/modeltype/modeltype.go | 259 ++++++++++++++++++ internal/modeltype/modeltype_internal_test.go | 159 +++++++++++ internal/profile/profile.go | 14 + internal/resolve/modeltype.go | 142 ++++++++++ internal/resolve/modeltype_internal_test.go | 161 +++++++++++ internal/resolve/resolve.go | 42 ++- internal/resolve/resolve_internal_test.go | 57 +++- internal/resolve/tiktoken.go | 155 ----------- internal/resolve/tiktoken_internal_test.go | 125 --------- internal/vllm/validate.go | 12 + 32 files changed, 1334 insertions(+), 463 deletions(-) create mode 100644 catalogue/model-types/README.md create mode 100644 catalogue/model-types/gpt_oss.json create mode 100644 catalogue/model-types/qwen3_5.json create mode 100644 catalogue/model-types/qwen3_5_moe.json create mode 100644 docs/model-types.md create mode 100644 docs/reference/cli/model-types.md create mode 100644 docs/reference/cli/model-types/list.md create mode 100644 docs/reference/cli/model-types/show.md create mode 100644 internal/cli/modeltypescmd.go create mode 100644 internal/modeltype/local.go create mode 100644 internal/modeltype/modeltype.go create mode 100644 internal/modeltype/modeltype_internal_test.go create mode 100644 internal/resolve/modeltype.go create mode 100644 internal/resolve/modeltype_internal_test.go delete mode 100644 internal/resolve/tiktoken.go delete mode 100644 internal/resolve/tiktoken_internal_test.go diff --git a/AGENTS.md b/AGENTS.md index 048c4de..1c9a2e2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -23,7 +23,7 @@ llm-helper is one Go binary that manages inference servers (vLLM, mlx-lm, llama. ## Repository layout -`internal/` holds the packages the binary builds from: `cli` for the commands, `control` for what the CLI and the web panel share, `web` for the panel, and the rest one package per feature. `catalogue/` holds the shipped data definitions (engines, recipes, hardware) with the Go that embeds them in `catalogue/catalogue.go`; each kind is read by the `internal/` package of the same name (`engine`, `recipe`, `hardware`), and `resolve` combines them with a model's card and config into launch settings. `docs/` is the shipped documentation, and `tools/recipediff` is a separate module. +`internal/` holds the packages the binary builds from: `cli` for the commands, `control` for what the CLI and the web panel share, `web` for the panel, and the rest one package per feature. `catalogue/` holds the shipped data definitions (engines, recipes, model types, hardware) with the Go that embeds them in `catalogue/catalogue.go`; each kind is read by the `internal/` package of the same name (`engine`, `recipe`, `modeltype`, `hardware`), and `resolve` combines them with a model's card and config into launch settings. `docs/` is the shipped documentation, and `tools/recipediff` is a separate module. ## Essential commands @@ -51,6 +51,7 @@ CI runs gofmt, vet, and the race tests on Linux and macOS, builds `linux/amd64`, - **On-disk compatibility:** config, profiles, and state outlive the binary. Changes to profile parsing, `EXTRA_ARGS` handling, or the service unit must still read files written by the previous release. Follow "Upgrade safety" in [RELEASING.md](RELEASING.md). - **Recipes:** each file in `catalogue/recipes/` is one recipe, named after its id, measured on real hardware and pinned by image digest. Do not invent or generate one. Record each deviation from upstream as data in the entry, not as a comment. - **Hardware:** each vendor, architecture and machine is one file in `catalogue/hardware/`, in the folder for its kind, named after its id. A machine needs a report in `catalogue/hardware/reports/` that detects it, and no two machines may match the same GPU. Do not match a GPU by its name in Go code: add a machine file. +- **Model types:** what an architecture needs (files, flags, chat template fixes) is one file per HuggingFace `model_type` in `catalogue/model-types/`. Do not name a model type in Go; `TestNoGoCodeRepeatsAShippedModelType` fails if Go repeats what a shipped file says. - **Engines and sources:** an engine is one definition file in `catalogue/engines/`, named after its id, and a recipe's upstream is data that is read, never executed. Do not add code that branches on an engine's name: act on its capabilities, and put behavior a definition cannot express in a Go hook gated by a capability. - **Secrets:** keep API keys and tokens off command lines, out of dry-run output, and out of logs. Never commit `secrets.env`, `.env`, or a token. - **Dependencies:** prefer the standard library. Ask before adding a module. `tools/recipediff` is a separate module so its dependencies stay out of the CLI binary. diff --git a/CHANGELOG.md b/CHANGELOG.md index a5cc3c0..3cedbc0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,27 @@ the fact. ## [Unreleased] +### Added + +- Model types: what an architecture needs to run correctly is now data, one + file per HuggingFace `model_type` in `catalogue/model-types/`. `new` applies + the file for the repo's `model_type` and the profile's engine. + `llm-helper model-types list` and `show` print them, and a file in + `model-types.d/` in the config directory adds one or replaces a shipped one + without a rebuild. + +### Changed + +- gpt-oss profiles now carry `TIKTOKEN_ENCODINGS_BASE` themselves, and + `setup` no longer writes it into `config.env`. Existing configs keep the + line, and existing profiles serve as before. +- `doctor` no longer reports whether the tiktoken vocab files are on disk, + and its JSON drops `harmony_vocab`. `new` fetches the files when a model + needs them. +- The chat template fix for a system message after the first turn applies to + Qwen3.5 repos (`qwen3_5` and `qwen3_5_moe`, which includes Ornith), not to + any repo whose template contains the same text. + ### Fixed - Profiles for Qwen3 hybrid models got `--reasoning-parser deepseek_r1` from diff --git a/catalogue/catalogue.go b/catalogue/catalogue.go index ad55c82..afe5dc5 100644 --- a/catalogue/catalogue.go +++ b/catalogue/catalogue.go @@ -1,7 +1,8 @@ // Package catalogue holds the definitions llm-helper ships as data, one JSON // file each, named by id: engines/ has the inference servers it can launch, // recipes/ the launch configurations that were run and measured on named -// hardware, and hardware/ what is known about specific GPUs. Each folder's +// hardware, model-types/ what every repo of a HuggingFace model_type needs on +// each engine, and hardware/ what is known about specific GPUs. Each folder's // README says how to add one. package catalogue @@ -13,7 +14,7 @@ import ( // The hardware reports are test fixtures, read from disk by the tests, so // they are not embedded. // -//go:embed engines/*.json recipes/*.json +//go:embed engines/*.json recipes/*.json model-types/*.json //go:embed hardware/vendors/*.json hardware/architectures/*.json hardware/machines/*.json var files embed.FS @@ -23,6 +24,9 @@ var Engines = sub("engines") // Recipes is every tested recipe shipped in the binary. var Recipes = sub("recipes") +// ModelTypes is every model-type definition shipped in the binary. +var ModelTypes = sub("model-types") + // Hardware is every vendor, architecture and machine shipped in the binary, // each kind in a folder of its own. var Hardware = sub("hardware") diff --git a/catalogue/model-types/README.md b/catalogue/model-types/README.md new file mode 100644 index 0000000..51fafcb --- /dev/null +++ b/catalogue/model-types/README.md @@ -0,0 +1,12 @@ +# Model types + +Each file here is one HuggingFace `model_type`, named after it: the value in a repo's `config.json`, such as `gpt_oss` in `openai/gpt-oss-120b`. +It says what every repo of that type needs on each engine, so the finetunes and re-uploads of a model get it too. + +To add one, copy the closest file to `.json`. +Under `engines`, keyed by an engine id from [`catalogue/engines/`](../engines), it can list files to fetch (`files`), flags to add when the card did not name them (`args`), and chat template fixes (`chat_template_fixes`). +Give each a `why`. +A configuration for one specific repo that was run and measured belongs in [`catalogue/recipes/`](../recipes) instead. + +The fields are described in [docs/model-types.md](../../docs/model-types.md). +To try one without rebuilding, put its file in `~/.config/vllm-helper/model-types.d/`; a file there with a shipped id replaces the shipped one. diff --git a/catalogue/model-types/gpt_oss.json b/catalogue/model-types/gpt_oss.json new file mode 100644 index 0000000..6696cf6 --- /dev/null +++ b/catalogue/model-types/gpt_oss.json @@ -0,0 +1,32 @@ +{ + "schema_version": 1, + "id": "gpt_oss", + "display_name": "gpt-oss", + "why": "gpt-oss writes its tool calls and its thinking in OpenAI's harmony format, and harmony's tokenizer reads its vocab from disk rather than from the model repo.", + "engines": { + "vllm": { + "files": [ + { + "dir": "tiktoken-encodings", + "env": "TIKTOKEN_ENCODINGS_BASE", + "base_url": "https://openaipublic.blob.core.windows.net/encodings/", + "names": ["o200k_base.tiktoken", "cl100k_base.tiktoken"], + "why": "Harmony's tokenizer reads these from the directory TIKTOKEN_ENCODINGS_BASE names. Without them the server starts healthy and fails every request with \"invalid tiktoken vocab file\"." + } + ], + "args": [ + { + "flag": "--tool-call-parser", + "values": ["openai", "openai_gptoss", "gpt_oss"], + "with": ["--enable-auto-tool-choice"], + "why": "This model reports tool calls in its own format, and without a parser for it vLLM returns an empty message instead, which looks like the model ignoring the request. Measured on a DGX Spark serving gpt-oss-120b, where a tool request generated 47 tokens and returned nothing." + }, + { + "flag": "--reasoning-parser", + "values": ["openai_gptoss", "gpt_oss", "openai"], + "why": "This separates the model's thinking from its answer." + } + ] + } + } +} diff --git a/catalogue/model-types/qwen3_5.json b/catalogue/model-types/qwen3_5.json new file mode 100644 index 0000000..bede09b --- /dev/null +++ b/catalogue/model-types/qwen3_5.json @@ -0,0 +1,18 @@ +{ + "schema_version": 1, + "id": "qwen3_5", + "display_name": "Qwen3.5", + "why": "Qwen3.5 and models built on it (Ornith among them) ship a chat template that raises an error on a system message after the first turn.", + "engines": { + "vllm": { + "chat_template_fixes": [ + { + "contains": "System message must be at the beginning", + "match": "(\\{%-\\s*if\\s+not\\s+loop\\.first\\s*%\\}\\s*)\\{\\{-\\s*raise_exception\\('System message must be at the beginning\\.'\\)\\s*\\}\\}(\\s*\\{%-\\s*endif\\s*%\\})", + "replace": "${1}{# llm-helper: dropped instead of raising #}${2}", + "why": "This model's bundled template crashes if a system-role message isn't first in the conversation, which agent harnesses like Coder Agents send. The patched copy drops a misplaced system message instead." + } + ] + } + } +} diff --git a/catalogue/model-types/qwen3_5_moe.json b/catalogue/model-types/qwen3_5_moe.json new file mode 100644 index 0000000..2a48bc8 --- /dev/null +++ b/catalogue/model-types/qwen3_5_moe.json @@ -0,0 +1,18 @@ +{ + "schema_version": 1, + "id": "qwen3_5_moe", + "display_name": "Qwen3.5 MoE", + "why": "Qwen3.5 and models built on it (Ornith among them) ship a chat template that raises an error on a system message after the first turn.", + "engines": { + "vllm": { + "chat_template_fixes": [ + { + "contains": "System message must be at the beginning", + "match": "(\\{%-\\s*if\\s+not\\s+loop\\.first\\s*%\\}\\s*)\\{\\{-\\s*raise_exception\\('System message must be at the beginning\\.'\\)\\s*\\}\\}(\\s*\\{%-\\s*endif\\s*%\\})", + "replace": "${1}{# llm-helper: dropped instead of raising #}${2}", + "why": "This model's bundled template crashes if a system-role message isn't first in the conversation, which agent harnesses like Coder Agents send. The patched copy drops a misplaced system message instead." + } + ] + } + } +} diff --git a/docs/install.md b/docs/install.md index b44b91d..a2cefbb 100644 --- a/docs/install.md +++ b/docs/install.md @@ -46,7 +46,6 @@ See [Hardware detection](./hardware.md). | `MODEL_BUDGET_GB` | config.env | Overrides the measured model budget. See [Memory budget](./memory.md). | | `KV_CONTEXT_TARGET` | config.env | How many full-length requests each profile's KV cache is sized to hold. Default `4`. | | `METAL_WIRED_LIMIT_MB` | config.env | macOS only. GPU wired memory limit in MiB, applied at boot. Written by `llm-helper metal-limit --set`. See [Running on macOS](./macos.md). | -| `TIKTOKEN_ENCODINGS_BASE` | config.env | Where gpt-oss-style models find their tiktoken vocab files. | | `UPDATE_CHANNEL` | config.env | Where `upgrade` looks for builds: `stable` or `dev`. Written when you pass `--channel`. | | `CODER_URL` | config.env | Coder deployment to sync with. See [Coder integration](./coder.md). | | `CODER_AI_PROVIDER_ID` | config.env | The `openai-compat` provider to hang model entries off. | diff --git a/docs/manifest.json b/docs/manifest.json index bd3a973..ccb2f96 100644 --- a/docs/manifest.json +++ b/docs/manifest.json @@ -31,6 +31,11 @@ "description": "Finding a model that fits this machine, and choosing a quant.", "path": "./models.md" }, + { + "title": "Model types", + "description": "What a model's architecture needs to run correctly, and how to add or change it.", + "path": "./model-types.md" + }, { "title": "Memory budget", "description": "How llm-helper decides what this machine can hold, and how to override it.", @@ -155,6 +160,23 @@ "description": "Show or raise how much unified memory macOS lets the GPU use", "path": "./reference/cli/metal-limit.md" }, + { + "title": "llm-helper model-types", + "description": "List what specific model architectures need to run correctly", + "path": "./reference/cli/model-types.md", + "children": [ + { + "title": "llm-helper model-types list", + "description": "List the model types in effect", + "path": "./reference/cli/model-types/list.md" + }, + { + "title": "llm-helper model-types show", + "description": "Show a model type's definition", + "path": "./reference/cli/model-types/show.md" + } + ] + }, { "title": "llm-helper new", "description": "Pull a new HuggingFace model, save it as a profile, and switch to it", diff --git a/docs/model-types.md b/docs/model-types.md new file mode 100644 index 0000000..ee31b0c --- /dev/null +++ b/docs/model-types.md @@ -0,0 +1,69 @@ +--- +title: Model types +description: What a model's architecture needs to run correctly, and how to add or change it. +--- + +Some architectures need more than their model card says. +gpt-oss needs vocab files on disk and parsers for its harmony format, and Qwen3.5's chat template raises an error on a system message after the first turn. +llm-helper keeps this as data, one file per HuggingFace `model_type` in [`catalogue/model-types/`](https://github.com/coder/llm-helper/tree/main/catalogue/model-types). +`new` and the panel read the repo's `config.json`, find the file for its `model_type`, and apply what it says for the engine the profile runs on. +A GGUF repo is matched by the `model_type` of the model it was quantized from. + +A model type applies to every repo of that architecture, including finetunes and re-uploads. +A launch configuration for one specific repo, run and measured on named hardware, is a [recipe](./recipes.md). + +`llm-helper model-types list` shows the model types in effect, and `llm-helper model-types show ` shows what one does. + +## Fields + +```json +{ + "schema_version": 1, + "id": "gpt_oss", + "display_name": "gpt-oss", + "why": "Why this architecture needs anything at all.", + "engines": { + "vllm": { + "files": [ + { + "dir": "tiktoken-encodings", + "env": "TIKTOKEN_ENCODINGS_BASE", + "base_url": "https://openaipublic.blob.core.windows.net/encodings/", + "names": ["o200k_base.tiktoken", "cl100k_base.tiktoken"], + "why": "What fails without them." + } + ], + "args": [ + { + "flag": "--tool-call-parser", + "values": ["openai", "openai_gptoss"], + "with": ["--enable-auto-tool-choice"], + "why": "What fails without it." + } + ], + "chat_template_fixes": [ + { + "contains": "text that marks the broken template", + "match": "a regular expression", + "replace": "its replacement, with ${1} for groups", + "why": "What fails without it." + } + ] + } + } +} +``` + +- `id` is the `model_type` value, and the file is named after it. +- `engines` is keyed by engine id. An engine with no entry gets nothing added. +- `files` are fetched into `dir` under the data directory when a profile is created, unless already there. With `env` set, the profile gets that variable pointing at the directory. +- `args` are added to the profile's `EXTRA_ARGS` unless the card already named the flag. `values` are tried in order and the first the installed engine registers is used; if none is registered, the flag is left out. `with` flags are added alongside. +- `chat_template_fixes` apply when the repo's `chat_template.jinja` contains `contains`. The patched copy is saved under the data directory and passed with `--chat-template`. If the text is there but `match` does not match, nothing is patched and `new` says so. + +Each file is checked when it loads: an unknown field, an unknown engine, a bad regular expression, a `dir` outside the data directory or a `base_url` that is not https is refused. + +## Changing one locally + +Put a file in `~/.config/vllm-helper/model-types.d/`, named after its `model_type`. +A new id adds a model type, and a shipped id replaces the shipped file, so you can change what llm-helper does for an architecture without rebuilding. +A file that fails to load is skipped, and `doctor` and `model-types list` say why. diff --git a/docs/reference/cli.md b/docs/reference/cli.md index 794f753..cf05aec 100644 --- a/docs/reference/cli.md +++ b/docs/reference/cli.md @@ -36,6 +36,7 @@ loaded at a time. | [`list`](cli/list.md) | List saved model profiles | | [`logs`](cli/logs.md) | Show the server log, following by default on a terminal | | [`metal-limit`](cli/metal-limit.md) | Show or raise how much unified memory macOS lets the GPU use | +| [`model-types`](cli/model-types.md) | List what specific model architectures need to run correctly | | [`new`](cli/new.md) | Pull a new HuggingFace model, save it as a profile, and switch to it | | [`node`](cli/node.md) | Register machines by HTTP URL and SSH target | | [`pool`](cli/pool.md) | Run a router in front of replicas serving the same model | diff --git a/docs/reference/cli/model-types.md b/docs/reference/cli/model-types.md new file mode 100644 index 0000000..75bf5d7 --- /dev/null +++ b/docs/reference/cli/model-types.md @@ -0,0 +1,19 @@ +--- +# Code generated by go run ./scripts/gendocs. DO NOT EDIT. +title: "llm-helper model-types" +description: "List what specific model architectures need to run correctly" +--- + + + +A model type is the model_type in a HuggingFace repo's config.json, such as gpt_oss. +Its definition says what every repo of that type needs on each engine: files to +fetch, flags to add, chat template fixes. 'new' applies it to the profile it writes. +They ship in the binary, and a file in /model-types.d adds one or replaces +a shipped one with the same id, without a rebuild. + +## Usage + +```console +llm-helper model-types +``` diff --git a/docs/reference/cli/model-types/list.md b/docs/reference/cli/model-types/list.md new file mode 100644 index 0000000..156d055 --- /dev/null +++ b/docs/reference/cli/model-types/list.md @@ -0,0 +1,26 @@ +--- +# Code generated by go run ./scripts/gendocs. DO NOT EDIT. +title: "llm-helper model-types list" +description: "List the model types in effect" +--- + + + +List the model types in effect: the shipped ones, and any added or replaced by +files in /model-types.d, which are shown with the file they came from. + +## Usage + +```console +llm-helper model-types list [flags] +``` + +## Options + +### --json + +| | | +| ---- | ------ | +| Type | `bool` | + +print the model types as JSON diff --git a/docs/reference/cli/model-types/show.md b/docs/reference/cli/model-types/show.md new file mode 100644 index 0000000..55b0d1a --- /dev/null +++ b/docs/reference/cli/model-types/show.md @@ -0,0 +1,23 @@ +--- +# Code generated by go run ./scripts/gendocs. DO NOT EDIT. +title: "llm-helper model-types show" +description: "Show a model type's definition" +--- + + + +## Usage + +```console +llm-helper model-types show [flags] +``` + +## Options + +### --json + +| | | +| ---- | ------ | +| Type | `bool` | + +print the definition as JSON diff --git a/internal/cli/detectcmds.go b/internal/cli/detectcmds.go index 2b52c98..4d816b7 100644 --- a/internal/cli/detectcmds.go +++ b/internal/cli/detectcmds.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "os" - "path/filepath" "strings" "github.com/spf13/cobra" @@ -16,6 +15,7 @@ import ( "github.com/coder/llm-helper/internal/engine" "github.com/coder/llm-helper/internal/hardware" "github.com/coder/llm-helper/internal/hf" + "github.com/coder/llm-helper/internal/modeltype" "github.com/coder/llm-helper/internal/plugin" "github.com/coder/llm-helper/internal/profile" "github.com/coder/llm-helper/internal/recipe" @@ -57,9 +57,7 @@ type doctorConfig struct { ServiceCrashed bool `json:"service_crashed"` ServiceRestarts int `json:"service_restarts"` BudgetOverride string `json:"budget_override,omitempty"` - // HarmonyVocab reports whether the tiktoken vocab files are on disk. - HarmonyVocab bool `json:"harmony_vocab"` - CoderConfigured bool `json:"coder_sync_configured"` + CoderConfigured bool `json:"coder_sync_configured"` } type coderStatus struct { @@ -136,7 +134,6 @@ func gatherDoctor() doctorReport { ServiceCrashed: health.Crashed, ServiceRestarts: health.NRestarts, BudgetOverride: cfg.ModelBudgetGB(), - HarmonyVocab: hasHarmonyVocab(cfg.Dirs.Data), CoderConfigured: cfg.CoderConfigured(), } if prov, err := engine.Lookup(cfg.Provider()); err != nil { @@ -258,14 +255,6 @@ func gatherHFToken(cfg *config.Config) hfTokenStatus { return st } -// hasHarmonyVocab reports whether the tiktoken vocab files are present. Only -// the o200k one is checked: it is the one gpt-oss reads, and a directory with -// just the other is the same problem. -func hasHarmonyVocab(dataDir string) bool { - st, err := os.Stat(filepath.Join(dataDir, "tiktoken-encodings", "o200k_base.tiktoken")) - return err == nil && st.Size() > 0 -} - // hardwareWarnings is what doctor has to say about the machine itself: the // cases where the budget it prints is right by its own rules but would // still mislead someone choosing a model. @@ -280,6 +269,9 @@ func hardwareWarnings(hw detect.Hardware) []string { for _, err := range recipe.LocalErrors() { w = append(w, "a local recipe was skipped: "+err.Error()) } + for _, err := range modeltype.LocalErrors() { + w = append(w, "a local model type was skipped: "+err.Error()) + } w = append(w, overlapWarnings(hardware.Default(), hw)...) if hw.AllGPUsHidden() { w = append(w, fmt.Sprintf( @@ -537,11 +529,6 @@ func renderDoctor(r doctorReport, yes bool) { default: fmt.Printf(" hf token: OK -- authenticated as %q (%s)\n", r.HFToken.Account, r.HFToken.Source) } - if r.Config != nil && !r.Config.HarmonyVocab { - fmt.Println(" tiktoken: no vocab files yet -- gpt-oss-style models read them from") - fmt.Println(" disk, and without them requests fail after a healthy start.") - fmt.Println(" Pulling such a model fetches them.") - } fmt.Println() renderToolchain(r.Toolchain, yes) diff --git a/internal/cli/modeltypescmd.go b/internal/cli/modeltypescmd.go new file mode 100644 index 0000000..409cbdd --- /dev/null +++ b/internal/cli/modeltypescmd.go @@ -0,0 +1,150 @@ +package cli + +import ( + "fmt" + "io" + "os" + "path/filepath" + "slices" + "strings" + "text/tabwriter" + + "github.com/spf13/cobra" + + "github.com/coder/llm-helper/internal/config" + "github.com/coder/llm-helper/internal/modeltype" +) + +func newModelTypesCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "model-types", + Short: "List what specific model architectures need to run correctly", + Long: "A model type is the model_type in a HuggingFace repo's config.json, such as gpt_oss.\n" + + "Its definition says what every repo of that type needs on each engine: files to\n" + + "fetch, flags to add, chat template fixes. 'new' applies it to the profile it writes.\n" + + "They ship in the binary, and a file in /model-types.d adds one or replaces\n" + + "a shipped one with the same id, without a rebuild.", + GroupID: groupModels, + } + cmd.AddCommand(newModelTypesListCmd(), newModelTypesShowCmd()) + return cmd +} + +func newModelTypesListCmd() *cobra.Command { + var jsonOut bool + c := &cobra.Command{ + Use: "list", + Short: "List the model types in effect", + Long: "List the model types in effect: the shipped ones, and any added or replaced by\n" + + "files in /model-types.d, which are shown with the file they came from.", + Aliases: []string{"ls"}, + Args: cobra.NoArgs, + RunE: func(*cobra.Command, []string) error { + return runModelTypesList(os.Stdout, os.Stderr, jsonOut) + }, + } + c.Flags().BoolVar(&jsonOut, "json", false, "print the model types as JSON") + return c +} + +func newModelTypesShowCmd() *cobra.Command { + var jsonOut bool + c := &cobra.Command{ + Use: "show ", + Short: "Show a model type's definition", + Args: cobra.ExactArgs(1), + RunE: func(_ *cobra.Command, a []string) error { + return runModelTypesShow(os.Stdout, a[0], jsonOut) + }, + } + c.Flags().BoolVar(&jsonOut, "json", false, "print the definition as JSON") + return c +} + +// modelTypeRow is one model type as 'model-types list --json' prints it. +type modelTypeRow struct { + modeltype.Entry + // Source is the model-types.d file a local entry came from, or "shipped". + Source string `json:"source"` +} + +func modelTypeSource(e modeltype.Entry) string { + if e.Source == "" { + return "shipped" + } + return e.Source +} + +func engineIDs(e modeltype.Entry) []string { + ids := make([]string, 0, len(e.Engines)) + for id := range e.Engines { + ids = append(ids, id) + } + slices.Sort(ids) + return ids +} + +func runModelTypesList(out, errOut io.Writer, jsonOut bool) error { + for _, err := range modeltype.LocalErrors() { + fmt.Fprintf(errOut, "warning: skipped a local model type file: %v\n", err) + } + var rows []modelTypeRow + for _, e := range modeltype.Default().List() { + rows = append(rows, modelTypeRow{Entry: e, Source: modelTypeSource(e)}) + } + if jsonOut { + return printJSONTo(out, rows) + } + tw := tabwriter.NewWriter(out, 0, 0, 2, ' ', 0) + fmt.Fprintln(tw, "ID\tENGINES\tNAME\tSOURCE") + for _, r := range rows { + fmt.Fprintf(tw, "%s\t%s\t%s\t%s\n", r.ID, strings.Join(engineIDs(r.Entry), ","), r.DisplayName, r.Source) + } + if err := tw.Flush(); err != nil { + return err + } + fmt.Fprintf(out, "\nLocal model types go in %s.\n", filepath.Join(config.ResolveDirs().Config, modeltype.LocalDir)) + return nil +} + +func runModelTypesShow(out io.Writer, id string, jsonOut bool) error { + e, ok := modeltype.Default().Lookup(id) + if !ok { + var have []string + for _, e := range modeltype.Default().List() { + have = append(have, e.ID) + } + return fmt.Errorf("no model type %q (have: %s)", id, strings.Join(have, ", ")) + } + if jsonOut { + return printJSONTo(out, modelTypeRow{Entry: e, Source: modelTypeSource(e)}) + } + fmt.Fprintf(out, "%s (%s)\n\n", e.DisplayName, e.ID) + fmt.Fprintf(out, " %-14s %s\n\n", "source", modelTypeSource(e)) + fmt.Fprintln(out, e.Why) + for _, id := range engineIDs(e) { + r := e.Engines[id] + fmt.Fprintf(out, "\nOn %s\n", id) + for _, f := range r.Files { + envNote := "" + if f.Env != "" { + envNote = ", " + f.Env + " points at it" + } + fmt.Fprintf(out, " fetch %s from %s into /%s%s\n", strings.Join(f.Names, ", "), f.BaseURL, f.Dir, envNote) + } + for _, a := range r.Args { + arg := a.Flag + if len(a.Values) > 0 { + arg += " " + } + if len(a.With) > 0 { + arg += " with " + strings.Join(a.With, " ") + } + fmt.Fprintf(out, " add %s\n", arg) + } + for _, t := range r.ChatTemplateFixes { + fmt.Fprintf(out, " fix chat templates containing %q\n", t.Contains) + } + } + return nil +} diff --git a/internal/cli/root.go b/internal/cli/root.go index aa56692..b65a446 100644 --- a/internal/cli/root.go +++ b/internal/cli/root.go @@ -16,6 +16,7 @@ import ( "github.com/coder/llm-helper/internal/control" "github.com/coder/llm-helper/internal/engine" "github.com/coder/llm-helper/internal/hardware" + "github.com/coder/llm-helper/internal/modeltype" "github.com/coder/llm-helper/internal/plugin" "github.com/coder/llm-helper/internal/profile" "github.com/coder/llm-helper/internal/recipe" @@ -77,6 +78,7 @@ func newRootCmd() *cobra.Command { // Engines load after hardware, because a local engine may name a // vendor only a local hardware definition adds. engine.UseLocal(filepath.Join(config.ResolveDirs().Config, engine.LocalDir)) + modeltype.UseLocal(filepath.Join(config.ResolveDirs().Config, modeltype.LocalDir)) // Recipes load last, because a local recipe may run on a local // engine. recipe.UseLocal(filepath.Join(config.ResolveDirs().Config, recipe.LocalDir)) @@ -104,6 +106,7 @@ func newRootCmd() *cobra.Command { newNewCmd(), newRecipesCmd(), newEnginesCmd(), + newModelTypesCmd(), newSearchCmd(), newInspectCmd(), newInstallGGUFCmd(), diff --git a/internal/cli/root_internal_test.go b/internal/cli/root_internal_test.go index c49b346..0800fa7 100644 --- a/internal/cli/root_internal_test.go +++ b/internal/cli/root_internal_test.go @@ -14,7 +14,7 @@ func TestTheCommandSetIsPinned(t *testing.T) { // completion is not here either, because the root disables it. want := []string{ "adopt", "coder-sync", "doctor", "edit", "engines", "hardware", "inspect", "install-gguf", "list", "logs", "metal-limit", - "new", "node", "pool", "recipes", "remove", "restart", "search", "serve", "set", "setup", "shard", "spark", "start", + "model-types", "new", "node", "pool", "recipes", "remove", "restart", "search", "serve", "set", "setup", "shard", "spark", "start", "status", "stop", "switch", "upgrade", "version", } diff --git a/internal/cli/setup.go b/internal/cli/setup.go index 04ee20e..8404f25 100644 --- a/internal/cli/setup.go +++ b/internal/cli/setup.go @@ -81,7 +81,7 @@ func runSetup(providerName string) error { } fmt.Printf("\nBudget: ~%d GB of model weights (%s)\n\n", hw.BudgetGB, hw.BudgetReason) - for _, d := range []string{dirs.Config, dirs.Profiles, dirs.State, filepath.Join(dirs.Data, "tiktoken-encodings")} { + for _, d := range []string{dirs.Config, dirs.Profiles, dirs.State} { if err := os.MkdirAll(d, 0o755); err != nil { return err } @@ -118,9 +118,6 @@ SERVICE_NAME=%s # %s. Uncomment only to override that. #MODEL_BUDGET_GB=%d -# Where gpt-oss-style models find their tiktoken vocab files. -TIKTOKEN_ENCODINGS_BASE=%s - # How many full-length requests each profile's KV cache is sized to hold. # Defaults to %d. A property of the workload rather than the machine: lower it # on a single-user box, raise it for a shared endpoint. @@ -135,7 +132,7 @@ TIKTOKEN_ENCODINGS_BASE=%s #CODER_ORGANIZATION=default `, strings.Join(engine.ProcessNames(), ", "), prov.Name(), prov.BinName(), prov.BinKey(), bin, config.ServiceManager, config.DefaultServiceScope, config.DefaultServiceName, - hw.BudgetGB, hw.BudgetReason, hw.BudgetGB, filepath.Join(dirs.Data, "tiktoken-encodings"), + hw.BudgetGB, hw.BudgetReason, hw.BudgetGB, detect.DefaultContextTarget, detect.DefaultContextTarget) if err := os.WriteFile(dirs.ConfigFile(), []byte(content), 0o644); err != nil { return err diff --git a/internal/hf/hf.go b/internal/hf/hf.go index a5f897f..95a6545 100644 --- a/internal/hf/hf.go +++ b/internal/hf/hf.go @@ -777,53 +777,20 @@ func clampContext(value string, limit int) string { return value } -var systemMessagePositionRe = regexp.MustCompile( - `(\{%-\s*if\s+not\s+loop\.first\s*%\}\s*)` + - `\{\{-\s*raise_exception\('System message must be at the beginning\.'\)\s*\}\}` + - `(\s*\{%-\s*endif\s*%\})`, -) - -// DetectChatTemplatePatch fetches a model's bundled chat template and checks -// whether it hard-crashes on a system-role message that isn't first in the -// conversation (found for real in some Qwen3.5-derived templates -- Ornith -// and Qwen3.5-122B both have it, byte-for-byte identical check). Agent -// harnesses -- Coder Agents included -- routinely inject periodic system- -// role "reminder" messages mid-conversation, which trips this. No vLLM flag -// controls it; the only lever is --chat-template to override the -// whole file, so this returns a patched copy that drops a misplaced system -// message instead of crashing. -// -// ok is false (with empty patched and nil err) if the model's template -// doesn't have this issue -- the caller should just use the model's own -// bundled template as normal in that case, not add --chat-template at all. -func DetectChatTemplatePatch(ctx context.Context, repo string) (patched string, ok bool, err error) { +// ChatTemplate fetches a repo's chat_template.jinja. found is false when the +// repo has no separate template file. +func ChatTemplate(ctx context.Context, repo string) (text string, found bool, err error) { resp, err := get(ctx, baseURL+"/"+repo+"/raw/main/chat_template.jinja") if err != nil { return "", false, err } defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return "", false, nil // no separate template file -- nothing to patch + return "", false, nil } body, err := io.ReadAll(io.LimitReader(resp.Body, 2<<20)) if err != nil { return "", false, err } - return patchSystemMessageCheck(string(body)) -} - -// patchSystemMessageCheck is the pure (no I/O) half of -// DetectChatTemplatePatch, split out so the regex logic is unit-testable -// without mocking HTTP. -func patchSystemMessageCheck(text string) (patched string, ok bool, err error) { - if !strings.Contains(text, "System message must be at the beginning") { - return "", false, nil - } - result := systemMessagePositionRe.ReplaceAllString(text, "${1}{# llm-helper: dropped instead of raising #}${2}") - if result == text { - // The crash string is present but our regex didn't match its exact - // surrounding shape -- don't silently claim a fix that didn't apply. - return "", false, fmt.Errorf("found the crash string but this template's exact shape didn't match the patch regex -- patch by hand") - } - return result, true, nil + return string(body), true, nil } diff --git a/internal/hf/hf_internal_test.go b/internal/hf/hf_internal_test.go index a5bc2da..a581ed2 100644 --- a/internal/hf/hf_internal_test.go +++ b/internal/hf/hf_internal_test.go @@ -713,16 +713,14 @@ func TestWhoAmINon200(t *testing.T) { } } -func TestDetectChatTemplatePatch(t *testing.T) { +func TestChatTemplate(t *testing.T) { tests := []struct { name string file string - wantOK bool missing bool }{ - {name: "template that crashes on a late system message", file: "chat_template_broken.jinja", wantOK: true}, - {name: "template without the check", file: "chat_template_clean.jinja"}, - {name: "model with no separate template file", missing: true}, + {name: "repo with a template file", file: "chat_template_broken.jinja"}, + {name: "repo with no separate template file", missing: true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -731,93 +729,20 @@ func TestDetectChatTemplatePatch(t *testing.T) { } else { serveTestdata(t, map[string]string{"/" + repo + "/raw/main/chat_template.jinja": tt.file}) } - patched, ok, err := DetectChatTemplatePatch(context.Background(), repo) + text, found, err := ChatTemplate(context.Background(), repo) if err != nil { - t.Fatalf("DetectChatTemplatePatch: %v", err) + t.Fatalf("ChatTemplate: %v", err) } - if ok != tt.wantOK { - t.Fatalf("ok = %v, want %v", ok, tt.wantOK) + if found == tt.missing { + t.Fatalf("found = %v, want %v", found, !tt.missing) } - if !ok { - if patched != "" { - t.Errorf("patched = %q, want empty when there's nothing to fix", patched) - } - return - } - if strings.Contains(patched, "raise_exception") { - t.Error("the patched template still raises") - } - if !strings.Contains(patched, `message.role == "system"`) { - t.Error("the patch should leave the rest of the template alone") + if found && !strings.Contains(text, "raise_exception") { + t.Errorf("text = %q, want the template file", text) } }) } } -func TestPatchSystemMessageCheck(t *testing.T) { - t.Parallel() - - const withCheck = `{%- for message in messages %} - {%- set content = render_content(message.content, true)|trim %} - {%- if message.role == "system" %} - {%- if not loop.first %} - {{- raise_exception('System message must be at the beginning.') }} - {%- endif %} - {%- elif message.role == "user" %} - {{- '<|im_start|>' + message.role + '\n' + content + '<|im_end|>' + '\n' }} - {%- endif %} -{%- endfor %}` - - t.Run("patches the exact known shape", func(t *testing.T) { - t.Parallel() - patched, ok, err := patchSystemMessageCheck(withCheck) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if !ok { - t.Fatal("expected ok=true, got false") - } - if strings.Contains(patched, "raise_exception") { - t.Error("patched template still contains the raise_exception call") - } - if !strings.Contains(patched, `if not loop.first`) { - t.Error("patch should leave the surrounding if/endif guard intact") - } - if !strings.Contains(patched, `message.role == "system"`) { - t.Error("patch should leave unrelated template logic untouched") - } - }) - - t.Run("no-op when the check isn't present", func(t *testing.T) { - t.Parallel() - const clean = `{%- for message in messages %} - {{- message.content }} -{%- endfor %}` - patched, ok, err := patchSystemMessageCheck(clean) - if err != nil { - t.Fatalf("unexpected error: %v", err) - } - if ok { - t.Error("expected ok=false for a template without the issue") - } - if patched != "" { - t.Error("expected empty patched output when ok=false") - } - }) - - t.Run("errors instead of silently no-op when the string is present but the shape doesn't match", func(t *testing.T) { - t.Parallel() - const weirdShape = "System message must be at the beginning. (mentioned in a comment, not the real check)" - _, ok, err := patchSystemMessageCheck(weirdShape) - if ok { - t.Error("expected ok=false when the regex can't safely match") - } - if err == nil { - t.Error("expected an error flagging the unmatched shape, got nil") - } - }) -} - func TestClampContext(t *testing.T) { t.Parallel() diff --git a/internal/modeltype/local.go b/internal/modeltype/local.go new file mode 100644 index 0000000..3db554a --- /dev/null +++ b/internal/modeltype/local.go @@ -0,0 +1,77 @@ +package modeltype + +import ( + "errors" + "fmt" + "io/fs" + "os" + "path/filepath" + "slices" + "strings" + "sync" +) + +// LocalDir is the directory under the config directory that local model +// types are read from. +const LocalDir = "model-types.d" + +var ( + localMu sync.Mutex + current = shippedRegistry + localErrs []error +) + +// Default returns the model types in effect: the shipped ones, with any +// UseLocal loaded added or put in their place. +func Default() *Registry { + localMu.Lock() + defer localMu.Unlock() + return current +} + +// UseLocal loads every *.json file in dir, in name order, over the shipped +// model types. A file with a shipped id replaces that entry. A missing dir is +// not an error; a bad file is skipped and reported, and the files after it +// still load. +// +// Call it after engine.UseLocal, since a local file may name a local engine. +func UseLocal(dir string) []error { + reg := shippedRegistry + var errs []error + entries, err := os.ReadDir(dir) + if err != nil && !errors.Is(err, fs.ErrNotExist) { + errs = append(errs, fmt.Errorf("%s: %w", dir, err)) + } + var names []string + for _, e := range entries { + if !e.IsDir() && strings.HasSuffix(e.Name(), ".json") { + names = append(names, e.Name()) + } + } + slices.Sort(names) + for _, name := range names { + p := filepath.Join(dir, name) + data, err := os.ReadFile(p) + var e Entry + if err == nil { + e, err = parse(name, data) + } + if err != nil { + errs = append(errs, fmt.Errorf("%s: %w", p, err)) + continue + } + e.Source = p + reg = reg.with(e) + } + localMu.Lock() + defer localMu.Unlock() + current, localErrs = reg, errs + return errs +} + +// LocalErrors reports the problems the last UseLocal found. +func LocalErrors() []error { + localMu.Lock() + defer localMu.Unlock() + return slices.Clone(localErrs) +} diff --git a/internal/modeltype/modeltype.go b/internal/modeltype/modeltype.go new file mode 100644 index 0000000..6b115cf --- /dev/null +++ b/internal/modeltype/modeltype.go @@ -0,0 +1,259 @@ +// Package modeltype reads the model-type definitions in catalogue/model-types/: +// what every repo with a given HuggingFace model_type needs to run correctly +// on each engine. A machine owner can add or replace one without a rebuild by +// dropping a file into model-types.d in the config directory; see UseLocal. +package modeltype + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "io" + "io/fs" + "path" + "regexp" + "slices" + "strings" + + "github.com/coder/llm-helper/catalogue" + "github.com/coder/llm-helper/internal/engine" +) + +// SchemaVersion is the version of the file format this build reads. +const SchemaVersion = 1 + +// Entry is one model type. +type Entry struct { + // ID is the model_type value in a repo's config.json, and the file name. + ID string `json:"id"` + DisplayName string `json:"display_name"` + Why string `json:"why"` + // Engines holds what this model type needs on each engine, by engine id. + Engines map[string]Rules `json:"engines"` + // Source is the model-types.d file a local entry came from, and "" for + // a shipped one. + Source string `json:"-"` +} + +// Rules is what a model type needs when one engine serves it. +type Rules struct { + Files []FileSet `json:"files,omitempty"` + Args []Arg `json:"args,omitempty"` + ChatTemplateFixes []TemplateFix `json:"chat_template_fixes,omitempty"` +} + +// FileSet is files fetched into a directory under the data directory before +// the model is served. +type FileSet struct { + Dir string `json:"dir"` + // Env, when set, is put in the profile pointing at the directory. + Env string `json:"env,omitempty"` + BaseURL string `json:"base_url"` + Names []string `json:"names"` + Why string `json:"why"` +} + +// Arg is a flag added to a new profile unless it already names it. Values +// are tried in order and the first the installed engine accepts is used; with +// none the flag is added bare. With is added alongside it. +type Arg struct { + Flag string `json:"flag"` + Values []string `json:"values,omitempty"` + With []string `json:"with,omitempty"` + Why string `json:"why"` +} + +// TemplateFix patches a model's bundled chat template when it contains +// Contains, replacing Match (a regular expression) with Replace. +type TemplateFix struct { + Contains string `json:"contains"` + Match string `json:"match"` + Replace string `json:"replace"` + Why string `json:"why"` +} + +// Apply returns the patched template, or ok false when the template does not +// contain the text this fix is for. It is an error for the text to be there +// and the pattern not to match, so a fix is never claimed that did not apply. +func (f TemplateFix) Apply(template string) (patched string, ok bool, err error) { + if !strings.Contains(template, f.Contains) { + return "", false, nil + } + re, err := regexp.Compile(f.Match) + if err != nil { + return "", false, err + } + out := re.ReplaceAllString(template, f.Replace) + if out == template { + return "", false, fmt.Errorf("the template contains %q but not in the shape the fix matches; patch it by hand", f.Contains) + } + return out, true, nil +} + +// Registry is a set of validated entries. +type Registry struct { + entries map[string]Entry +} + +// Lookup returns the entry for a model_type. +func (r *Registry) Lookup(modelType string) (Entry, bool) { + e, ok := r.entries[modelType] + return e, ok +} + +// RulesFor returns what modelType needs on engineID, empty when nothing. +func (r *Registry) RulesFor(modelType, engineID string) Rules { + e, ok := r.Lookup(modelType) + if !ok { + return Rules{} + } + return e.Engines[engineID] +} + +// List returns every entry, sorted by id. +func (r *Registry) List() []Entry { + out := make([]Entry, 0, len(r.entries)) + for _, e := range r.entries { + out = append(out, e) + } + slices.SortFunc(out, func(a, b Entry) int { return strings.Compare(a.ID, b.ID) }) + return out +} + +// with returns a copy of r with e added, replacing any entry with its id. +func (r *Registry) with(e Entry) *Registry { + next := &Registry{entries: make(map[string]Entry, len(r.entries)+1)} + for id, old := range r.entries { + next.entries[id] = old + } + next.entries[e.ID] = e + return next +} + +// Load reads and validates every *.json file in fsys, each named after the +// id it defines. +func Load(fsys fs.FS) (*Registry, error) { + names, err := fs.Glob(fsys, "*.json") + if err != nil { + return nil, err + } + reg := &Registry{entries: map[string]Entry{}} + var errs []error + for _, name := range names { + data, err := fs.ReadFile(fsys, name) + var e Entry + if err == nil { + e, err = parse(name, data) + } + if err != nil { + errs = append(errs, fmt.Errorf("%s: %w", name, err)) + continue + } + reg = reg.with(e) + } + return reg, errors.Join(errs...) +} + +var ( + idPattern = regexp.MustCompile(`^[a-z0-9][a-z0-9_.-]*$`) + envPattern = regexp.MustCompile(`^[A-Z_][A-Z0-9_]*$`) + flagPattern = regexp.MustCompile(`^--?[A-Za-z0-9][A-Za-z0-9-]*$`) + namePattern = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`) +) + +// parse decodes one file strictly and checks it, so a typo fails when the +// file loads rather than when a model is pulled. +func parse(name string, data []byte) (Entry, error) { + var f struct { + SchemaVersion int `json:"schema_version"` + Entry + } + dec := json.NewDecoder(bytes.NewReader(data)) + dec.DisallowUnknownFields() + if err := dec.Decode(&f); err != nil { + return Entry{}, err + } + if _, err := dec.Token(); err != io.EOF { + return Entry{}, errors.New("unexpected data after the top-level object") + } + if f.SchemaVersion != SchemaVersion { + return Entry{}, fmt.Errorf("schema_version is %d; this llm-helper reads version %d", f.SchemaVersion, SchemaVersion) + } + e := f.Entry + if base := strings.TrimSuffix(path.Base(name), ".json"); e.ID != base { + return Entry{}, fmt.Errorf("the file is named %s.json but defines %q; name it after the model_type it describes", base, e.ID) + } + if !idPattern.MatchString(e.ID) { + return Entry{}, fmt.Errorf("id %q is not a model_type", e.ID) + } + if e.Why == "" { + return Entry{}, errors.New("why is empty") + } + if len(e.Engines) == 0 { + return Entry{}, errors.New("engines is empty") + } + for id, rules := range e.Engines { + if _, err := engine.Lookup(id); err != nil { + return Entry{}, fmt.Errorf("engines: %w", err) + } + if err := rules.check(); err != nil { + return Entry{}, fmt.Errorf("engines.%s: %w", id, err) + } + } + return e, nil +} + +func (r Rules) check() error { + for i, f := range r.Files { + switch { + case f.Dir == "" || path.IsAbs(f.Dir) || path.Clean(f.Dir) != f.Dir || strings.HasPrefix(f.Dir, ".."): + return fmt.Errorf("files[%d].dir %q must be a plain path under the data directory", i, f.Dir) + case f.Env != "" && !envPattern.MatchString(f.Env): + return fmt.Errorf("files[%d].env %q is not an environment variable name", i, f.Env) + case !strings.HasPrefix(f.BaseURL, "https://") || !strings.HasSuffix(f.BaseURL, "/"): + return fmt.Errorf("files[%d].base_url must be an https URL ending in /", i) + case len(f.Names) == 0: + return fmt.Errorf("files[%d].names is empty", i) + case f.Why == "": + return fmt.Errorf("files[%d].why is empty", i) + } + for _, n := range f.Names { + if !namePattern.MatchString(n) || n == "." || n == ".." { + return fmt.Errorf("files[%d].names: %q is not a file name", i, n) + } + } + } + for i, a := range r.Args { + if !flagPattern.MatchString(a.Flag) { + return fmt.Errorf("args[%d].flag %q is not a flag", i, a.Flag) + } + if a.Why == "" { + return fmt.Errorf("args[%d].why is empty", i) + } + for _, w := range a.With { + if !flagPattern.MatchString(w) { + return fmt.Errorf("args[%d].with: %q is not a flag", i, w) + } + } + } + for i, t := range r.ChatTemplateFixes { + if t.Contains == "" || t.Why == "" { + return fmt.Errorf("chat_template_fixes[%d] needs contains and why", i) + } + if _, err := regexp.Compile(t.Match); err != nil { + return fmt.Errorf("chat_template_fixes[%d].match: %w", i, err) + } + } + return nil +} + +var shippedRegistry = mustLoad(catalogue.ModelTypes) + +func mustLoad(fsys fs.FS) *Registry { + reg, err := Load(fsys) + if err != nil { + panic("catalogue/model-types: " + err.Error()) + } + return reg +} diff --git a/internal/modeltype/modeltype_internal_test.go b/internal/modeltype/modeltype_internal_test.go new file mode 100644 index 0000000..c4a2dc7 --- /dev/null +++ b/internal/modeltype/modeltype_internal_test.go @@ -0,0 +1,159 @@ +package modeltype + +import ( + "io/fs" + "os" + "path/filepath" + "strings" + "testing" + "testing/fstest" + + "github.com/coder/llm-helper/catalogue" +) + +func TestTheShippedModelTypesLoad(t *testing.T) { + reg, err := Load(catalogue.ModelTypes) + if err != nil { + t.Fatal(err) + } + if len(reg.List()) == 0 { + t.Fatal("no model types shipped") + } +} + +func readTemplate(t *testing.T, name string) string { + t.Helper() + b, err := os.ReadFile(filepath.Join("..", "hf", "testdata", name)) + if err != nil { + t.Fatal(err) + } + return string(b) +} + +func TestTheShippedQwen35FixPatchesItsTemplate(t *testing.T) { + for _, id := range []string{"qwen3_5", "qwen3_5_moe"} { + fixes := Default().RulesFor(id, "vllm").ChatTemplateFixes + if len(fixes) != 1 { + t.Fatalf("%s: %d fixes, want 1", id, len(fixes)) + } + patched, ok, err := fixes[0].Apply(readTemplate(t, "chat_template_broken.jinja")) + if err != nil || !ok { + t.Fatalf("%s: ok %v, err %v", id, ok, err) + } + if strings.Contains(patched, "raise_exception") { + t.Errorf("%s: the patched template still raises", id) + } + if !strings.Contains(patched, `message.role == "system"`) { + t.Errorf("%s: the rest of the template should be left alone", id) + } + if _, ok, err := fixes[0].Apply(readTemplate(t, "chat_template_clean.jinja")); ok || err != nil { + t.Errorf("%s: a clean template got ok %v, err %v", id, ok, err) + } + } +} + +// The text being there and the pattern not matching means the fix would be +// claimed without applying. +func TestAFixThatDoesNotMatchItsShapeIsAnError(t *testing.T) { + f := Default().RulesFor("qwen3_5", "vllm").ChatTemplateFixes[0] + if _, ok, err := f.Apply(f.Contains + " (mentioned in a comment)"); ok || err == nil { + t.Errorf("ok %v, err %v, want an error", ok, err) + } +} + +const good = `{"schema_version":1,"id":"toy","why":"w","engines":{"vllm":{"args":[{"flag":"--x","why":"w"}]}}}` + +func TestBadFilesAreRefused(t *testing.T) { + for name, body := range map[string]string{ + "misnamed": good, + "unknown field": `{"schema_version":1,"id":"unknown field","why":"w","engines":{},"extra":1}`, + "unknown engine": `{"schema_version":1,"id":"unknown engine","why":"w","engines":{"nope":{}}}`, + "bad regex": `{"schema_version":1,"id":"bad regex","why":"w","engines":{"vllm":{"chat_template_fixes":[{"contains":"c","match":"(","replace":"","why":"w"}]}}}`, + "escaping dir": `{"schema_version":1,"id":"escaping dir","why":"w","engines":{"vllm":{"files":[{"dir":"../x","base_url":"https://a/","names":["f"],"why":"w"}]}}}`, + "http url": `{"schema_version":1,"id":"http url","why":"w","engines":{"vllm":{"files":[{"dir":"x","base_url":"http://a/","names":["f"],"why":"w"}]}}}`, + "no why": `{"schema_version":1,"id":"no why","engines":{"vllm":{}}}`, + "wrong version": `{"schema_version":2,"id":"wrong version","why":"w","engines":{"vllm":{}}}`, + } { + fsys := fstest.MapFS{name + ".json": {Data: []byte(body)}} + if _, err := Load(fsys); err == nil { + t.Errorf("%s: loaded, want an error", name) + } + } +} + +func TestALocalFileAddsOrReplaces(t *testing.T) { + dir := t.TempDir() + replaced := `{"schema_version":1,"id":"gpt_oss","why":"local","engines":{"vllm":{}}}` + for name, body := range map[string]string{ + "toy.json": good, + "gpt_oss.json": replaced, + "broken.json": "{", + } { + if err := os.WriteFile(filepath.Join(dir, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + errs := UseLocal(dir) + t.Cleanup(func() { UseLocal(t.TempDir()) }) + if len(errs) != 1 || !strings.Contains(errs[0].Error(), "broken.json") { + t.Errorf("errs = %v, want only broken.json reported", errs) + } + if e, ok := Default().Lookup("toy"); !ok || e.Source != filepath.Join(dir, "toy.json") { + t.Errorf("toy = %+v, %v, want it added with its source", e, ok) + } + if e, _ := Default().Lookup("gpt_oss"); e.Why != "local" { + t.Errorf("gpt_oss why = %q, want the local file to replace the shipped one", e.Why) + } + if _, ok := Default().Lookup("qwen3_5"); !ok { + t.Error("a local file removed an unrelated shipped entry") + } +} + +// What a model type needs belongs in its catalogue file, so Go code must not +// name one or repeat what its file says. +func TestNoGoCodeRepeatsAShippedModelType(t *testing.T) { + reg, err := Load(catalogue.ModelTypes) + if err != nil { + t.Fatal(err) + } + var needles []string + for _, e := range reg.List() { + needles = append(needles, `"`+e.ID+`"`) + for _, r := range e.Engines { + for _, f := range r.Files { + needles = append(needles, f.Names...) + needles = append(needles, f.BaseURL) + if f.Env != "" { + needles = append(needles, f.Env) + } + } + for _, a := range r.Args { + for _, v := range a.Values { + needles = append(needles, `"`+v+`"`) + } + } + for _, x := range r.ChatTemplateFixes { + needles = append(needles, x.Contains) + } + } + } + root := filepath.Join("..", "..", "internal") + err = filepath.WalkDir(root, func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return err + } + b, err := os.ReadFile(path) + if err != nil { + return err + } + for _, n := range needles { + if strings.Contains(string(b), n) { + t.Errorf("%s contains %s, which belongs in catalogue/model-types", path, n) + } + } + return nil + }) + if err != nil { + t.Fatal(err) + } +} diff --git a/internal/profile/profile.go b/internal/profile/profile.go index a547eef..f03d723 100644 --- a/internal/profile/profile.go +++ b/internal/profile/profile.go @@ -167,6 +167,9 @@ type CreateOpts struct { // are left out: a profile for another server has no use for them. Empty // writes the vLLM profile this has always written. Provider string + // Env is KEY=value lines added to the profile, which reach the server's + // environment. + Env []string } // Create writes a new profile file. Fails if it already exists. @@ -192,6 +195,7 @@ func Create(dirs config.Dirs, opts CreateOpts) (*Profile, error) { content := fmt.Sprintf("# Status: %s (added via %s on %s)\nPROVIDER=%s\nMODEL=%s\nEXTRA_ARGS=%s\n", opts.Status, opts.Source, time.Now().Format("2006-01-02"), opts.Provider, opts.Repo, config.Quote(opts.ExtraArgs)) + content += envLines(opts.Env) if err := os.WriteFile(path, []byte(content), 0o644); err != nil { return nil, err } @@ -208,12 +212,22 @@ EXTRA_ARGS=%s `, opts.Status, opts.Source, time.Now().Format("2006-01-02"), opts.Repo, opts.MaxModelLen, opts.MemUtilization, kvCacheValue(opts.KVCacheBytes), config.Quote(opts.ExtraArgs)) + content += envLines(opts.Env) if err := os.WriteFile(path, []byte(content), 0o644); err != nil { return nil, err } return Load(dirs, name) } +func envLines(env []string) string { + var b strings.Builder + for _, kv := range env { + k, v, _ := strings.Cut(kv, "=") + b.WriteString(k + "=" + config.Quote(v) + "\n") + } + return b.String() +} + // DefaultMaxModelLen is the context a profile gets when the model card did not // say. Long enough to be useful, short of the 1M-token configs that would make // the KV cache the dominant cost on any machine. diff --git a/internal/resolve/modeltype.go b/internal/resolve/modeltype.go new file mode 100644 index 0000000..db1926c --- /dev/null +++ b/internal/resolve/modeltype.go @@ -0,0 +1,142 @@ +package resolve + +import ( + "context" + "fmt" + "io" + "net/http" + "os" + "path/filepath" + "strings" + "time" + + "github.com/coder/llm-helper/internal/config" + "github.com/coder/llm-helper/internal/hf" + "github.com/coder/llm-helper/internal/modeltype" + "github.com/coder/llm-helper/internal/vllm" +) + +var httpClient = &http.Client{Timeout: 5 * time.Minute} + +// modelTypeArgs returns the model type's flags this engine accepts that +// present does not already name, and a note for each. A flag or value the +// installed engine is not known to register is skipped, so an unreadable +// registry adds nothing. +func modelTypeArgs(args []modeltype.Arg, caps vllm.Caps, present string) (flags []string, notes []string) { + asked := map[string]bool{} + if split, err := config.SplitArgs(present); err == nil { + for _, a := range split { + name, _, _ := strings.Cut(a, "=") + asked[name] = true + } + } + for _, a := range args { + if asked[a.Flag] || !caps.HasFlag(a.Flag) { + continue + } + added := []string{a.Flag} + if len(a.Values) > 0 { + value := "" + for _, v := range a.Values { + if vllm.Accepts(caps, a.Flag, v) { + value = v + break + } + } + if value == "" { + continue + } + added = append(added, value) + } + flags = append(flags, added...) + said := strings.Join(added, " ") + for _, w := range a.With { + if !asked[w] && caps.HasFlag(w) { + flags = append(flags, w) + said += " and " + w + } + } + asked[a.Flag] = true + notes = append(notes, "Added "+said+". "+a.Why) + } + return flags, notes +} + +// modelTypeFiles fetches whatever the model type's files are missing and +// returns the profile lines that point at them, with a note for anything +// fetched or not fetched. +func modelTypeFiles(dataDir string, sets []modeltype.FileSet) (env []string, notes []string) { + for _, set := range sets { + dir := filepath.Join(dataDir, set.Dir) + if set.Env != "" { + env = append(env, set.Env+"="+dir) + } + var fetched []string + for _, name := range set.Names { + path := filepath.Join(dir, name) + if st, err := os.Stat(path); err == nil && st.Size() > 0 { + continue + } + if err := download(set.BaseURL+name, path); err != nil { + notes = append(notes, fmt.Sprintf("Fetching %s into %s failed (%v). %s", name, dir, err, set.Why)) + continue + } + fetched = append(fetched, name) + } + if len(fetched) > 0 { + notes = append(notes, fmt.Sprintf("Fetched %s into %s. %s", strings.Join(fetched, ", "), dir, set.Why)) + } + } + return env, notes +} + +// fixChatTemplate applies the first of fixes that matches the repo's bundled +// chat template, and returns the patched template. +func fixChatTemplate(ctx context.Context, repo string, fixes []modeltype.TemplateFix) (patched string, why string, err error) { + if len(fixes) == 0 { + return "", "", nil + } + text, found, err := hf.ChatTemplate(ctx, repo) + if err != nil || !found { + return "", "", err + } + for _, f := range fixes { + out, ok, err := f.Apply(text) + if err != nil { + return "", "", err + } + if ok { + return out, f.Why, nil + } + } + return "", "", nil +} + +func download(url, path string) error { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return err + } + resp, err := httpClient.Get(url) + if err != nil { + return err + } + defer resp.Body.Close() + if resp.StatusCode != http.StatusOK { + return fmt.Errorf("HTTP %d", resp.StatusCode) + } + // Written beside the target and renamed, so an interrupted fetch cannot + // leave a half file that later looks present. + tmp, err := os.CreateTemp(filepath.Dir(path), ".fetch-*") + if err != nil { + return err + } + defer os.Remove(tmp.Name()) + if _, err := io.Copy(tmp, resp.Body); err != nil { + tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmp.Name(), path) +} diff --git a/internal/resolve/modeltype_internal_test.go b/internal/resolve/modeltype_internal_test.go new file mode 100644 index 0000000..090ab56 --- /dev/null +++ b/internal/resolve/modeltype_internal_test.go @@ -0,0 +1,161 @@ +package resolve + +import ( + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/coder/llm-helper/internal/modeltype" + "github.com/coder/llm-helper/internal/vllm" +) + +type roundTripFunc func(*http.Request) (*http.Response, error) + +func (f roundTripFunc) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } + +// routeFetchesTo sends every file fetch to srv, whatever host it names. +func routeFetchesTo(t *testing.T, srv *httptest.Server) { + t.Helper() + target, err := url.Parse(srv.URL) + if err != nil { + t.Fatal(err) + } + old := httpClient + httpClient = &http.Client{Transport: roundTripFunc(func(r *http.Request) (*http.Response, error) { + r = r.Clone(r.Context()) + r.URL.Scheme, r.URL.Host = target.Scheme, target.Host + return http.DefaultTransport.RoundTrip(r) + })} + t.Cleanup(func() { httpClient = old }) +} + +var vocab = []modeltype.FileSet{{ + Dir: "tiktoken-encodings", + Env: "TIKTOKEN_ENCODINGS_BASE", + BaseURL: "https://example.invalid/encodings/", + Names: []string{"a.tiktoken", "b.tiktoken"}, + Why: "The tokenizer reads these from disk.", +}} + +func TestModelTypeFilesAreFetchedOnce(t *testing.T) { + var mu sync.Mutex + var asked []string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + mu.Lock() + asked = append(asked, r.URL.Path) + mu.Unlock() + w.Write([]byte("vocab bytes")) + })) + defer srv.Close() + routeFetchesTo(t, srv) + + data := t.TempDir() + env, notes := modelTypeFiles(data, vocab) + dir := filepath.Join(data, "tiktoken-encodings") + if len(env) != 1 || env[0] != "TIKTOKEN_ENCODINGS_BASE="+dir { + t.Errorf("env = %v, want the variable pointing at %s", env, dir) + } + if len(notes) != 1 || !strings.Contains(notes[0], "a.tiktoken, b.tiktoken") { + t.Errorf("notes = %v, want one naming both files", notes) + } + for _, name := range vocab[0].Names { + if _, err := os.Stat(filepath.Join(dir, name)); err != nil { + t.Errorf("%s was not written: %v", name, err) + } + } + if asked[0] != "/encodings/a.tiktoken" { + t.Errorf("fetched %s, want the base_url path", asked[0]) + } + + before := len(asked) + if _, notes := modelTypeFiles(data, vocab); len(notes) != 0 { + t.Errorf("refetched files already on disk: %v", notes) + } + if len(asked) != before { + t.Errorf("made %d more requests for files already present", len(asked)-before) + } +} + +func TestAFailedFileFetchIsReported(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, "nope", http.StatusInternalServerError) + })) + defer srv.Close() + routeFetchesTo(t, srv) + + _, notes := modelTypeFiles(t.TempDir(), vocab) + if len(notes) != 2 || !strings.Contains(notes[0], "failed") || !strings.Contains(notes[0], vocab[0].Why) { + t.Errorf("notes = %v, want each failure reported with the reason the file matters", notes) + } +} + +// spark is what a DGX Spark running vLLM 0.27.1 reports. +var spark = vllm.Caps{ + ToolParsers: []string{"hermes", "openai", "pythonic"}, + ReasoningParsers: []string{"deepseek_r1", "openai_gptoss", "qwen3"}, + Flags: []string{"--tool-call-parser", "--reasoning-parser", "--enable-auto-tool-choice"}, +} + +var harmonyArgs = []modeltype.Arg{ + {Flag: "--tool-call-parser", Values: []string{"openai", "openai_gptoss"}, With: []string{"--enable-auto-tool-choice"}, Why: "tools"}, + {Flag: "--reasoning-parser", Values: []string{"openai_gptoss", "gpt_oss"}, Why: "thinking"}, +} + +func TestModelTypeArgsFillGaps(t *testing.T) { + flags, notes := modelTypeArgs(harmonyArgs, spark, "") + got := strings.Join(flags, " ") + want := "--tool-call-parser openai --enable-auto-tool-choice --reasoning-parser openai_gptoss" + if got != want { + t.Errorf("flags = %q, want %q", got, want) + } + if len(notes) != 2 || !strings.HasPrefix(notes[0], "Added --tool-call-parser openai and --enable-auto-tool-choice. ") { + t.Errorf("notes = %v, want one per arg naming what was added", notes) + } +} + +// The card's author knows the model; a flag it already names is left alone. +func TestAnArgTheCardAlreadyNamedIsLeftAlone(t *testing.T) { + flags, _ := modelTypeArgs(harmonyArgs, spark, "--tool-call-parser hermes") + got := strings.Join(flags, " ") + if strings.Contains(got, "--tool-call-parser") { + t.Errorf("overrode the card's tool parser: %q", got) + } + if !strings.Contains(got, "--reasoning-parser openai_gptoss") { + t.Errorf("the gap that was left should still be filled: %q", got) + } +} + +// A profile naming a parser this vLLM lacks will not start at all. +func TestNoValueIsNamedThatThisEngineDoesNotRegister(t *testing.T) { + for _, caps := range []vllm.Caps{ + {}, + {Flags: []string{"--tool-call-parser", "--reasoning-parser"}}, + {ToolParsers: []string{"openai"}, ReasoningParsers: []string{"openai_gptoss"}}, + } { + if flags, _ := modelTypeArgs(harmonyArgs, caps, ""); len(flags) > 0 { + t.Errorf("named a parser without confirming it exists: %v (caps %+v)", flags, caps) + } + } +} + +func TestShippedGPTOSSRulesProduceTheHarmonyFlags(t *testing.T) { + rules := modeltype.Default().RulesFor("gpt_oss", "vllm") + flags, _ := modelTypeArgs(rules.Args, spark, "") + want := "--tool-call-parser openai --enable-auto-tool-choice --reasoning-parser openai_gptoss" + if got := strings.Join(flags, " "); got != want { + t.Errorf("flags = %q, want %q", got, want) + } + if len(rules.Files) != 1 || rules.Files[0].Env != "TIKTOKEN_ENCODINGS_BASE" { + t.Errorf("files = %+v, want the tiktoken vocab", rules.Files) + } + for _, other := range [][2]string{{"qwen3", "vllm"}, {"gpt_oss", "mlx"}} { + if r := modeltype.Default().RulesFor(other[0], other[1]); len(r.Args)+len(r.Files)+len(r.ChatTemplateFixes) > 0 { + t.Errorf("%s on %s has rules %+v, want none", other[0], other[1], r) + } + } +} diff --git a/internal/resolve/resolve.go b/internal/resolve/resolve.go index 4777f4a..ad4ceee 100644 --- a/internal/resolve/resolve.go +++ b/internal/resolve/resolve.go @@ -22,6 +22,7 @@ import ( "github.com/coder/llm-helper/internal/detect" "github.com/coder/llm-helper/internal/engine" "github.com/coder/llm-helper/internal/hf" + "github.com/coder/llm-helper/internal/modeltype" "github.com/coder/llm-helper/internal/profile" "github.com/coder/llm-helper/internal/vllm" ) @@ -310,7 +311,7 @@ type Plan struct { // provider names the model and leaves the rest to that server's defaults. func PrepareFor(ctx context.Context, dirs config.Dirs, prov engine.Provider, repo string, choice *Choice, caps vllm.Caps, contextTarget int) Plan { if prov.Capabilities().VLLMArgs { - return PrepareProfile(ctx, dirs, repo, choice, caps, contextTarget) + return PrepareProfile(ctx, dirs, prov.Name(), repo, choice, caps, contextTarget) } p := Plan{ Opts: profile.CreateOpts{ @@ -334,14 +335,14 @@ func PrepareFor(ctx context.Context, dirs config.Dirs, prov engine.Provider, rep return p } -// PrepareProfile works out how to serve a model this machine has never run: -// which flags its card prescribes, which of those the installed vLLM will -// take, and whether its chat template needs the local patch. +// PrepareProfile works out how to serve a model this machine has never run on +// engineID: which flags its card prescribes, which of those the installed vLLM +// will take, and what its model type's catalogue entry adds. // // Nothing here is fatal. Every check that fails degrades to a note, because // the alternative -- refusing to save a profile because the Hub was slow -- // helps nobody. -func PrepareProfile(ctx context.Context, dirs config.Dirs, repo string, choice *Choice, caps vllm.Caps, contextTarget int) Plan { +func PrepareProfile(ctx context.Context, dirs config.Dirs, engineID, repo string, choice *Choice, caps vllm.Caps, contextTarget int) Plan { var p Plan name := profile.NameFromRepo(repo) model := repo @@ -381,9 +382,12 @@ func PrepareProfile(ctx context.Context, dirs config.Dirs, repo string, choice * cardArgs, dropped := vllm.Check(cardArgs, caps) p.Notes = append(p.Notes, dropped...) - if patched, needsPatch, err := hf.DetectChatTemplatePatch(ctx, repo); err != nil { + arch, _ := hf.Config(ctx, configRepoFor(repo, choice)) + rules := modeltype.Default().RulesFor(arch.ModelType, engineID) + + if patched, why, err := fixChatTemplate(ctx, repo, rules.ChatTemplateFixes); err != nil { p.Notes = append(p.Notes, "Chat template check failed, continuing without it: "+err.Error()) - } else if needsPatch { + } else if patched != "" { tmplPath := filepath.Join(dirs.Data, "chat-templates", name+".jinja") switch { case os.MkdirAll(filepath.Dir(tmplPath), 0o755) != nil: @@ -391,10 +395,7 @@ func PrepareProfile(ctx context.Context, dirs config.Dirs, repo string, choice * case os.WriteFile(tmplPath, []byte(patched), 0o644) != nil: p.Notes = append(p.Notes, "Couldn't write the patched template, continuing without it.") default: - p.Notes = append(p.Notes, - "This model's bundled template crashes if a system-role message isn't first in "+ - "the conversation (common with agent harnesses like Coder Agents). Patched a "+ - "local copy: "+tmplPath) + p.Notes = append(p.Notes, why+" Patched a local copy: "+tmplPath) extraFlags = strings.TrimSpace(extraFlags + " --chat-template " + tmplPath) } } @@ -412,7 +413,6 @@ func PrepareProfile(ctx context.Context, dirs config.Dirs, repo string, choice * if maxLen == 0 { maxLen = profile.DefaultMaxModelLen } - arch, _ := hf.Config(ctx, configRepoFor(repo, choice)) cache, clamped := planCache(detect.Probe(), arch, weightBytes(ctx, repo, choice), maxLen, contextTarget) if clamped != maxLen { p.Opts.MaxModelLen = strconv.Itoa(clamped) @@ -429,18 +429,12 @@ func PrepareProfile(ctx context.Context, dirs config.Dirs, repo string, choice * p.Notes = append(p.Notes, cache.Note) } - // Harmony reads its vocab from disk rather than the model repo, and - // without it the server starts healthy and fails every request. - if needsHarmony(arch.ModelType) { - if note := ensureHarmonyEncodings(dirs.Data); note != "" { - p.Notes = append(p.Notes, note) - } - // Harmony is a wire format as well as a tokenizer: the parsers that - // read it are what turn the model's output into an answer. - if flags, notes := harmonyServeFlags(caps, p.Opts.ExtraArgs); len(flags) > 0 { - p.Opts.ExtraArgs = strings.TrimSpace(p.Opts.ExtraArgs + " " + strings.Join(flags, " ")) - p.Notes = append(p.Notes, notes...) - } + env, notes := modelTypeFiles(dirs.Data, rules.Files) + p.Opts.Env = env + p.Notes = append(p.Notes, notes...) + if flags, notes := modelTypeArgs(rules.Args, caps, p.Opts.ExtraArgs); len(flags) > 0 { + p.Opts.ExtraArgs = strings.TrimSpace(p.Opts.ExtraArgs + " " + strings.Join(flags, " ")) + p.Notes = append(p.Notes, notes...) } // Only the card's own flags decide this. What we added says nothing about // whether the author documented how to serve the model, which is what the diff --git a/internal/resolve/resolve_internal_test.go b/internal/resolve/resolve_internal_test.go index bdd8fb3..701863f 100644 --- a/internal/resolve/resolve_internal_test.go +++ b/internal/resolve/resolve_internal_test.go @@ -6,6 +6,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "sync" "testing" @@ -107,7 +109,7 @@ func TestPrepareProfileAppliesAQuantChoice(t *testing.T) { Variant: hf.Variant{Name: "m-Q8_0.gguf", Label: "Q8_0", GGUF: true}, BaseModel: "Qwen/Qwen3-32B", } - p := PrepareProfile(context.Background(), dirs, "bartowski/Qwen_Qwen3-32B-GGUF", choice, vllm.Caps{}, 0) + p := PrepareProfile(context.Background(), dirs, "vllm", "bartowski/Qwen_Qwen3-32B-GGUF", choice, vllm.Caps{}, 0) if want := "bartowski/Qwen_Qwen3-32B-GGUF:Q8_0"; p.Opts.Repo != want { t.Errorf("Repo = %q, want %q", p.Opts.Repo, want) @@ -122,7 +124,7 @@ func TestPrepareProfileAppliesAQuantChoice(t *testing.T) { } // No choice means the repo's default, unchanged. - plain := PrepareProfile(context.Background(), dirs, "Qwen/Qwen3-32B", nil, vllm.Caps{}, 0) + plain := PrepareProfile(context.Background(), dirs, "vllm", "Qwen/Qwen3-32B", nil, vllm.Caps{}, 0) if plain.Opts.Repo != "Qwen/Qwen3-32B" { t.Errorf("Repo = %q, want the bare repo when nothing was chosen", plain.Opts.Repo) } @@ -323,10 +325,10 @@ func TestAHarmonyProfileIsWrittenWithItsParsers(t *testing.T) { defer hf.SetBaseURLForTest(srv.URL)() // A harmony model also fetches its vocab, which must not reach the real // internet from a test. - defer swapEncodingsURL(srv.URL + "/encodings/")() + routeFetchesTo(t, srv) dirs := config.Dirs{Data: t.TempDir()} - p := PrepareProfile(context.Background(), dirs, "openai/gpt-oss-120b", nil, spark, 0) + p := PrepareProfile(context.Background(), dirs, "vllm", "openai/gpt-oss-120b", nil, spark, 0) for _, want := range []string{ "--tool-call-parser openai", "--enable-auto-tool-choice", @@ -339,9 +341,46 @@ func TestAHarmonyProfileIsWrittenWithItsParsers(t *testing.T) { // A model that does not speak harmony has no use for them, and naming a // parser that does not match the model is how #23 produced empty reasoning. - q := PrepareProfile(context.Background(), dirs, "Qwen/Qwen3-32B", nil, spark, 0) - if strings.Contains(q.Opts.ExtraArgs, "parser") { - t.Errorf("ExtraArgs = %q, want no parser flags on a model that isn't harmony", q.Opts.ExtraArgs) + if len(p.Opts.Env) != 1 || !strings.HasPrefix(p.Opts.Env[0], "TIKTOKEN_ENCODINGS_BASE="+dirs.Data) { + t.Errorf("Env = %v, want TIKTOKEN_ENCODINGS_BASE pointing under the data dir", p.Opts.Env) + } + + q := PrepareProfile(context.Background(), dirs, "vllm", "Qwen/Qwen3-32B", nil, spark, 0) + if strings.Contains(q.Opts.ExtraArgs, "parser") || len(q.Opts.Env) > 0 { + t.Errorf("ExtraArgs = %q, Env = %v, want nothing added to a model that isn't harmony", q.Opts.ExtraArgs, q.Opts.Env) + } +} + +// The template fix belongs to the model types whose templates have the bug. +// A repo of another type is not patched even with the same template text. +func TestTheChatTemplateFixFollowsTheModelType(t *testing.T) { + broken, err := os.ReadFile("../hf/testdata/chat_template_broken.jinja") + if err != nil { + t.Fatal(err) + } + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch { + case strings.HasSuffix(r.URL.Path, "config.json") && strings.Contains(r.URL.Path, "Qwen3.5"): + fmt.Fprint(w, `{"model_type":"qwen3_5_moe"}`) + case strings.HasSuffix(r.URL.Path, "config.json"): + fmt.Fprint(w, `{"model_type":"qwen3"}`) + case strings.HasSuffix(r.URL.Path, "chat_template.jinja"): + w.Write(broken) + default: + http.NotFound(w, r) + } + })) + defer srv.Close() + defer hf.SetBaseURLForTest(srv.URL)() + + dirs := config.Dirs{Data: t.TempDir()} + p := PrepareProfile(context.Background(), dirs, "vllm", "Qwen/Qwen3.5-35B-A3B", nil, vllm.Caps{}, 0) + if !strings.Contains(p.Opts.ExtraArgs, "--chat-template "+filepath.Join(dirs.Data, "chat-templates")) { + t.Fatalf("ExtraArgs = %q, want the patched template", p.Opts.ExtraArgs) + } + q := PrepareProfile(context.Background(), dirs, "vllm", "Qwen/Qwen3-32B", nil, vllm.Caps{}, 0) + if strings.Contains(q.Opts.ExtraArgs, "--chat-template") { + t.Errorf("ExtraArgs = %q, want no patch for a model type without the fix", q.Opts.ExtraArgs) } } @@ -375,7 +414,7 @@ func TestNotesDoNotCreditTheCardForOurOwnFlags(t *testing.T) { BaseModel: "Qwen/Qwen3-Coder-30B-A3B-Instruct", } // A repo with no README snippet, which is the usual case for GGUF. - p := PrepareProfile(context.Background(), dirs, "unsloth/Qwen3-Coder-30B-A3B-Instruct-GGUF", choice, vllm.Caps{}, 0) + p := PrepareProfile(context.Background(), dirs, "vllm", "unsloth/Qwen3-Coder-30B-A3B-Instruct-GGUF", choice, vllm.Caps{}, 0) // The flag still has to reach vLLM -- the plugin cannot start without it. if !strings.Contains(p.Opts.ExtraArgs, "--tokenizer Qwen/Qwen3-Coder-30B-A3B-Instruct") { @@ -755,7 +794,7 @@ func TestAPullWithoutAQuantChoiceStillMeasuresTheWeights(t *testing.T) { defer srv.Close() defer hf.SetBaseURLForTest(srv.URL)() - PrepareProfile(context.Background(), config.Dirs{Data: t.TempDir()}, "org/big-model", nil, vllm.Caps{}, 0) + PrepareProfile(context.Background(), config.Dirs{Data: t.TempDir()}, "vllm", "org/big-model", nil, vllm.Caps{}, 0) mu.Lock() defer mu.Unlock() diff --git a/internal/resolve/tiktoken.go b/internal/resolve/tiktoken.go deleted file mode 100644 index 0f5f413..0000000 --- a/internal/resolve/tiktoken.go +++ /dev/null @@ -1,155 +0,0 @@ -package resolve - -import ( - "fmt" - "io" - "net/http" - "os" - "path/filepath" - "strings" - "time" - - "github.com/coder/llm-helper/internal/config" - "github.com/coder/llm-helper/internal/vllm" -) - -// harmonyEncodings are the vocab files the harmony tokenizer reads, by the -// names it looks for under TIKTOKEN_ENCODINGS_BASE. -var harmonyEncodings = []string{"o200k_base.tiktoken", "cl100k_base.tiktoken"} - -var encodingsURL = "https://openaipublic.blob.core.windows.net/encodings/" - -// swapEncodingsURL points the fetch at a stand-in and returns a restore func. -func swapEncodingsURL(url string) func() { - old := encodingsURL - encodingsURL = url - return func() { encodingsURL = old } -} - -// needsHarmony reports whether a model tokenizes with harmony, which reads its -// vocab from disk rather than from the model repo. -func needsHarmony(modelType string) bool { return modelType == "gpt_oss" } - -// Parser names for harmony output, best first. They have moved between -// releases, so which one is real is a question for the installed vLLM rather -// than something to hardcode. -var ( - harmonyToolParsers = []string{"openai", "openai_gptoss", "gpt_oss"} - harmonyReasoningParsers = []string{"openai_gptoss", "gpt_oss", "openai"} -) - -// harmonyServeFlags reports the flags a harmony model needs before a client can -// use it, and says what it added. -// -// A harmony model emits its tool calls and its thinking as channels in its own -// format rather than as JSON. Without parsers for them vLLM has nothing to turn -// that into tool_calls, and drops the message: content "", tool_calls null, -// finish_reason "stop". Plain chat still answers normally, so it reads as the -// model ignoring the request rather than as a missing flag. Measured on a DGX -// Spark serving gpt-oss-120b, where a tool request generated 47 tokens and -// returned nothing at all. -// -// present is what has already been asked for. Anything named there wins: the -// card's author knows the model, and this is only filling a gap. -func harmonyServeFlags(caps vllm.Caps, present string) (flags []string, notes []string) { - // Every flag below is gated on this vLLM registering both the option and - // the parser name, so an unreadable registry adds nothing on its own and - // needs no separate guard. Guessing a name into a profile would trade a - // model that answers plainly for one that will not start. - asked := map[string]bool{} - if args, err := config.SplitArgs(present); err == nil { - for _, a := range args { - name, _, _ := strings.Cut(a, "=") - asked[name] = true - } - } - - pick := func(candidates []string, registered func(string) bool) string { - for _, name := range candidates { - if registered(name) { - return name - } - } - return "" - } - - if !asked["--tool-call-parser"] && caps.HasFlag("--tool-call-parser") { - if name := pick(harmonyToolParsers, caps.HasToolParser); name != "" { - flags = append(flags, "--tool-call-parser", name) - // vLLM refuses to start with a tool parser and no way to reach it. - if !asked["--enable-auto-tool-choice"] && caps.HasFlag("--enable-auto-tool-choice") { - flags = append(flags, "--enable-auto-tool-choice") - } - notes = append(notes, "Added --tool-call-parser "+name+" and --enable-auto-tool-choice. "+ - "This model reports tool calls in its own format, and without a parser for it vLLM "+ - "returns an empty message instead, which looks like the model ignoring the request.") - } - } - if !asked["--reasoning-parser"] && caps.HasFlag("--reasoning-parser") { - if name := pick(harmonyReasoningParsers, caps.HasReasoningParser); name != "" { - flags = append(flags, "--reasoning-parser", name) - notes = append(notes, "Added --reasoning-parser "+name+ - ", which separates this model's thinking from its answer.") - } - } - return flags, notes -} - -// ensureHarmonyEncodings downloads the vocab files if they are missing. -// -// setup creates this directory and points vLLM at it, but nothing filled it, so -// a gpt-oss model started healthy and then failed every request with "invalid -// tiktoken vocab file" -- a failure that looks like a broken model rather than -// a missing file. Returns a note for the user, empty when there was nothing to -// do or nothing useful to say. -func ensureHarmonyEncodings(dataDir string) string { - dir := filepath.Join(dataDir, "tiktoken-encodings") - var fetched []string - for _, name := range harmonyEncodings { - path := filepath.Join(dir, name) - if st, err := os.Stat(path); err == nil && st.Size() > 0 { - continue - } - if err := download(encodingsURL+name, path); err != nil { - return fmt.Sprintf( - "This model tokenizes with harmony, which needs %s in %s. "+ - "Fetching it failed (%v), and requests will return an error until it is there.", - name, dir, err) - } - fetched = append(fetched, name) - } - if len(fetched) == 0 { - return "" - } - return fmt.Sprintf("Fetched the harmony vocab files (%v) into %s -- this model reads them from disk.", fetched, dir) -} - -func download(url, path string) error { - if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { - return err - } - client := &http.Client{Timeout: 5 * time.Minute} - resp, err := client.Get(url) - if err != nil { - return err - } - defer resp.Body.Close() - if resp.StatusCode != http.StatusOK { - return fmt.Errorf("HTTP %d", resp.StatusCode) - } - // Written beside the target and renamed, so an interrupted fetch cannot - // leave a half file that later looks present and passes the size check. - tmp, err := os.CreateTemp(filepath.Dir(path), ".tiktoken-*") - if err != nil { - return err - } - defer os.Remove(tmp.Name()) - if _, err := io.Copy(tmp, resp.Body); err != nil { - tmp.Close() - return err - } - if err := tmp.Close(); err != nil { - return err - } - return os.Rename(tmp.Name(), path) -} diff --git a/internal/resolve/tiktoken_internal_test.go b/internal/resolve/tiktoken_internal_test.go deleted file mode 100644 index 1d68b90..0000000 --- a/internal/resolve/tiktoken_internal_test.go +++ /dev/null @@ -1,125 +0,0 @@ -package resolve - -import ( - "net/http" - "net/http/httptest" - "os" - "path/filepath" - "strings" - "testing" - - "github.com/coder/llm-helper/internal/vllm" -) - -// setup creates the encodings directory and points vLLM at it, but nothing -// filled it -- so a gpt-oss model started healthy and then failed every -// request with "invalid tiktoken vocab file". Measured on a GB10. -func TestHarmonyVocabIsFetchedWhenMissing(t *testing.T) { - var asked []string - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - asked = append(asked, strings.TrimPrefix(r.URL.Path, "/")) - w.Write([]byte("vocab bytes")) - })) - defer srv.Close() - defer swapEncodingsURL(srv.URL + "/")() - - data := t.TempDir() - if note := ensureHarmonyEncodings(data); note == "" { - t.Error("fetched the vocab files without saying so") - } - for _, name := range harmonyEncodings { - if _, err := os.Stat(filepath.Join(data, "tiktoken-encodings", name)); err != nil { - t.Errorf("%s was not written: %v", name, err) - } - } - - // A second pull must not refetch: these are several megabytes and do - // not change. - before := len(asked) - if note := ensureHarmonyEncodings(data); note != "" { - t.Errorf("refetched files already on disk: %q", note) - } - if len(asked) != before { - t.Errorf("made %d more requests for files already present", len(asked)-before) - } -} - -// A failed fetch has to say so: the server will start, look healthy, and then -// refuse every request. -func TestAFailedVocabFetchIsReported(t *testing.T) { - srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { - http.Error(w, "nope", http.StatusInternalServerError) - })) - defer srv.Close() - defer swapEncodingsURL(srv.URL + "/")() - - note := ensureHarmonyEncodings(t.TempDir()) - if !strings.Contains(note, "harmony") || !strings.Contains(note, "error") { - t.Errorf("note = %q, want it to name harmony and the failure", note) - } -} - -// Only gpt-oss-style models read a vocab from disk; nothing else should pay -// for the download. -func TestOnlyHarmonyModelsNeedTheVocab(t *testing.T) { - for arch, want := range map[string]bool{ - "gpt_oss": true, "qwen3": false, "llama": false, "": false, - } { - if got := needsHarmony(arch); got != want { - t.Errorf("needsHarmony(%q) = %v, want %v", arch, got, want) - } - } -} - -// spark is what a DGX Spark running vLLM 0.27.1 reports, which is where the -// empty-message failure was measured. -var spark = vllm.Caps{ - ToolParsers: []string{"hermes", "openai", "pythonic"}, - ReasoningParsers: []string{"deepseek_r1", "openai_gptoss", "qwen3"}, - Flags: []string{"--tool-call-parser", "--reasoning-parser", "--enable-auto-tool-choice"}, -} - -// Without these flags vLLM has no way to read a harmony model's tool calls, so -// it returns content "", tool_calls null and finish_reason "stop" -- 47 tokens -// generated and nothing handed back. Plain chat still answers, which is what -// makes it read as the model ignoring the request. -func TestAHarmonyModelGetsTheParsersThatMakeItUsable(t *testing.T) { - flags, notes := harmonyServeFlags(spark, "") - got := strings.Join(flags, " ") - want := "--tool-call-parser openai --enable-auto-tool-choice --reasoning-parser openai_gptoss" - if got != want { - t.Errorf("harmony flags = %q, want %q", got, want) - } - if len(notes) != 2 { - t.Errorf("got %d notes, want one per flag pair: %v", len(notes), notes) - } -} - -// The card's author knows the model. Filling a gap is help; overriding a -// deliberate choice is a different thing, and would also pass the same flag -// twice. -func TestAParserTheCardAlreadyNamedIsLeftAlone(t *testing.T) { - flags, _ := harmonyServeFlags(spark, "--tool-call-parser hermes") - got := strings.Join(flags, " ") - if strings.Contains(got, "--tool-call-parser") { - t.Errorf("overrode the card's tool parser: %q", got) - } - if !strings.Contains(got, "--reasoning-parser openai_gptoss") { - t.Errorf("the gap that was left should still be filled: %q", got) - } -} - -// A name is only worth writing if this vLLM registers it. Parser names move -// between releases, and a profile naming one that is gone will not start at -// all -- strictly worse than the empty answers it was meant to fix. -func TestNoParserIsNamedThatThisVLLMDoesNotRegister(t *testing.T) { - for _, caps := range []vllm.Caps{ - {}, // introspection failed: we could not ask - {Flags: []string{"--tool-call-parser", "--reasoning-parser"}}, // registries empty - {ToolParsers: []string{"openai"}, ReasoningParsers: []string{"openai_gptoss"}}, // flags unreadable - } { - if flags, _ := harmonyServeFlags(caps, ""); len(flags) > 0 { - t.Errorf("named a parser without confirming it exists: %v (caps %+v)", flags, caps) - } - } -} diff --git a/internal/vllm/validate.go b/internal/vllm/validate.go index 4aae029..6564482 100644 --- a/internal/vllm/validate.go +++ b/internal/vllm/validate.go @@ -14,6 +14,18 @@ var parserFlags = map[string]func(Caps, string) bool{ "--reasoning-parser": Caps.HasReasoningParser, } +// Accepts reports whether this vLLM is known to take flag with value: the +// flag is registered, and a parser flag's value names a registered parser. +func Accepts(caps Caps, flag, value string) bool { + if !caps.HasFlag(flag) { + return false + } + if has, ok := parserFlags[flag]; ok { + return has(caps, value) + } + return true +} + // pluginFlags load a parser from a file shipped alongside a model. The file // lives in the model's repo, not on this machine, so a path copied out of a // card points at nothing here and vLLM exits on the missing file. From faaff06329a94f50cc7af27e6ec62033df13b982 Mon Sep 17 00:00:00 2001 From: DevelopmentCats Date: Thu, 5 Nov 2026 19:43:46 +0000 Subject: [PATCH 2/3] model-types: add deepseek_v4 DeepSeek V4 Flash, Pro and Flash Vision all report model_type deepseek_v4 and ship no chat template; vLLM encodes their chats through its deepseek_v4 tokenizer mode. Their cards' vllm serve lines name neither that nor the deepseek_v4 parsers, so a profile from 'new' could not render a chat request. --- CHANGELOG.md | 4 +++ catalogue/model-types/deepseek_v4.json | 28 +++++++++++++++++++++ internal/cli/modeltypescmd.go | 6 ++++- internal/resolve/modeltype_internal_test.go | 13 ++++++++++ 4 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 catalogue/model-types/deepseek_v4.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cedbc0..adc47a4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ the fact. `llm-helper model-types list` and `show` print them, and a file in `model-types.d/` in the config directory adds one or replaces a shipped one without a rebuild. +- `new` on a DeepSeek V4 repo (`deepseek_v4`: Flash, Pro, Flash Vision) adds + `--tokenizer-mode deepseek_v4` and the `deepseek_v4` tool-call and reasoning + parsers. The repos ship no chat template, so a profile without the tokenizer + mode could not serve chat requests. ### Changed diff --git a/catalogue/model-types/deepseek_v4.json b/catalogue/model-types/deepseek_v4.json new file mode 100644 index 0000000..d789925 --- /dev/null +++ b/catalogue/model-types/deepseek_v4.json @@ -0,0 +1,28 @@ +{ + "schema_version": 1, + "id": "deepseek_v4", + "display_name": "DeepSeek V4", + "why": "DeepSeek V4 (Flash, Pro and the Flash Vision preview) ships no chat template. It encodes chats in Python, which vLLM carries as its deepseek_v4 tokenizer mode, and writes tool calls and thinking in its own format.", + "engines": { + "vllm": { + "args": [ + { + "flag": "--tokenizer-mode", + "values": ["deepseek_v4"], + "why": "The repos have no chat_template, so without this vLLM has no way to turn a chat request into a prompt." + }, + { + "flag": "--tool-call-parser", + "values": ["deepseek_v4"], + "with": ["--enable-auto-tool-choice"], + "why": "vLLM refuses requests that let the model choose a tool unless a tool-call parser is set." + }, + { + "flag": "--reasoning-parser", + "values": ["deepseek_v4"], + "why": "This separates the model's thinking from its answer." + } + ] + } + } +} diff --git a/internal/cli/modeltypescmd.go b/internal/cli/modeltypescmd.go index 409cbdd..e736d98 100644 --- a/internal/cli/modeltypescmd.go +++ b/internal/cli/modeltypescmd.go @@ -134,7 +134,11 @@ func runModelTypesShow(out io.Writer, id string, jsonOut bool) error { } for _, a := range r.Args { arg := a.Flag - if len(a.Values) > 0 { + switch len(a.Values) { + case 0: + case 1: + arg += " " + a.Values[0] + default: arg += " " } if len(a.With) > 0 { diff --git a/internal/resolve/modeltype_internal_test.go b/internal/resolve/modeltype_internal_test.go index 090ab56..19fba29 100644 --- a/internal/resolve/modeltype_internal_test.go +++ b/internal/resolve/modeltype_internal_test.go @@ -159,3 +159,16 @@ func TestShippedGPTOSSRulesProduceTheHarmonyFlags(t *testing.T) { } } } + +func TestShippedDeepSeekV4RulesProduceItsTokenizerAndParsers(t *testing.T) { + caps := vllm.Caps{ + ToolParsers: []string{"deepseek_v4"}, + ReasoningParsers: []string{"deepseek_v4"}, + Flags: []string{"--tokenizer-mode", "--tool-call-parser", "--reasoning-parser", "--enable-auto-tool-choice"}, + } + flags, _ := modelTypeArgs(modeltype.Default().RulesFor("deepseek_v4", "vllm").Args, caps, "--trust-remote-code") + want := "--tokenizer-mode deepseek_v4 --tool-call-parser deepseek_v4 --enable-auto-tool-choice --reasoning-parser deepseek_v4" + if got := strings.Join(flags, " "); got != want { + t.Errorf("flags = %q, want %q", got, want) + } +} From 41d13a6a5d92fbe0e4f42ab20050865cd2ea66ba Mon Sep 17 00:00:00 2001 From: DevelopmentCats Date: Thu, 5 Nov 2026 21:11:28 +0000 Subject: [PATCH 3/3] model-types: quote values, give files to every engine, sharpen load tests A catalogue value or a patched template path with a space in it split into two words when EXTRA_ARGS was read back, so both are quoted now. Model-type files applied only on engines with vllm_args, so a local definition giving mlx a file loaded and did nothing. Files now apply on every engine; args and chat_template_fixes are vLLM flag syntax and are refused at load for engines without vllm_args. Non-vLLM profiles still skip the Hub unless some model type has files for that engine. The bad-file fixtures had ids with spaces, so they all failed at the id check. They use valid ids now and each asserts its own error. --- docs/model-types.md | 4 +- internal/modeltype/modeltype.go | 6 ++- internal/modeltype/modeltype_internal_test.go | 26 ++++++------ internal/resolve/modeltype.go | 22 ++++++++-- internal/resolve/modeltype_internal_test.go | 12 ++++++ internal/resolve/resolve.go | 8 +++- internal/resolve/resolve_internal_test.go | 40 ++++++++++++++++++- 7 files changed, 95 insertions(+), 23 deletions(-) diff --git a/docs/model-types.md b/docs/model-types.md index ee31b0c..85263ef 100644 --- a/docs/model-types.md +++ b/docs/model-types.md @@ -55,12 +55,12 @@ A launch configuration for one specific repo, run and measured on named hardware ``` - `id` is the `model_type` value, and the file is named after it. -- `engines` is keyed by engine id. An engine with no entry gets nothing added. +- `engines` is keyed by engine id. An engine with no entry gets nothing added. `files` apply on any engine. `args` and `chat_template_fixes` are in vLLM's flag syntax, so they are only accepted for engines with the `vllm_args` capability. - `files` are fetched into `dir` under the data directory when a profile is created, unless already there. With `env` set, the profile gets that variable pointing at the directory. - `args` are added to the profile's `EXTRA_ARGS` unless the card already named the flag. `values` are tried in order and the first the installed engine registers is used; if none is registered, the flag is left out. `with` flags are added alongside. - `chat_template_fixes` apply when the repo's `chat_template.jinja` contains `contains`. The patched copy is saved under the data directory and passed with `--chat-template`. If the text is there but `match` does not match, nothing is patched and `new` says so. -Each file is checked when it loads: an unknown field, an unknown engine, a bad regular expression, a `dir` outside the data directory or a `base_url` that is not https is refused. +Each file is checked when it loads: an unknown field, an unknown engine, `args` or `chat_template_fixes` for an engine without `vllm_args`, a bad regular expression, a `dir` outside the data directory or a `base_url` that is not https is refused. ## Changing one locally diff --git a/internal/modeltype/modeltype.go b/internal/modeltype/modeltype.go index 6b115cf..c5a4a26 100644 --- a/internal/modeltype/modeltype.go +++ b/internal/modeltype/modeltype.go @@ -194,9 +194,13 @@ func parse(name string, data []byte) (Entry, error) { return Entry{}, errors.New("engines is empty") } for id, rules := range e.Engines { - if _, err := engine.Lookup(id); err != nil { + prov, err := engine.Lookup(id) + if err != nil { return Entry{}, fmt.Errorf("engines: %w", err) } + if !prov.Capabilities().VLLMArgs && (len(rules.Args) > 0 || len(rules.ChatTemplateFixes) > 0) { + return Entry{}, fmt.Errorf("engines.%s: args and chat_template_fixes apply only to engines with the vllm_args capability; %s takes files", id, id) + } if err := rules.check(); err != nil { return Entry{}, fmt.Errorf("engines.%s: %w", id, err) } diff --git a/internal/modeltype/modeltype_internal_test.go b/internal/modeltype/modeltype_internal_test.go index c4a2dc7..a57a1c8 100644 --- a/internal/modeltype/modeltype_internal_test.go +++ b/internal/modeltype/modeltype_internal_test.go @@ -64,19 +64,21 @@ func TestAFixThatDoesNotMatchItsShapeIsAnError(t *testing.T) { const good = `{"schema_version":1,"id":"toy","why":"w","engines":{"vllm":{"args":[{"flag":"--x","why":"w"}]}}}` func TestBadFilesAreRefused(t *testing.T) { - for name, body := range map[string]string{ - "misnamed": good, - "unknown field": `{"schema_version":1,"id":"unknown field","why":"w","engines":{},"extra":1}`, - "unknown engine": `{"schema_version":1,"id":"unknown engine","why":"w","engines":{"nope":{}}}`, - "bad regex": `{"schema_version":1,"id":"bad regex","why":"w","engines":{"vllm":{"chat_template_fixes":[{"contains":"c","match":"(","replace":"","why":"w"}]}}}`, - "escaping dir": `{"schema_version":1,"id":"escaping dir","why":"w","engines":{"vllm":{"files":[{"dir":"../x","base_url":"https://a/","names":["f"],"why":"w"}]}}}`, - "http url": `{"schema_version":1,"id":"http url","why":"w","engines":{"vllm":{"files":[{"dir":"x","base_url":"http://a/","names":["f"],"why":"w"}]}}}`, - "no why": `{"schema_version":1,"id":"no why","engines":{"vllm":{}}}`, - "wrong version": `{"schema_version":2,"id":"wrong version","why":"w","engines":{"vllm":{}}}`, + for name, c := range map[string]struct{ body, want string }{ + "misnamed": {good, "defines \"toy\""}, + "bad id": {`{"schema_version":1,"id":"bad id","why":"w","engines":{"vllm":{}}}`, "is not a model_type"}, + "unknown_field": {`{"schema_version":1,"id":"unknown_field","why":"w","engines":{},"extra":1}`, "unknown field"}, + "unknown_engine": {`{"schema_version":1,"id":"unknown_engine","why":"w","engines":{"nope":{}}}`, "unknown PROVIDER"}, + "mlx_args": {`{"schema_version":1,"id":"mlx_args","why":"w","engines":{"mlx":{"args":[{"flag":"--x","why":"w"}]}}}`, "vllm_args capability"}, + "bad_regex": {`{"schema_version":1,"id":"bad_regex","why":"w","engines":{"vllm":{"chat_template_fixes":[{"contains":"c","match":"(","replace":"","why":"w"}]}}}`, "match"}, + "escaping_dir": {`{"schema_version":1,"id":"escaping_dir","why":"w","engines":{"vllm":{"files":[{"dir":"../x","base_url":"https://a/","names":["f"],"why":"w"}]}}}`, "plain path"}, + "http_url": {`{"schema_version":1,"id":"http_url","why":"w","engines":{"vllm":{"files":[{"dir":"x","base_url":"http://a/","names":["f"],"why":"w"}]}}}`, "https URL"}, + "no_why": {`{"schema_version":1,"id":"no_why","engines":{"vllm":{}}}`, "why is empty"}, + "wrong_version": {`{"schema_version":2,"id":"wrong_version","why":"w","engines":{"vllm":{}}}`, "schema_version"}, } { - fsys := fstest.MapFS{name + ".json": {Data: []byte(body)}} - if _, err := Load(fsys); err == nil { - t.Errorf("%s: loaded, want an error", name) + fsys := fstest.MapFS{name + ".json": {Data: []byte(c.body)}} + if _, err := Load(fsys); err == nil || !strings.Contains(err.Error(), c.want) { + t.Errorf("%s: err = %v, want one mentioning %q", name, err, c.want) } } } diff --git a/internal/resolve/modeltype.go b/internal/resolve/modeltype.go index db1926c..75094a6 100644 --- a/internal/resolve/modeltype.go +++ b/internal/resolve/modeltype.go @@ -7,6 +7,7 @@ import ( "net/http" "os" "path/filepath" + "slices" "strings" "time" @@ -34,7 +35,7 @@ func modelTypeArgs(args []modeltype.Arg, caps vllm.Caps, present string) (flags if asked[a.Flag] || !caps.HasFlag(a.Flag) { continue } - added := []string{a.Flag} + said := a.Flag if len(a.Values) > 0 { value := "" for _, v := range a.Values { @@ -46,10 +47,11 @@ func modelTypeArgs(args []modeltype.Arg, caps vllm.Caps, present string) (flags if value == "" { continue } - added = append(added, value) + flags = append(flags, a.Flag, config.Quote(value)) + said += " " + value + } else { + flags = append(flags, a.Flag) } - flags = append(flags, added...) - said := strings.Join(added, " ") for _, w := range a.With { if !asked[w] && caps.HasFlag(w) { flags = append(flags, w) @@ -90,6 +92,18 @@ func modelTypeFiles(dataDir string, sets []modeltype.FileSet) (env []string, not return env, notes } +// otherEngineFiles applies model-type files for an engine that takes no +// vLLM flags. The Hub is asked for the model_type only when some model type +// has files for this engine. +func otherEngineFiles(ctx context.Context, dataDir, engineID, configRepo string) (env []string, notes []string) { + reg := modeltype.Default() + if !slices.ContainsFunc(reg.List(), func(e modeltype.Entry) bool { return len(e.Engines[engineID].Files) > 0 }) { + return nil, nil + } + arch, _ := hf.Config(ctx, configRepo) + return modelTypeFiles(dataDir, reg.RulesFor(arch.ModelType, engineID).Files) +} + // fixChatTemplate applies the first of fixes that matches the repo's bundled // chat template, and returns the patched template. func fixChatTemplate(ctx context.Context, repo string, fixes []modeltype.TemplateFix) (patched string, why string, err error) { diff --git a/internal/resolve/modeltype_internal_test.go b/internal/resolve/modeltype_internal_test.go index 19fba29..e10fe7d 100644 --- a/internal/resolve/modeltype_internal_test.go +++ b/internal/resolve/modeltype_internal_test.go @@ -6,10 +6,12 @@ import ( "net/url" "os" "path/filepath" + "slices" "strings" "sync" "testing" + "github.com/coder/llm-helper/internal/config" "github.com/coder/llm-helper/internal/modeltype" "github.com/coder/llm-helper/internal/vllm" ) @@ -172,3 +174,13 @@ func TestShippedDeepSeekV4RulesProduceItsTokenizerAndParsers(t *testing.T) { t.Errorf("flags = %q, want %q", got, want) } } + +func TestAValueWithSpacesSurvivesTheProfile(t *testing.T) { + caps := vllm.Caps{Flags: []string{"--download-dir"}} + args := []modeltype.Arg{{Flag: "--download-dir", Values: []string{"/srv/Model Cache"}, Why: "w"}} + flags, _ := modelTypeArgs(args, caps, "") + got, err := config.SplitArgs(strings.Join(flags, " ")) + if err != nil || !slices.Equal(got, []string{"--download-dir", "/srv/Model Cache"}) { + t.Errorf("EXTRA_ARGS splits to %q (%v), want the value kept whole", got, err) + } +} diff --git a/internal/resolve/resolve.go b/internal/resolve/resolve.go index ad4ceee..ecd9780 100644 --- a/internal/resolve/resolve.go +++ b/internal/resolve/resolve.go @@ -308,7 +308,8 @@ type Plan struct { // PrepareFor is PrepareProfile for whichever provider will serve the model. // Everything PrepareProfile works out (card flags, parser checks, a sized KV // cache, the template patch) is in vLLM's terms, so a profile for any other -// provider names the model and leaves the rest to that server's defaults. +// provider names the model, gets its model type's files, and leaves the rest +// to that server's defaults. func PrepareFor(ctx context.Context, dirs config.Dirs, prov engine.Provider, repo string, choice *Choice, caps vllm.Caps, contextTarget int) Plan { if prov.Capabilities().VLLMArgs { return PrepareProfile(ctx, dirs, prov.Name(), repo, choice, caps, contextTarget) @@ -323,6 +324,9 @@ func PrepareFor(ctx context.Context, dirs config.Dirs, prov engine.Provider, rep Notes: []string{"Served by " + prov.Name() + ". The flags on a model card are vLLM's, so none were copied; " + "add any this server needs to EXTRA_ARGS."}, } + env, notes := otherEngineFiles(ctx, dirs.Data, prov.Name(), configRepoFor(repo, choice)) + p.Opts.Env = env + p.Notes = append(p.Notes, notes...) // Named the way PrepareProfile names a quant, so `new` and the panel // find the profile again under the name they derive. if choice != nil && prov.Capabilities().GGUFQuant { @@ -396,7 +400,7 @@ func PrepareProfile(ctx context.Context, dirs config.Dirs, engineID, repo string p.Notes = append(p.Notes, "Couldn't write the patched template, continuing without it.") default: p.Notes = append(p.Notes, why+" Patched a local copy: "+tmplPath) - extraFlags = strings.TrimSpace(extraFlags + " --chat-template " + tmplPath) + extraFlags = strings.TrimSpace(extraFlags + " --chat-template " + config.Quote(tmplPath)) } } diff --git a/internal/resolve/resolve_internal_test.go b/internal/resolve/resolve_internal_test.go index 701863f..53e4a25 100644 --- a/internal/resolve/resolve_internal_test.go +++ b/internal/resolve/resolve_internal_test.go @@ -17,6 +17,7 @@ import ( "github.com/coder/llm-helper/internal/detect" "github.com/coder/llm-helper/internal/engine" "github.com/coder/llm-helper/internal/hf" + "github.com/coder/llm-helper/internal/modeltype" "github.com/coder/llm-helper/internal/profile" "github.com/coder/llm-helper/internal/vllm" ) @@ -173,6 +174,41 @@ func TestPrepareForAnotherProviderNamesOnlyTheModel(t *testing.T) { } } +// Files are not in vLLM's terms, so a model type can give them to any engine. +func TestAnotherEngineGetsItsModelTypeFiles(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if strings.HasSuffix(r.URL.Path, "config.json") { + fmt.Fprint(w, `{"model_type":"toy_mlx"}`) + return + } + fmt.Fprint(w, "vocab") + })) + defer srv.Close() + defer hf.SetBaseURLForTest(srv.URL)() + routeFetchesTo(t, srv) + + local := t.TempDir() + def := `{"schema_version":1,"id":"toy_mlx","why":"w","engines":{"mlx":{"files":[{"dir":"toy","env":"TOY_DIR","base_url":"https://example.invalid/","names":["v.txt"],"why":"w"}]}}}` + if err := os.WriteFile(filepath.Join(local, "toy_mlx.json"), []byte(def), 0o644); err != nil { + t.Fatal(err) + } + if errs := modeltype.UseLocal(local); len(errs) > 0 { + t.Fatal(errs) + } + t.Cleanup(func() { modeltype.UseLocal(t.TempDir()) }) + + dirs := config.Dirs{Data: t.TempDir()} + mlx, _ := engine.Lookup(engine.MLX) + p := PrepareFor(context.Background(), dirs, mlx, "mlx-community/Toy-4bit", nil, vllm.Caps{}, 0) + want := "TOY_DIR=" + filepath.Join(dirs.Data, "toy") + if len(p.Opts.Env) != 1 || p.Opts.Env[0] != want { + t.Errorf("Env = %v, want [%s]", p.Opts.Env, want) + } + if b, err := os.ReadFile(filepath.Join(dirs.Data, "toy", "v.txt")); err != nil || string(b) != "vocab" { + t.Errorf("v.txt = %q, %v; want it fetched", b, err) + } +} + // llama.cpp loads one GGUF file out of a repo, including a split one that // vLLM's plugin cannot, and the profile is named and addressed the way a vLLM // quant profile is, so `new` finds it again under the name it derives. @@ -339,12 +375,12 @@ func TestAHarmonyProfileIsWrittenWithItsParsers(t *testing.T) { } } - // A model that does not speak harmony has no use for them, and naming a - // parser that does not match the model is how #23 produced empty reasoning. if len(p.Opts.Env) != 1 || !strings.HasPrefix(p.Opts.Env[0], "TIKTOKEN_ENCODINGS_BASE="+dirs.Data) { t.Errorf("Env = %v, want TIKTOKEN_ENCODINGS_BASE pointing under the data dir", p.Opts.Env) } + // A model that does not speak harmony has no use for them, and naming a + // parser that does not match the model is how #23 produced empty reasoning. q := PrepareProfile(context.Background(), dirs, "vllm", "Qwen/Qwen3-32B", nil, spark, 0) if strings.Contains(q.Opts.ExtraArgs, "parser") || len(q.Opts.Env) > 0 { t.Errorf("ExtraArgs = %q, Env = %v, want nothing added to a model that isn't harmony", q.Opts.ExtraArgs, q.Opts.Env)