diff --git a/CHANGELOG.md b/CHANGELOG.md index 887a5c138..fb584d53a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -138,7 +138,50 @@ from version 5.0.0 onward. Pre-fork releases (`1.x`–`4.2.0`) were authored by follows upstream's rename rather than papering over it; keeping the old names would leave the API describing a flag that no longer exists. +### Added +- **Wire-name registries with a declared contract, checked in CI against the real receiver.** The + three surfaces that leave this library as names on a wire — CLI options, request keys, trainer + configuration keys — are now enum constants (`args.ModelOption` + `args.ModelFlag`, + `parameters.RequestField`, `parameters.TrainingField`), each declaring the contract it satisfies. + The parameter base classes accept nothing else, so an undeclared name cannot reach the wire at + all. CMake extracts the declarations at configure time and three C++ test files feed them to the + actual receivers — llama.cpp's server argument parser, its completion-request schema, and the + trainer's own key list — on **every** platform, which is the only place these can be checked: the + request schema and the trainer both ignore an unknown key silently. `WireNameRegistryTest` checks + the other direction, that every declared constant is still reachable from a public builder method. + + This found the twelve dead names removed below. It also found one that a schema check alone could + not: a key exempted as "consumed by the OpenAI layer before the schema" is only proven *absent* + from the schema, which a key nothing reads at all satisfies equally well. That exemption now + additionally requires a reader upstream, and `chat_template` was the one key that had none. + ### Removed +- **Twelve builder methods that wrote a name no llama.cpp receiver reads — breaking, no + deprecation window.** Each one looked like configuration and behaved as a no-op, or worse: + + | Removed | Why | + |---|---| + | `InferenceParameters.withTfsZ` | `tfs_z` — upstream deleted the tail-free sampler | + | `InferenceParameters.withPenalizeNl` | `penalize_nl` — deleted upstream | + | `InferenceParameters.withPenaltyPrompt(String)` / `(int...)` | `penalty_prompt` — deleted upstream | + | `InferenceParameters.withUseChatTemplate` | `use_jinja` is a **server start** flag, never a request key | + | `InferenceParameters.withChatTemplate` | `chat_template` is a **load-time** option; the server only ever *emits* that name, in `/props` | + | `ModelParameters.setGrpAttnN` / `setGrpAttnW` | `--grp-attn-n`/`-w` exist in `arg.cpp` but are `set_examples()`-scoped away from the server, so the parser rejects them | + | `ModelParameters.enableDumpKvCache` | `--dump-kv-cache` — deleted upstream | + | `ModelParameters.setHfRepoV` / `setHfFileV` | `--hf-repo-v`/`--hf-file-v` — deleted upstream | + | `ModelParameters.enableMlock` / `disableMmap` | `--mlock`/`--no-mmap` — deleted at b10878 in favour of `--load-mode` | + + The two failure modes differ and neither was visible from Java. A dead **CLI** name is a hard parse + error, so `loadModel()` throws `"Failed to parse model parameters"` and the model does not load. A + dead **request** key is discarded by llama.cpp's schema without a word, so the parameter simply + stops having an effect. Either way the Java tests asserting the string mapping + (`hasKey("--mlock")`) stayed green. Replacements where one exists: `setLoadMode(LoadMode)` for the + last row, `ModelParameters.setChatTemplate(String)` for `withChatTemplate`, and `--jinja` at server + start for `withUseChatTemplate`. + + Deprecating was considered and rejected for the same reason as `enableFlashAttn` below: a method + that keeps writing a name nothing reads is a trap with a warning label on it. + - **`ModelParameters.enableFlashAttn()` and `ModelFlag.FLASH_ATTN` — breaking.** Both modelled `--flash-attn` as a valueless flag, which it has not been since b10273. Keeping either would leave the broken argv reachable: the method directly, the enum constant through the public @@ -148,6 +191,20 @@ from version 5.0.0 onward. Pre-fork releases (`1.x`–`4.2.0`) were authored by llama.cpp misparses is a trap with a warning label on it, and this is a major-version window. ### Fixed +- **A caller-supplied JSON fragment could inject sibling fields into a request body.** + `InferenceParameters` stored every value as a raw string and built the request by concatenating + `"key": value` pairs, so a fragment passed to `withJsonSchema` / `withResponseFormat` / + `withStreamOptions` / `withMessagesJson` / `withToolsJson` — anywhere a caller supplies JSON text — + could close its own object and append arbitrary keys. Duplicate keys resolve last-wins in the + native parser, so an injected `n_predict` or `grammar` silently beat the one the builder wrote. + The body is now built as a real JSON tree, and every stored value is parsed and required to be + **exactly one well-formed JSON value** at write time — a fragment with a trailing sibling is + rejected with the offending key named. `toString()` on a parameter object is now a redacted debug + view (keys only) and deliberately not valid JSON; use `toJson()` for the wire form. +- **The Android "LLM Service" app applied its chat-template override per request**, where llama.cpp + discarded it. It is now set at model load (`ModelParameters.setChatTemplate`), which is where + upstream reads it. Only the `CHAT_TEMPLATE` test hook set it, so no shipped UI path changed + behaviour — but the on-device test was proving less than it looked. - **A test pinned the broken argv shape as correct.** `ModelParametersExtendedTest`'s complex-combination case asserted a 9-token argv built with `enableFlashAttn()` — i.e. it encoded the valueless emission as the expected contract, which is why no gate ever flagged it. It now uses diff --git a/CLAUDE.md b/CLAUDE.md index 1a1121a49..3cd8299a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -644,6 +644,67 @@ siblings; why (and why the `DEPOT_TOKEN` org secret and the README "Build cache are kept jllama-only) is explained in the cross-repo status under "Deliberate non-parity": [`../workspace/crossrepostatus.md`](../workspace/crossrepostatus.md). +## Wire-name registries (CLI options, request keys, trainer keys) + +Three surfaces leave this library as names on a wire, and each is checked against the code that reads +them. The names are **enum constants**, not string literals, and each declares the contract it must +satisfy: + +| Registry | Receiver it is checked against | Contract kinds | Guard | +|---|---|---|---| +| `args.ModelFlag` + `args.ModelOption` | `common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options` | `SERVER_PARSER`, `PROJECT_PSEUDO` | `src/test/cpp/test_model_flags.cpp` | +| `parameters.RequestField` | `server_schema::make_llama_cmpl_schema(...)` | `SCHEMA`, `OAI_LAYER` | `src/test/cpp/test_wire_contracts.cpp` | +| `parameters.TrainingField` | `jllama_train::config_keys()` (`train_engine.h`) | — | `src/test/cpp/test_wire_contracts.cpp` | + +`cmake/extract-java-wire-names.cmake` reads the registry `.java` files at configure time and emits +`{name, contract}` pairs into generated headers; the C++ tests feed them to the receivers. It matches +**enum constant declarations only**, so prose and javadoc cannot contribute a name, and it fails the +configure when a registry declares a name twice or extracts implausibly few. For `OAI_LAYER` keys — +which by definition never reach the schema — it additionally sweeps upstream's own sources +(`tools/server/*.cpp` + `common/*.cpp`, globbed) for a *reader shape* (`json_value(x, "k", …)`, +`.contains("k")`, `.at("k")`) and emits the hit count, because the schema cannot vouch for them. + +**Why the failure modes differ, and why all three need a guard.** An unregistered CLI option is a hard +parse error — `loadModel()` throws `"Failed to parse model parameters"`, so the model does not load. +The other two are worse: llama.cpp's request schema discards an unknown key without a word, and +`train_engine.cpp` reads with `j.value(key, default)`, so a dead field simply stops having an effect. +Either way a Java test asserting the string mapping (`hasKey("--mlock")`) passes forever. + +**Rules when touching a registry:** + +1. **Adding a name** means adding a constant with its contract. There is no `put(String, ...)` to + bypass — that is the point. +2. **A name with no counterpart is deleted, never deprecated.** A method that writes a key the + receiver discards reads as configuration and behaves as a no-op. +3. **The exemption checks are inverted on purpose.** A `PROJECT_PSEUDO` / `OAI_LAYER` name cannot go + stale by outliving its constant. It *can* go stale the other way — upstream may later register a + name we exempted, hiding a real check — so the tests assert such a name is still unknown to the + receiver, and that the exempt set is non-empty (a generator that lost the contract column would + otherwise exempt everything). +4. **An `OAI_LAYER` name must additionally be read by *something* upstream.** Absence from the schema + is satisfied just as well by a key nothing reads at all, so the inverted check alone left a hole + the exact size of the problem — `chat_template` sat in it, written by a public builder method and + read by nobody (upstream's only occurrence of that name is the `/props` payload it *emits*). The + reader sweep above closes it. It is a source pattern, not the parser: it proves a key is read from + some body, not that this endpoint reads it. That is enough for the failure that occurred, a count + of zero. +5. **`WireNameRegistryTest` checks the other direction**: every declared constant must be reachable + from some public builder method (driven reflectively), names are unique across both CLI + registries, and every `OCP_OVERLY_CONCRETE_PARAMETER` suppression still names a real enum-valued + setter. + +`JsonParameters` additionally enforces that **every stored value is exactly one well-formed JSON +value**, checked on write. That is what stops a caller-supplied fragment +(`withJsonSchema`/`withResponseFormat`/`withStreamOptions`/`withMessagesJson`/`withToolsJson`) from +injecting sibling fields into a request body — a demonstrated defect, with duplicate keys resolving +last-wins in the native parser. Note `FAIL_ON_TRAILING_TOKENS` is load-bearing: plain `readTree` +parses the first value and ignores the rest, which would silently truncate such a fragment instead of +rejecting it. + +The full record — which names were dead when, the fork-point archaeology, and the injection +reproducer — is in +[`docs/history/parameter-wire-surface.md`](docs/history/parameter-wire-surface.md). + ## Local llama.cpp source patches (`patches/`) The fetched llama.cpp source is patched before it compiles, via a generic mechanism: @@ -1164,7 +1225,11 @@ If the local check passes (`BUILD SUCCESS`), the `mvn package` job in **Java layer** (`src/main/java/net/ladenthin/llama/`): - `LlamaModel` — Main API class (AutoCloseable). Wraps native context for inference, embeddings, re-ranking, and tokenization. - `TextToSpeech` — Separate AutoCloseable native type for speech synthesis over llama.cpp's upstream Qwen3-TTS pipeline (a backbone text GGUF + an mmproj GGUF bundling the speaker encoder, code predictor, and code2wav decoder); `synthesize(text)` returns a 24 kHz mono 16-bit WAV byte stream, with overloads for a cloned-voice speaker-reference clip and language. Native orchestration in `tts_engine.{h,cpp}` drives upstream's `mtmd_helper::gen_audio` streaming API directly (see "Qwen3-TTS via `mtmd_helper::gen_audio`" below) — there is nothing extracted or hand-copied from llama.cpp source; the in-memory WAV writer is `tts_wav.hpp`. -- `ModelParameters` / `InferenceParameters` — Builder-pattern parameter classes that serialize to JSON (extend `JsonParameters`) for passing to native code. +- `ModelParameters` / `InferenceParameters` — Builder-pattern parameter classes. Every wire name they can + emit is an enum constant carrying the contract it must satisfy (`args.ModelOption` + `args.ModelFlag` for + argv, `parameters.RequestField` for the request body), and the base classes accept nothing else — see + "Wire-name registries" below. `InferenceParameters.toJson()` renders the request body; its `toString()` + is a redacted debug view and deliberately not valid JSON. - `LlamaIterator` / `LlamaIterable` — Streaming generation via Java `Iterator`/`Iterable`. - `LlamaLoader` — Extracts the platform-specific native library from the JAR to a temp directory, or finds it on `java.library.path`. - `OSInfo` — Detects OS and architecture for library resolution. @@ -1395,7 +1460,10 @@ prompt clip is committed (`src/test/resources/audios/sample.wav`) but the audio no CI download — and `LlamaTrainerIntegrationTest`, whose `net.ladenthin.llama.train.model` property is set by no job and whose model is in no `models.csv` row. The trainer one matters more than it looks: `train_engine.cpp` carries the same `postprocess_cpu_params` pair as `tts_params.hpp`, so the -JVM-abort class of bug documented under "Qwen3-TTS" can regress there with no runnable guard. +JVM-abort class of bug documented under "Qwen3-TTS" can regress there with no runnable guard. Two +slices of it are now covered without a model — `test_tts_params.cpp` drives `build_train_params` and +`jllama::resolve_cpu_params`, and `test_wire_contracts.cpp` pins the configuration key set against +`TrainingField` — but the Java → JNI → native round trip itself still runs nowhere. The model set has a **single source of truth: `.github/models.csv`** (one `filename,url` row per model; `#` comments). Everything derives from it: the **`download-models`** job (ubuntu, `needs: startgate`) is the only place models are fetched from HuggingFace (one manifest-driven @@ -1460,9 +1528,10 @@ ctest --test-dir build --output-on-failure -R "ResultsToJson" | `src/test/cpp/test_tts_wav.cpp` | 2 | The in-memory WAV writer `pcm_to_wav16_bytes` in `tts_wav.hpp` (WAV header/payload + little-endian clamping) — our own code, not upstream. The Qwen3-TTS pipeline it pairs with (`mtmd_helper::gen_audio`) is entirely upstream-owned (no project-side DSP to unit-test here). The load path is additionally covered by `test_tts_params.cpp` (3 tests over `tts_params.hpp`'s `build_tts_params`, plus 2 pinning the upstream `-1` default it depends on), which pins the CPU-thread resolution whose absence used to crash the JVM on every platform — see the `TODO.md` entry for the mechanism. End-to-end coverage is `TtsIntegrationTest`, which is model-gated. | | `src/test/cpp/test_tts_params.cpp` | 13 | The **three** builders every hand-assembled `common_params` goes through: `build_tts_params` (`tts_params.hpp`), `build_train_params` (`train_params.hpp`) and the shared `jllama::resolve_cpu_params` (`cpu_params.hpp`). Each builder is guarded separately on purpose — testing the resolver alone does **not** cover its call sites, because `train_engine.cpp` is compiled into `jllama` only, never into `jllama_test`, and `LlamaTrainerIntegrationTest` is gated on `net.ladenthin.llama.train.model`, which no CI job sets. Without these the JVM-abort bug could regress in the trainer on every platform, unseen. | | `src/test/cpp/test_model_split.cpp` | 7 | The two `load_tensors()` split helpers that `patches/0012` extracts out of llama.cpp's `src/llama-model.cpp` — `llama_model_splits_normalize` (proportional split, single device, and the zero-sum case that used to produce NaN, **and the cancelling `--tensor-split` case** — `-ts 1,-1` reaches the identical line on any backend with no GPU memory pressure at all) and `llama_model_splits_select_device` (every layer maps to a real device index; malformed split points throw a message that names the function, the layer, the index and the split values instead of libc++'s bare `"vector"`). **This is the runnable guard for `0012`**: the patch also ships an upstream `tests/test-model-split.cpp`, but a FetchContent subproject builds with `LLAMA_BUILD_TESTS=OFF`, so that one is applied-but-never-compiled here. This file is the only place the two functions are linked in CI, on every platform — so a bump that drops the patch fails the `C++ Tests` build outright rather than resurfacing as one red macOS Java job. It is the one test file that includes an **internal** upstream header (`llama-model.h`, via the `${llama.cpp_SOURCE_DIR}/src` include dir added for it), which is deliberate: a signature drift should fail loudly at compile time. | -| `src/test/cpp/test_model_flags.cpp` | 4 | **The contract between the Java flag surface and llama.cpp's server argument parser.** CMake extracts every `"--flag"` literal `ModelFlag.java` + `ModelParameters.java` can emit (`cmake/extract-java-cli-flags.cmake` → a generated header), and this file asserts each one is in `common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options`. It exists because **no Java test can catch this class**: `ModelFlagTest`/`ModelParametersExtendedTest` pin the *string mapping* (`hasKey("--mlock")`), never that llama.cpp still accepts the string, so they stay green forever while the flag is dead — and `common_params_parse` treats an unregistered option as a hard error, so the affected builder method makes the model **unloadable**, not merely ineffective. **A grep over `arg.cpp` is not a substitute**: `--grp-attn-n`/`-w` are present there at every pinned tag but `set_examples()`-scoped to `LLAMA_EXAMPLE_COMPLETION`/`PASSKEY`, so the server parser rejects them exactly like a deleted flag — only the real option table sees that. `--vocab-only` is the one exemption (a project pseudo-flag `strip_flag_from_argv` removes before the parse); the exemption list is itself asserted to stay live. | +| `src/test/cpp/test_model_flags.cpp` | 4 | **The contract between the Java CLI-flag registries and llama.cpp's server argument parser.** CMake reads `ModelFlag.java` + `ModelOption.java` (`cmake/extract-java-wire-names.cmake` → a generated header of `{name, contract}` pairs), and this file asserts every `SERVER_PARSER` name is in `common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options`. It exists because **no Java test can catch this class**: `ModelFlagTest`/`ModelParametersExtendedTest` pin the *string mapping* (`hasKey("--mlock")`), never that llama.cpp still accepts the string, so they stay green forever while the flag is dead — and `common_params_parse` treats an unregistered option as a hard error, so the affected builder method makes the model **unloadable**, not merely ineffective. **A grep over `arg.cpp` is not a substitute**: `--grp-attn-n`/`-w` are present there at every pinned tag but `set_examples()`-scoped to `LLAMA_EXAMPLE_COMPLETION`/`PASSKEY`, so the server parser rejects them exactly like a deleted flag — only the real option table sees that. `--vocab-only` is the one exemption, and it declares itself `CliContract.PROJECT_PSEUDO` on its own constant rather than appearing in a list inside this file; the test asserts such a name is **still unknown** to the parser (an exemption upstream later registers would be hiding a real check) and that the exempt set is non-empty. | +| `src/test/cpp/test_wire_contracts.cpp` | 6 | **The same contract for the two quieter surfaces.** `RequestField` against `server_schema::make_llama_cmpl_schema(...)` (5 tests) and `TrainingField` against `jllama_train::config_keys()` (1 test). Both receivers *silently ignore* an unknown key — the schema skips it, `train_engine.cpp` reads with `j.value(key, default)` and falls back — so a dead field produces no error anywhere and every string-mapping test keeps passing. `OAI_LAYER`-declared keys (consumed by `oaicompat_*_params_parse` before the schema) are exempt from the schema check, and are checked **both** ways: still unknown to the schema (the inverted check), and read by at least one upstream reader-shaped site (the configure-time sweep — this is what caught `chat_template`, a key a public builder wrote and nothing read). See [`docs/history/parameter-wire-surface.md`](docs/history/parameter-wire-surface.md). | -**Current total: 531 tests (all passing).** +**Current total: 537 tests (all passing).** #### Upstream source location (in CMake build tree) @@ -1726,6 +1795,14 @@ rename or addition of an enum-valued `ModelParameters` setter needs that list up commit** — `setLoadMode` was added to it for exactly this reason — the same "FQN not updated after a rename" class as the stale PIT `targetClasses` and `CMakeLists.txt` OSInfo repairs. +**Half of that is now a test.** `WireNameRegistryTest.everyOcpSuppressionStillNamesAnEnumValuedSetter` +asserts every method named in those suppressions still exists as an enum-valued setter, which is the +half nothing else covers: a suppression for a method that no longer exists is silently inert, so the +next real finding on the renamed method arrives as a surprise. The opposite direction — a flagged +setter *missing* from the list — already reds `spotbugs:check`, and is not derivable by reflection +anyway: SpotBugs raises OCP only when a method uses nothing beyond the interface, so `setPoolingType` +(compares a concrete constant) and `withMiroStat` (calls `ordinal()`) are legitimately absent. + ## Spotless Formatting See [`../workspace/policies/spotless-formatting.md`](../workspace/policies/spotless-formatting.md). @@ -1755,6 +1832,12 @@ audio-fixture gotcha is resolved). `value` type needs its own test or the gate reds — the `ServerMetrics` counters added for the `getMetrics()` merge are covered by `ServerMetricsTest`. +**`parameters.JsonParameters` is on the gate too**, because it carries the one-JSON-value invariant +rather than plumbing. The rest of the `parameters` package is deliberately **not**: ~200 one-line +builder setters would add cost without signal. Getting `JsonParameters` to 100% needed one test more +than expected — the bounded excerpt in its rejection message is observable only *exactly* at the +limit, so it is pinned from both sides. + ## JPMS Module Descriptor This repo ships a `module-info.java` compiled in a separate `release 9` execution. Javadoc diff --git a/README.md b/README.md index 1913a0999..0667f269c 100644 --- a/README.md +++ b/README.md @@ -390,10 +390,9 @@ public class Example { System.out.print("Llama: "); prompt += "\nLlama: "; InferenceParameters inferParams = new InferenceParameters(prompt) - .setTemperature(0.7f) - .setPenalizeNl(true) - .setMiroStat(MiroStat.V2) - .setStopStrings("User:"); + .withTemperature(0.7f) + .withMiroStat(MiroStat.V2) + .withStopStrings("User:"); for (LlamaOutput output : model.generate(inferParams)) { System.out.print(output); prompt += output; @@ -443,9 +442,8 @@ just the text content of the assistant message. List> messages = new ArrayList<>(); messages.add(new Pair<>("user", "Write a haiku about Java.")); -InferenceParameters inferParams = new InferenceParameters("") - .setMessages("You are a helpful assistant.", messages) - .setUseChatTemplate(true); +InferenceParameters inferParams = + new InferenceParameters("").withMessages("You are a helpful assistant.", messages); try (LlamaModel model = new LlamaModel(modelParams)) { // Streaming @@ -588,7 +586,7 @@ unknown tool names are returned to the model as valid `{"error":"..."}` tool-res ### Infilling -You can simply set `InferenceParameters#setInputPrefix(String)` and `InferenceParameters#setInputSuffix(String)`. +You can simply set `InferenceParameters#withInputPrefix(String)` and `InferenceParameters#withInputSuffix(String)`. ### Embeddings & Reranking @@ -1036,8 +1034,8 @@ String grammar = """ expr ::= term ([-+*/] term)* term ::= [0-9]"""; InferenceParameters inferParams = new InferenceParameters("") - .setGrammar(grammar) - .setTemperature(0.8); + .withGrammar(grammar) + .withTemperature(0.8f); try (LlamaModel model = new LlamaModel(modelParams)) { model.generate(inferParams); } diff --git a/TODO.md b/TODO.md index 6242a6054..1f1305fa0 100644 --- a/TODO.md +++ b/TODO.md @@ -124,7 +124,8 @@ These are JNI plumbing items for upstream API additions. Policy: add only after - **Three upstream flags found by the b10878 flag audit, deliberately NOT implemented there.** The audit that produced `test_model_flags.cpp` swept every option `common/arg.cpp` registers for - `LLAMA_EXAMPLE_SERVER` against what `ModelParameters`/`ModelFlag` emit. Beyond the seven dead + `LLAMA_EXAMPLE_SERVER` against what the Java layer emits (now `args.ModelFlag` + `args.ModelOption`; + at the time of the audit, the string literals in `ModelParameters`). Beyond the seven dead flags it retired, it found ten option groups upstream had added since b10456 that the Java API does not expose. Seven were already covered (`--kv-unified-per-slot`, `--mmproj-device`/`-mmdev`, `--video-fps`, `--video-timestamp-interval`, `--video-ffmpeg-dir`, `--lazy-mode`/`-lzm`, @@ -139,13 +140,25 @@ These are JNI plumbing items for upstream API additions. Policy: add only after `format_log_as_json` — so the two would overlap and could contradict each other on the same stream. Deciding which layer owns the format is a **feature decision**, not a correctness fix, and needs its own change with its own tests. - - **`--spec-synth-len` and `--spec-synth-rates`** — upstream's own help text marks both as - benchmarking-only knobs for synthetic speculative-decoding measurements. No consumer use case - here; listed so a future audit does not re-discover them as an oversight. + - **`--spec-synth-len` and `--spec-synth-rates`** — a documented non-goal, not deferred work. The + reasoning lives in its own entry below (**"deliberately NOT exposed, and this should stay that + way"**); it is not repeated here. Nothing is broken by leaving these out: `NativeServer` forwards raw llama-server argv verbatim, so all three remain reachable that way. The gap is only in the typed `ModelParameters` surface. +- **Request-key exposure, measured rather than guessed.** With `parameters.RequestField` in place the + gap is countable instead of arguable. The Java layer writes **57** request keys (47 checked against + llama.cpp's completion-request schema, 10 consumed by the OpenAI layer ahead of it); upstream's + schema declares **68** primary fields, and **22** of those nothing here writes: `logprobs`, `lora`, + `response_fields`, `return_progress`, `n`, `echo`, `max_tokens`/`max_completion_tokens`, the + `reasoning_*` family, `grammar_lazy`/`grammar_triggers`, `preserved_tokens`, `chat_format`, + `parse_tool_calls`, `adaptive_target`/`adaptive_decay` and `backend_sampling`. Same policy as the + flags above — add on a real request, not speculatively — but the list is no longer something a + future audit has to rediscover: re-derive it by diffing `RequestField.values()` against the field + table `src/test/cpp/test_wire_contracts.cpp` already walks. (Counts are from the b10883 pin; the + two numbers move independently, so re-measure rather than trusting them after a bump.) + - **Video input (`ContentPart.videoFile(...)`).** `mtmd` has had an end-to-end video path since llama.cpp **b9562** (#24269) — `mtmd_helper_video_init_params` was already present at the previous pin, b10456. What **b10647** (#24318, commit `f29551215`) added is the surfacing: a fourth @@ -190,6 +203,12 @@ These are JNI plumbing items for upstream API additions. Policy: add only after that fabricate rather than measure acceptance. Anyone who genuinely wants them already has them: `NativeServer` forwards raw llama-server argv verbatim. + **This is the single record for these two flags.** The b10878 flag audit (entry above) swept them up + again as "upstream options the Java API does not expose" and briefly carried its own copy of the + reasoning; that copy is now a pointer here. An audit re-finding them is expected and is not a signal + to reopen the decision — the audit answers "is this name reachable from Java", which is a different + question from "should it be". + - **Expose `--spec-draft-backend-sampling` toggle via `ModelParameters.setSpecDraftBackendSampling(boolean)`.** Added in b9437 (env `LLAMA_ARG_SPEC_DRAFT_BACKEND_SAMPLING`). Backend sampling for the speculative draft is enabled by default upstream but auto-disabled on `LLAMA_SPLIT_MODE_TENSOR` setups; an explicit Java-side setter lets callers force-disable it for benchmarking or for backends with sampler bugs. Speculative-decoding power users. - **Expose runtime reasoning control via `InferenceParameters.setReasoningControl(boolean)` + `LlamaModel.endReasoning(...)`.** Added in b9444–b9490: new `common_params_sampling::reasoning_control` flag arms the budget sampler so reasoning can be ended at runtime, and new `common_sampler_reasoning_budget_force(common_sampler *)` triggers the end-of-thinking token injection on the next sample. Upstream also adds a `POST /v1/chat/completions/control` server endpoint accepting `{"id": "...", "action": "reasoning_end"}`. Java mapping would be: (a) `InferenceParameters.setReasoningControl(boolean)` arms the sampler on the inference run, (b) a new `LlamaModel.endReasoning(int slotId)` (or per-streaming-task-id) JNI method calls the upstream `common_sampler_reasoning_budget_force` against the slot's sampler. Useful for interactive UIs that want a "skip thinking and answer now" button. Relevant only for reasoning-trained models (DeepSeek-R1, Qwen3-Thinking, GPT-OSS-Reasoner, etc.). @@ -413,10 +432,12 @@ introduced by the version bump — they were deferred to keep that PR landable. - **`LlamaTrainer`'s end-to-end path runs on no CI platform.** `LlamaTrainerIntegrationTest` self-skips everywhere: `net.ladenthin.llama.train.model` is set by no job and its model is in no - `.github/models.csv` row, so `validate-models.{sh,bat}` does not treat it as required. The C++ half - is now mitigated (`test_tts_params.cpp`'s `TrainParams` + `ResolveCpuParams` suites), but nothing - exercises the Java → JNI → native trainer round trip. Adding a small training model to `models.csv` - plus the matching property to the Java test jobs would close it. + `.github/models.csv` row, so `validate-models.{sh,bat}` does not treat it as required. Two slices + are now mitigated — `test_tts_params.cpp`'s `TrainParams` + `ResolveCpuParams` suites for the + parameter build, and `test_wire_contracts.cpp`'s `JavaTrainingFieldContract` for the configuration + key set (`parameters.TrainingField` against `jllama_train::config_keys()`) — but nothing exercises + the Java → JNI → native trainer round trip. Adding a small training model to `models.csv` plus the + matching property to the Java test jobs would close it. - **`LlamaLoader`'s jar-extraction internals need synthetic jar fixtures.** `readBackendManifest`, `tryLoadBackend`, `extractFile`, `moveIntoPlace`, `cleanPath` and `hasNativeLib` are named in no diff --git a/android-llmservice/app/src/main/kotlin/net/ladenthin/android/llmservice/ChatViewModel.kt b/android-llmservice/app/src/main/kotlin/net/ladenthin/android/llmservice/ChatViewModel.kt index c3c820071..6e86f0a0d 100644 --- a/android-llmservice/app/src/main/kotlin/net/ladenthin/android/llmservice/ChatViewModel.kt +++ b/android-llmservice/app/src/main/kotlin/net/ladenthin/android/llmservice/ChatViewModel.kt @@ -302,6 +302,12 @@ class ChatViewModel(application: Application) : AndroidViewModel(application) { .setCtxSize(config.contextSize) .setThreads(config.threads) .setGpuLayers(0) // CPU-only: portable across every device + // The chat template is a LOAD-time option. It used to be passed per request via + // InferenceParameters.withChatTemplate, which llama.cpp's request schema discarded + // without a word -- the MODEL_PATH/CHAT_TEMPLATE test hook silently had no effect. + if (template != null) { + parameters.setChatTemplate(template) + } if (mmproj != null) { // Mirrors the CPU-only + mmproj config validated by MultimodalIntegrationTest: // no GPU device selection, no mmproj offload attempt. @@ -397,7 +403,6 @@ class ChatViewModel(application: Application) : AndroidViewModel(application) { .withMinP(s.minP) .withRepeatPenalty(s.repeatPenalty) .withRepeatLastN(s.repeatLastN) - chatTemplate?.let { params = params.withChatTemplate(it) } log("Generating (temp=${s.temperature}, repeat=${s.repeatPenalty}/${s.repeatLastN}, maxTokens=${s.maxTokens})") val reply = StringBuilder() diff --git a/android-llmservice/requirements.md b/android-llmservice/requirements.md index b84514825..6254207ba 100644 --- a/android-llmservice/requirements.md +++ b/android-llmservice/requirements.md @@ -72,7 +72,7 @@ by hand only (no automated test); `build` = enforced at build/resource-compile t | R4.2 | Messages render as **bubbles** (user vs assistant styling); the list **auto-scrolls** to the newest message. | `MainActivity.MessageBubble` / `Conversation` | manual | | R4.3 | A **localized system prompt** is passed on every turn, nudging the model to answer in the user's language. | `system_prompt`; `ChatViewModel.send` | manual | | R4.4 | `send` is a **no-op** when the input is blank, no model is loaded, or a generation is already in flight. | `ChatViewModel.send` | manual | -| R4.5 | An optional **chat-template override** (e.g. `chatml`) is supported for template-less GGUFs (used by the test hook; real instruct models carry their own template). | `ChatViewModel.chatTemplate` | instrumented | +| R4.5 | An optional **chat-template override** (e.g. `chatml`) is supported for template-less GGUFs (used by the test hook; real instruct models carry their own template). It is applied at **model load** (`ModelParameters.setChatTemplate`) — per request it was silently discarded by llama.cpp's schema, so the hook proved nothing. | `ChatViewModel.openModel` | instrumented | | R4.6 | A generation error keeps the partial reply and surfaces a localized generation error. | `ChatViewModel.startGeneration`; `error_generation` | manual | | R4.7 | **Prompt shortcut chips** (localized `SuggestionChip`s) appear above the input when ready/idle and the input is empty; tapping one fills the draft with a quick-start prompt. | `MainActivity.Conversation` (`promptChip`); `chip_*` | manual | diff --git a/docs/history/parameter-wire-surface.md b/docs/history/parameter-wire-surface.md new file mode 100644 index 000000000..0593907d7 --- /dev/null +++ b/docs/history/parameter-wire-surface.md @@ -0,0 +1,232 @@ + + +# The parameter wire surface: what was wrong, why it was wrong, and what replaced it + +This library sends names on three wires — CLI options in the argv that loads a model, JSON keys in a +completion request, and JSON keys in a fine-tuning configuration. For most of its recorded history +nothing checked any of them against the code that reads them. + +This file records the measurements behind the rework that changed that, so a later reader does not +have to re-derive them. Every number here was produced by running something, not by reading +code; the commands are given so they can be re-run. + +## 1. Eleven names were dead, and most of them were born that way + +The rework deleted eleven wire names in its first pass (a twelfth, `chat_template`, was found later by +the guard itself — see section 5a). The interesting part is not that they were dead — it is **when** +they died. + +This repository's recorded history starts at commit `38f00b2`, which has **no parent**: the tree was +squashed at the fork from [`kherud/java-llama.cpp`](https://github.com/kherud/java-llama.cpp) +(fork point `49be664`, "bump pom.xml version 4.1.0 -> 4.20"). All eleven names are present at both +points. Checking each against the llama.cpp version pinned at the time: + +| Name | at **b4916** (kherud's own pin) | at **b9994** (this repo's first commit) | at **b10883** | +|---|---|---|---| +| `tfs_z`, `penalize_nl`, `penalty_prompt`, `use_jinja` | **0 occurrences** in `examples/server` + `common` | 0 occurrences | 0 occurrences | +| `--grp-attn-n`, `--grp-attn-w` | present, `set_examples({MAIN, PASSKEY})` | `{COMPLETION, PASSKEY}` | `{COMPLETION, PASSKEY}` | +| `--dump-kv-cache` | present, no `set_examples` (so server-visible) | **gone** | gone | +| `--hf-repo-v`, `--hf-file-v` | present | gone (by b10456) | gone | +| `--mlock`, `--no-mmap` | present | present | **gone at b10878** | + +**Six of the eleven were already non-functional at the fork point's own pin.** Not one of them ever +worked in this repository. Only two — `--mlock` / `--no-mmap` — are the "upstream moved and we lagged" +story that the b10878 bump told; the rest are older than that, and older than this repo. + +The two example-scoped ones deserve their own note, because they defeat the obvious check. +`--grp-attn-n` and `--grp-attn-w` are **present in `common/arg.cpp` at every tag this project has ever +pinned**, so any textual sweep of upstream reports them alive. `common_params_parser_init`'s `add_opt` +filters by example at registration time, so they are never registered for `LLAMA_EXAMPLE_SERVER` and +the server parser rejects them exactly like a deleted option. Only the real option table knows this. + +Reproducing the table: + +```bash +# request keys at a given tag +git -C grep -c '"tfs_z"' b4916 -- examples/server common + +# example scoping of a CLI option +git -C show b4916:common/arg.cpp | grep -A6 '"--grp-attn-n"' | grep set_examples +``` + +## 2. The design that made it possible, and where it came from + +The fork point already contains the mechanism, in `de/kherud/llama/JsonParameters.java`: + +```java +abstract class JsonParameters { + // We save parameters directly as a String map here, to re-use as much as possible of the + // (json-based) C++ code. The JNI code for a proper Java-typed data object is comparatively + // too complex and hard to maintain. + final Map parameters = new HashMap<>(); + + @Override public String toString() { + builder.append("\t\"").append(key).append("\": ").append(value); +``` + +That `toString()` is character-for-character the one this project shipped until the rework, and the +comment is the documented trade: type-safety for JNI simplicity. Two consequences follow directly. + +**A `String` key makes "a name with no counterpart" representable.** With ~200 builder setters each +writing a literal, the set of names that can reach a wire is whatever the sources happen to contain — +knowable only by scanning them, and never checked against the receiver. That is the shape of the +eleven above. + +**A map of pre-serialized text has nowhere to validate.** The value is already a string by the time +it is stored, so a method that accepts a JSON *fragment* has no place to check it. That did not matter +at the fork point, where every value came from an internal serializer. It began to matter when this +fork added five methods that take a fragment from the caller. + +## 3. A demonstrated field-injection defect + +`withJsonSchema`, `withResponseFormat`, `withStreamOptions`, `withMessagesJson` and `withToolsJson` +each accepted a caller-supplied JSON fragment and stored it verbatim in that map. None of the five +exists at the fork point — the unsafe serializer is inherited, the entry points that exploit it are +this fork's own. + +Because the document was assembled by string concatenation, a fragment carrying a top-level comma did +not only set its own field. Run against the built classes: + +```java +InferenceParameters.of("hi").withNPredict(16).withCachePrompt(true) + .withResponseFormat("{\"type\":\"json_object\"}, \"n_predict\": 999999, \"cache_prompt\": false") +``` + +```json +{ + "n_predict": 16, + "response_format": {"type":"json_object"}, "n_predict": 999999, "cache_prompt": false, + "prompt": "hi", + "cache_prompt": true +} +``` + +`n_predict` and `cache_prompt` each appear twice. nlohmann resolves duplicate keys **last-wins** +(verified directly: `n_predict=999999 cache_prompt=0`), so the injected values won over the ones the +application had set. `InferenceParameters.toString()` was the request body — `LlamaModel` passed it +straight to `requestCompletion` / `handleChatCompletions` / `requestChatCompletionStream` — so any +host assembling tools, `response_format` or messages JSON from something it did not fully control +could have its own limits overridden. + +**One detail worth keeping.** The obvious fix — parse the fragment with Jackson's `readTree` — is not +enough: `readTree` parses the first value and *ignores* what follows, so the payload above would have +been silently truncated to its first object. Rejecting it needs `FAIL_ON_TRAILING_TOKENS`. One quiet +wrong answer traded for another is not a fix. + +## 4. What replaced it + +Every wire name is now an enum constant carrying the contract it must satisfy, and the base classes +accept nothing else: + +| Registry | Receiver it is checked against | Contract kinds | +|---|---|---| +| `args.ModelFlag` + `args.ModelOption` | `common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options` | `SERVER_PARSER`, `PROJECT_PSEUDO` | +| `parameters.RequestField` | `server_schema::make_llama_cmpl_schema(...)` | `SCHEMA`, `OAI_LAYER` | +| `parameters.TrainingField` | `jllama_train::config_keys()` | — | + +`cmake/extract-java-wire-names.cmake` reads the registries at configure time and emits `{name, +contract}` pairs; `src/test/cpp/test_model_flags.cpp` and `src/test/cpp/test_wire_contracts.cpp` feed +them to the receivers above. Those run in `C++ Tests` on every platform. + +Three properties are worth stating explicitly, because each closes a specific hole: + +- **The contract lives on the constant, not in the test.** A list of exemptions inside a checking test + is the thing that goes stale. `--vocab-only` declares itself `PROJECT_PSEUDO` where it is defined, + and the eleven OAI-layer request keys declare themselves `OAI_LAYER` the same way. +- **The exemption checks are inverted.** Such a name cannot go stale by outliving its constant — it + lives on it. It can go stale the other way: upstream may later register a name we exempted, at which + point the exemption hides a real check. Both tests assert exactly that, plus that the exempt set is + non-empty, so a generator that lost the contract column would not silently exempt everything. +- **Reachability is checked in the other direction too.** `WireNameRegistryTest` drives every public + builder method reflectively and asserts every declared constant is actually emitted by one. A + constant nothing emits is invisible to the C++ check as well — it would be fed to the receiver + forever with no caller able to reach it. + +Values are no longer raw text either: `JsonParameters` enforces that **every stored value is exactly +one well-formed JSON value**, checked centrally on write, and renders the body through Jackson with +keys in sorted order. `toString()` became a redacted, deliberately non-JSON debug view — a parameter +set carries the prompt, the message history and the tool definitions, so a log line built from one +used to leak the whole payload, and a caller who still passes it to the native layer now fails at the +parser instead of quietly sending a different body. + +The invariant found a defect it was not written for on its first run: `JsonParameters.withEnum` stored +`getArgValue()` **unquoted** (`q8_0`, not `"q8_0"`), which is not a JSON value at all. It had no +production caller — the CLI side has its own `putEnum`, where a bare string is correct because argv is +not JSON — so it was deleted rather than fixed. + +## 5. The trainer surface, which looked safest and had no guard at all + +`TrainingParameters` → `train_engine.cpp` is a contract where both ends are ours, which is exactly why +it had nothing checking it. It reads with `j.value(key, default)`: a rename on either side does not +fail, it silently reverts one knob to its default. And `LlamaTrainerIntegrationTest` is gated on a +system property no CI job sets, so nothing runnable covered it. + +At the time of the rework the two sides agreed exactly (15/15) — there was no live defect. The parser +now goes through a `jllama_train::keys` constant per field and `config_keys()` returns those same +constants, so a changed spelling moves both at once, and `JavaTrainingFieldContract` asserts the Java +registry and the engine list are equal in both directions. + +## 5a. The exemption that proved a key dead, one commit after it was written + +Section 4's `OAI_LAYER` contract says: this key is consumed by `oaicompat_*_params_parse` or the task +layer before `make_llama_cmpl_schema` ever sees the body, so do not expect the schema to know it. The +test asserted exactly that — the key is **not** in the schema — with the inverted-check reasoning in +rule 3: an exemption cannot rot by outliving its constant, only by upstream later adopting the name. + +That reasoning was incomplete. Absence from the schema is satisfied equally well by a key **nothing +reads at all**, so the exemption was a hole exactly the size of the problem the registry was built to +close. `chat_template` sat in it: a public `InferenceParameters.withChatTemplate`, writing a key that +appears in upstream sources only where the server *emits* it, in the `/props` payload. + +It could not be closed by driving the parser, because the eleven keys have three different consumers +(`oaicompat_chat_params_parse`, the completion/task layer, and `server-context.cpp`'s infill path) and +two of them need a live `server_context`. The oracle chosen instead is a **reader-shaped** sweep of the +receiver's own sources, run at configure time by the same generator: + +```bash +# what the generator does, per OAI_LAYER key, over tools/server/*.cpp + common/*.cpp +grep -rEn 'json_value\([A-Za-z_.]+, *""|\.contains\(""\)|\.at\(""\)' +``` + +The *shape* is the whole point. `chat_template` does occur as a bare literal upstream, so a token grep +would have called it live; requiring it to appear in a position that reads it from a body does not. +Measured at b10883: + +| key | readers | key | readers | +|---|---|---|---| +| `chat_template` | **0** | `parallel_tool_calls` | 1 | +| `chat_template_kwargs` | 1 | `prompt` | 8 | +| `id_slot` | 1 | `response_format` | 3 | +| `input_prefix` | 2 | `tool_choice` | 3 | +| `input_suffix` | 2 | `tools` | 9 | +| `messages` | 4 | | | + +`JavaRequestFieldContract.EveryOaiLayerKeyIsReadSomewhereUpstream` fails on a count of zero, naming the +key and the remedy. Run against the tree that declared `chat_template`, that is exactly what it printed; +the constant and its builder method were then deleted under rule 2 (a name with no counterpart is +deleted, never deprecated). + +One consumer was affected: the Android "LLM Service" app passed its chat-template override per request, +where llama.cpp discarded it. It now sets it at load time via `ModelParameters.setChatTemplate`. Two +tests had also pinned the dead key — a `ChatAdvancedTest` case asserting only that `applyTemplate` did +not throw (its own Javadoc explained the missing behavioural assertion with the wrong cause: the model's +built-in template winning, rather than the field never being read), and an `InferenceParametersTest` +case asserting the string mapping. Both were deleted with the method. This is the same shape as every +other entry in section 1: a green test pinning the mapping, never the contract. + +## 6. What this does not cover + +- The **failure path** of a dead name is still only checkable by the receiver's own tables. If + upstream stops populating one of those tables, the guard degrades to a vacuous pass — which is why + each test also asserts its oracle is populated before trusting its verdict. +- `test_wire_contracts.cpp` checks the request schema, not the OAI/task layer above it. The + `OAI_LAYER` exemption is now checked from both sides — absent from the schema, and read by + *something* upstream (section 5a) — but the reader sweep is a source pattern, not the parser. It + proves a key is read from some request body; it does not prove *this* endpoint reads it, nor that + it is read with the meaning the builder method documents. +- The **Java↔C++ trainer contract** is guarded; the C++↔C++ pairing inside `train_engine.cpp` rests on + both sides being written against the same `keys` constants, not on a test. diff --git a/llama-langchain4j/src/test/java/net/ladenthin/llama/langchain4j/LangChain4jMappingTest.java b/llama-langchain4j/src/test/java/net/ladenthin/llama/langchain4j/LangChain4jMappingTest.java index e495e4c8c..320dd3275 100644 --- a/llama-langchain4j/src/test/java/net/ladenthin/llama/langchain4j/LangChain4jMappingTest.java +++ b/llama-langchain4j/src/test/java/net/ladenthin/llama/langchain4j/LangChain4jMappingTest.java @@ -95,7 +95,7 @@ void appliesSamplingParametersToInferenceJson() { .stopSequences(Arrays.asList("STOP")) .build(); - String json = LangChain4jMapping.toStreamingParameters(request).toString(); + String json = LangChain4jMapping.toStreamingParameters(request).toJson(); assertThat(json, containsString("\"temperature\"")); assertThat(json, containsString("\"top_k\"")); @@ -211,7 +211,7 @@ void streamingParametersCarryToolsAndToolChoice() { .toolChoice(ToolChoice.REQUIRED) .build(); - String json = LangChain4jMapping.toStreamingParameters(request).toString(); + String json = LangChain4jMapping.toStreamingParameters(request).toJson(); // The streaming blob must carry the same tools wiring the blocking path applies. assertThat(json, containsString("\"tools\"")); @@ -219,8 +219,9 @@ void streamingParametersCarryToolsAndToolChoice() { assertThat(json, containsString("\"tool_choice\"")); assertThat(json, containsString("required")); // Jinja is a load-time option (--jinja). Upstream's request parser never reads a "use_jinja" - // key and silently discards unknown fields, so a re-added withUseChatTemplate(true) here - // would be invisible at runtime and uncatchable by any integration test. Mirrors + // key and silently discards unknown fields, so sending one would be invisible at runtime + // and uncatchable by any integration test. The builder method that used to emit it is gone; + // this pins that nothing puts it back. Mirrors // OpenAiRequestMapperTest#toolsEnableChatTemplateAndForwardChoice. assertThat(json, not(containsString("\"use_jinja\""))); } @@ -295,7 +296,7 @@ void jsonResponseFormatWithoutSchemaMapsToJsonObjectMode() { .responseFormat(ResponseFormat.JSON) .build(); - String json = LangChain4jMapping.toStreamingParameters(request).toString(); + String json = LangChain4jMapping.toStreamingParameters(request).toJson(); assertThat(json, containsString("\"response_format\"")); assertThat(json, containsString("json_object")); @@ -318,7 +319,7 @@ void jsonResponseFormatWithSchemaMapsToJsonSchemaConstraint() { .responseFormat(format) .build(); - String json = LangChain4jMapping.toStreamingParameters(request).toString(); + String json = LangChain4jMapping.toStreamingParameters(request).toJson(); assertThat(json, containsString("\"json_schema\"")); assertThat(json, containsString("\"name\"")); @@ -331,7 +332,7 @@ void textResponseFormatAddsNoConstraint() { .responseFormat(ResponseFormat.TEXT) .build(); - String json = LangChain4jMapping.toStreamingParameters(request).toString(); + String json = LangChain4jMapping.toStreamingParameters(request).toJson(); assertThat(json, not(containsString("\"response_format\""))); assertThat(json, not(containsString("\"json_schema\""))); diff --git a/llama/CMakeLists.txt b/llama/CMakeLists.txt index 75a76716c..9b370e2fe 100644 --- a/llama/CMakeLists.txt +++ b/llama/CMakeLists.txt @@ -560,15 +560,44 @@ if(BUILD_TESTING) enable_testing() include(GoogleTest) - # Make the Java layer's emitted CLI-flag set available to the C++ contract test. The Java - # sources are the single source of truth; see cmake/extract-java-cli-flags.cmake for why a - # textual sweep of common/arg.cpp is not a substitute (example scoping is invisible to it). - include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/extract-java-cli-flags.cmake) - jllama_extract_java_cli_flags( - JAVA_FLAG_SOURCES + # Make the three wire-name registries the Java layer declares available to the C++ contract + # tests. The Java enums are the single source of truth; see cmake/extract-java-wire-names.cmake + # for why a textual sweep of the receiver's source is not a substitute (a CLI option can be + # present in common/arg.cpp yet scoped away from the server example, and an unknown request key + # is discarded without a word). + include(${CMAKE_CURRENT_SOURCE_DIR}/cmake/extract-java-wire-names.cmake) + file(GLOB JLLAMA_REQUEST_READER_SOURCES + ${llama.cpp_SOURCE_DIR}/tools/server/*.cpp + ${llama.cpp_SOURCE_DIR}/common/*.cpp) + jllama_extract_java_wire_names( + JAVA_SOURCES ${CMAKE_CURRENT_SOURCE_DIR}/src/main/java/net/ladenthin/llama/args/ModelFlag.java - ${CMAKE_CURRENT_SOURCE_DIR}/src/main/java/net/ladenthin/llama/parameters/ModelParameters.java - OUTPUT_HEADER ${CMAKE_CURRENT_BINARY_DIR}/generated/jllama_java_cli_flags.h + ${CMAKE_CURRENT_SOURCE_DIR}/src/main/java/net/ladenthin/llama/args/ModelOption.java + ARRAY_PREFIX JLLAMA_JAVA_CLI + DEFAULT_CONTRACT SERVER_PARSER + MIN_COUNT 50 + OUTPUT_HEADER ${CMAKE_CURRENT_BINARY_DIR}/generated/jllama_java_cli_flags.h + ) + jllama_extract_java_wire_names( + JAVA_SOURCES + ${CMAKE_CURRENT_SOURCE_DIR}/src/main/java/net/ladenthin/llama/parameters/RequestField.java + ARRAY_PREFIX JLLAMA_JAVA_REQUEST + DEFAULT_CONTRACT SCHEMA + MIN_COUNT 30 + OUTPUT_HEADER ${CMAKE_CURRENT_BINARY_DIR}/generated/jllama_java_request_fields.h + # An OAI_LAYER key never reaches make_llama_cmpl_schema, so the schema cannot vouch for + # it. Sweep the layer that does consume it instead -- globbed, not listed, so an upstream + # file split does not quietly narrow the corpus. + READER_CONTRACT OAI_LAYER + READER_SOURCES ${JLLAMA_REQUEST_READER_SOURCES} + ) + jllama_extract_java_wire_names( + JAVA_SOURCES + ${CMAKE_CURRENT_SOURCE_DIR}/src/main/java/net/ladenthin/llama/parameters/TrainingField.java + ARRAY_PREFIX JLLAMA_JAVA_TRAINING + DEFAULT_CONTRACT ENGINE + MIN_COUNT 10 + OUTPUT_HEADER ${CMAKE_CURRENT_BINARY_DIR}/generated/jllama_java_training_fields.h ) add_executable(jllama_test @@ -581,6 +610,7 @@ if(BUILD_TESTING) src/test/cpp/test_tts_params.cpp src/test/cpp/test_model_split.cpp src/test/cpp/test_model_flags.cpp + src/test/cpp/test_wire_contracts.cpp ${llama.cpp_SOURCE_DIR}/tools/server/server-common.cpp ${llama.cpp_SOURCE_DIR}/tools/server/server-chat.cpp ${llama.cpp_SOURCE_DIR}/tools/server/server-context.cpp diff --git a/llama/cmake/extract-java-cli-flags.cmake b/llama/cmake/extract-java-cli-flags.cmake deleted file mode 100644 index b58c2929e..000000000 --- a/llama/cmake/extract-java-cli-flags.cmake +++ /dev/null @@ -1,111 +0,0 @@ -# SPDX-FileCopyrightText: 2026 Bernard Ladenthin -# -# SPDX-License-Identifier: MIT - -# Generates a C++ header listing every CLI flag the Java layer can emit into the argv that -# LlamaModel.loadModel() hands to llama.cpp's common_params_parse(). -# -# WHY THIS EXISTS -# --------------- -# ModelFlag / ModelParameters are the only two places that write a "--flag" key into the -# parameter map, and that map becomes argv verbatim. common_params_parse() treats an -# *unregistered* option as a hard error, not a warning -- so the moment upstream removes a -# flag (or narrows its set_examples() scope away from LLAMA_EXAMPLE_SERVER), the matching -# builder method silently turns into "this model will never load". -# -# Every Java-side test of these methods asserts the *string mapping* (hasKey("--mlock")), never -# that llama.cpp still accepts the string, so they stay green forever while the flag is dead. -# That is exactly how --mlock/--no-mmap (removed at b10878), --dump-kv-cache, --hf-repo-v and -# --hf-file-v reached main, and how --grp-attn-n/--grp-attn-w hid for even longer: those two -# still exist in arg.cpp, but are scoped to LLAMA_EXAMPLE_COMPLETION, so a grep-based sweep -# reports them alive while the server parser rejects them. A textual check is structurally -# blind to example scoping; only the real parser knows. -# -# So: this script extracts the list from the Java sources (the single source of truth), and -# src/test/cpp/test_model_flags.cpp feeds every entry to the *actual* -# common_params_parser_init(params, LLAMA_EXAMPLE_SERVER) option table. jllama_test runs on -# every platform in the "C++ Tests" job, so a llama.cpp bump that kills a flag reds CI in the -# same run that introduces it. -# -# EXTRACTION -# ---------- -# Line-oriented on purpose: any line whose trimmed form starts with "*", "//" or "/*" is a -# comment and is dropped before matching, which removes every Javadoc mention of a flag -# ({@code --flash-attn}, prose referring to --mlock, ...) without needing a real Java parser. -# What survives is `"--something"` in code position -- enum constants, putScalar/putEnum keys, -# parameters.put keys, and the private static final ARG_* constants alike. -# -# Inputs : JAVA_FLAG_SOURCES - list of .java files to scan -# OUTPUT_HEADER - path of the header to write -# Output : a header defining JLLAMA_JAVA_CLI_FLAGS[] / JLLAMA_JAVA_CLI_FLAG_COUNT - -function(jllama_extract_java_cli_flags) - cmake_parse_arguments(ARG "" "OUTPUT_HEADER" "JAVA_FLAG_SOURCES" ${ARGN}) - - if(NOT ARG_OUTPUT_HEADER) - message(FATAL_ERROR "jllama_extract_java_cli_flags: OUTPUT_HEADER is required") - endif() - if(NOT ARG_JAVA_FLAG_SOURCES) - message(FATAL_ERROR "jllama_extract_java_cli_flags: JAVA_FLAG_SOURCES is required") - endif() - - set(_flags "") - foreach(_src IN LISTS ARG_JAVA_FLAG_SOURCES) - if(NOT EXISTS "${_src}") - message(FATAL_ERROR "jllama_extract_java_cli_flags: missing source ${_src}") - endif() - file(STRINGS "${_src}" _lines) - foreach(_line IN LISTS _lines) - string(STRIP "${_line}" _trimmed) - # Drop comment lines (Javadoc continuations, // and block-comment openers). - if(_trimmed MATCHES "^(\\*|//|/\\*)") - continue() - endif() - # CMake regex has no global match, so consume the line one literal at a time. - while(_line MATCHES "\"(--[A-Za-z0-9][A-Za-z0-9._+-]*)\"") - list(APPEND _flags "${CMAKE_MATCH_1}") - string(REPLACE "\"${CMAKE_MATCH_1}\"" "" _line "${_line}") - endwhile() - endforeach() - endforeach() - - list(REMOVE_DUPLICATES _flags) - list(SORT _flags) - list(LENGTH _flags _count) - - # A silently empty list would make the contract test vacuously pass -- the same - # "nothing to scan reported as a clean pass" trap verify-bytecode-version.sh exits 2 for. - if(_count LESS 50) - message(FATAL_ERROR - "jllama_extract_java_cli_flags: only ${_count} flags extracted from " - "${ARG_JAVA_FLAG_SOURCES} -- the extractor is broken or the sources moved") - endif() - - set(_body "") - foreach(_flag IN LISTS _flags) - string(APPEND _body " \"${_flag}\",\n") - endforeach() - - set(_header "// Generated by cmake/extract-java-cli-flags.cmake -- DO NOT EDIT.\n") - string(APPEND _header "// Source of truth: the Java files listed in llama/CMakeLists.txt.\n") - string(APPEND _header "#pragma once\n\n") - string(APPEND _header "static const char * const JLLAMA_JAVA_CLI_FLAGS[] = {\n${_body}};\n\n") - string(APPEND _header "static const int JLLAMA_JAVA_CLI_FLAG_COUNT = ${_count};\n") - - # Only rewrite when the content actually changed, so an unrelated re-configure does not - # touch the header and force a needless rebuild of the test. - set(_existing "") - if(EXISTS "${ARG_OUTPUT_HEADER}") - file(READ "${ARG_OUTPUT_HEADER}" _existing) - endif() - if(NOT _existing STREQUAL _header) - file(WRITE "${ARG_OUTPUT_HEADER}" "${_header}") - endif() - - # Re-run configure (and therefore this extractor) whenever a scanned Java file changes. - foreach(_src IN LISTS ARG_JAVA_FLAG_SOURCES) - set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${_src}") - endforeach() - - message(STATUS "jllama: extracted ${_count} Java CLI flags -> ${ARG_OUTPUT_HEADER}") -endfunction() diff --git a/llama/cmake/extract-java-wire-names.cmake b/llama/cmake/extract-java-wire-names.cmake new file mode 100644 index 000000000..d0a70be36 --- /dev/null +++ b/llama/cmake/extract-java-wire-names.cmake @@ -0,0 +1,191 @@ +# SPDX-FileCopyrightText: 2026 Bernard Ladenthin +# +# SPDX-License-Identifier: MIT + +# Generates a C++ header listing the wire names one Java enum registry declares, together with +# the contract each one states it satisfies. +# +# WHY THIS EXISTS +# --------------- +# Three surfaces leave this library as names on a wire, and each has a receiver that can be +# asked what it accepts: +# +# * CLI options (ModelFlag + ModelOption) -> llama.cpp's +# common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options +# * request keys (RequestField) -> server_schema::make_llama_cmpl_schema(...) +# * trainer keys (TrainingField) -> jllama_train::config_keys() in train_engine.cpp +# +# Both failure modes are silent on the Java side. An unregistered CLI option is a hard parse +# error, so the model simply never loads; an unknown request key is discarded without a word, so +# the parameter simply never takes effect. Either way a Java test that asserts the string mapping +# ("does the map contain top_k") passes forever while the name is dead. The C++ tests close that +# by feeding this generated list to the real receiver, and they run on every platform. +# +# EXTRACTION +# ---------- +# Only enum constant declarations are matched -- `NAME("wire-name")` or +# `NAME("wire-name", XxxContract.KIND)` -- so prose, javadoc and helper code cannot contribute a +# name by accident. That precision is why the registries were made enums in the first place: the +# previous version of this script scanned any string literal in code position across a 1900-line +# builder and needed a comment-stripping heuristic to do it. +# +# THE EXEMPTION HOLE, AND WHY THERE IS A SECOND SCAN +# -------------------------------------------------- +# A name that declares a contract the receiver above cannot answer for -- OAI_LAYER, consumed by +# oaicompat_*_params_parse and the task layer before the schema ever sees the body -- was checked +# only for *absence* from the schema. Absence is satisfied just as well by a name nothing reads at +# all, so the exemption was a hole exactly the size of the problem the registry was built to close: +# `chat_template` sat in it, written by a public builder method, read by nobody, discarded silently. +# +# There is no callable table to ask "which keys does this parser read", so the oracle here is a +# *reader-shaped* sweep of the receiver's own source -- `json_value(x, "k", ...)`, `.contains("k")`, +# `.at("k")` -- rather than a bare token grep. The shape is what makes it useful: `chat_template` +# does occur as a literal upstream, in the `/props` payload the server *emits*, and a token grep +# would have called it live. This is weaker evidence than driving the real receiver, so it proves +# only "something reads this key from a body"; the C++ test says so where it asserts on it. +# +# Inputs : JAVA_SOURCES - the registry .java files to scan +# ARRAY_PREFIX - C identifier prefix; emits

_NAMES[],

_CONTRACTS[], +#

_READERS[],

_COUNT +# DEFAULT_CONTRACT - contract for a constant that does not name one +# MIN_COUNT - floor below which extraction is treated as broken +# OUTPUT_HEADER - path of the header to write +# READER_CONTRACT - optional: contract whose names get the reader sweep +# READER_SOURCES - optional: receiver sources to sweep for those names + +function(jllama_extract_java_wire_names) + cmake_parse_arguments(ARG "" + "ARRAY_PREFIX;DEFAULT_CONTRACT;MIN_COUNT;OUTPUT_HEADER;READER_CONTRACT" + "JAVA_SOURCES;READER_SOURCES" ${ARGN}) + + foreach(_required ARRAY_PREFIX DEFAULT_CONTRACT MIN_COUNT OUTPUT_HEADER JAVA_SOURCES) + if(NOT ARG_${_required}) + message(FATAL_ERROR "jllama_extract_java_wire_names: ${_required} is required") + endif() + endforeach() + + set(_names "") + set(_contracts "") + foreach(_src IN LISTS ARG_JAVA_SOURCES) + if(NOT EXISTS "${_src}") + message(FATAL_ERROR "jllama_extract_java_wire_names: missing source ${_src}") + endif() + file(STRINGS "${_src}" _lines) + foreach(_line IN LISTS _lines) + string(STRIP "${_line}" _trimmed) + # Enum constant declarations only. A javadoc line mentioning {@code --mlock} cannot + # match this, so no comment stripping is needed to keep prose out. + if(NOT _trimmed MATCHES "^[A-Z][A-Z0-9_]*\\(\"([^\"]+)\"(.*)\\)[,;]$") + continue() + endif() + set(_name "${CMAKE_MATCH_1}") + set(_rest "${CMAKE_MATCH_2}") + set(_contract "${ARG_DEFAULT_CONTRACT}") + if(_rest MATCHES "Contract\\.([A-Z][A-Z0-9_]*)") + set(_contract "${CMAKE_MATCH_1}") + endif() + list(APPEND _names "${_name}") + list(APPEND _contracts "${_name}=${_contract}") + endforeach() + endforeach() + + list(LENGTH _names _count) + list(REMOVE_DUPLICATES _names) + list(LENGTH _names _unique_count) + if(NOT _count EQUAL _unique_count) + message(FATAL_ERROR + "jllama_extract_java_wire_names: ${ARG_ARRAY_PREFIX} declares a wire name twice -- " + "two builder methods would write the same key and the last call would win") + endif() + list(SORT _names) + + # A silently empty list would make the contract test vacuously pass -- the same + # "nothing to scan reported as a clean pass" trap verify-bytecode-version.sh exits 2 for. + if(_count LESS ${ARG_MIN_COUNT}) + message(FATAL_ERROR + "jllama_extract_java_wire_names: only ${_count} names extracted for " + "${ARG_ARRAY_PREFIX} from ${ARG_JAVA_SOURCES} -- the extractor is broken or the " + "sources moved") + endif() + + # Reader sweep. Concatenated once, then matched per name -- the receiver sources are read + # here and nowhere else, so a rename upstream shows up as an empty corpus, not as silence. + set(_reader_corpus "") + if(ARG_READER_CONTRACT) + if(NOT ARG_READER_SOURCES) + message(FATAL_ERROR + "jllama_extract_java_wire_names: READER_CONTRACT without READER_SOURCES") + endif() + foreach(_src IN LISTS ARG_READER_SOURCES) + if(NOT EXISTS "${_src}") + continue() + endif() + file(READ "${_src}" _content) + string(APPEND _reader_corpus "${_content}") + set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${_src}") + endforeach() + string(LENGTH "${_reader_corpus}" _corpus_length) + if(_corpus_length EQUAL 0) + message(FATAL_ERROR + "jllama_extract_java_wire_names: ${ARG_ARRAY_PREFIX} reader sweep read nothing " + "from READER_SOURCES -- the receiver sources moved, and every scanned name would " + "otherwise report as unread") + endif() + endif() + + set(_name_body "") + set(_contract_body "") + set(_reader_body "") + set(_swept 0) + foreach(_name IN LISTS _names) + string(APPEND _name_body " \"${_name}\",\n") + set(_this_contract "") + foreach(_pair IN LISTS _contracts) + if(_pair MATCHES "^${_name}=(.*)$") + set(_this_contract "${CMAKE_MATCH_1}") + string(APPEND _contract_body " \"${_this_contract}\",\n") + break() + endif() + endforeach() + # -1 means "not swept", which is not the same as "swept and found nothing" (0). + set(_readers -1) + if(ARG_READER_CONTRACT AND _this_contract STREQUAL "${ARG_READER_CONTRACT}") + string(REGEX MATCHALL + "json_value\\([A-Za-z_.]+, *\"${_name}\"|\\.contains\\(\"${_name}\"\\)|\\.at\\(\"${_name}\"\\)" + _hits "${_reader_corpus}") + list(LENGTH _hits _readers) + math(EXPR _swept "${_swept} + 1") + endif() + string(APPEND _reader_body " ${_readers},\n") + endforeach() + + set(_header "// Generated by cmake/extract-java-wire-names.cmake -- DO NOT EDIT.\n") + string(APPEND _header "// Source of truth: the Java registry files listed in llama/CMakeLists.txt.\n") + string(APPEND _header "#pragma once\n\n") + string(APPEND _header "static const char * const ${ARG_ARRAY_PREFIX}_NAMES[] = {\n${_name_body}};\n\n") + string(APPEND _header "static const char * const ${ARG_ARRAY_PREFIX}_CONTRACTS[] = {\n${_contract_body}};\n\n") + string(APPEND _header "static const int ${ARG_ARRAY_PREFIX}_READERS[] = {\n${_reader_body}};\n\n") + string(APPEND _header "static const int ${ARG_ARRAY_PREFIX}_COUNT = ${_count};\n") + + # Only rewrite when the content actually changed, so an unrelated re-configure does not + # touch the header and force a needless rebuild of the test. + set(_existing "") + if(EXISTS "${ARG_OUTPUT_HEADER}") + file(READ "${ARG_OUTPUT_HEADER}" _existing) + endif() + if(NOT _existing STREQUAL _header) + file(WRITE "${ARG_OUTPUT_HEADER}" "${_header}") + endif() + + # Re-run configure (and therefore this extractor) whenever a scanned Java file changes. + foreach(_src IN LISTS ARG_JAVA_SOURCES) + set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${_src}") + endforeach() + + set(_swept_note "") + if(ARG_READER_CONTRACT) + set(_swept_note " (${_swept} swept for ${ARG_READER_CONTRACT} readers)") + endif() + message(STATUS + "jllama: extracted ${_count} ${ARG_ARRAY_PREFIX} names${_swept_note} -> ${ARG_OUTPUT_HEADER}") +endfunction() diff --git a/llama/pom.xml b/llama/pom.xml index 909cfa455..d525bc012 100644 --- a/llama/pom.xml +++ b/llama/pom.xml @@ -831,12 +831,22 @@ SPDX-License-Identifier: MIT net.ladenthin.llama.json.RerankResponseParser net.ladenthin.llama.json.ChatResponseParser net.ladenthin.llama.json.CompletionResponseParser + + net.ladenthin.llama.parameters.JsonParameters net.ladenthin.llama.value.* net.ladenthin.llama.exception.* net.ladenthin.llama.args.* net.ladenthin.llama.json.* + net.ladenthin.llama.parameters.* 100 30000 diff --git a/llama/spotbugs-exclude.xml b/llama/spotbugs-exclude.xml index e7f478399..385440973 100644 --- a/llama/spotbugs-exclude.xml +++ b/llama/spotbugs-exclude.xml @@ -105,6 +105,23 @@ SPDX-License-Identifier: MIT + + + + + + +