From aae75790af67ea3c40930dcbe877361e157d1530 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 07:01:22 +0000 Subject: [PATCH 1/7] feat!: delete the parameter surface llama.cpp never reads Twelve public builder methods went away, because none of them could reach llama.cpp. Backward compatibility is deliberately not preserved: a method that writes a key the receiver discards is worse than no method, since the call site reads as configuration and behaves as a no-op. Five wrote request keys that no pinned llama.cpp has ever read -- withTfsZ (tfs_z), withPenalizeNl (penalize_nl), both withPenaltyPrompt overloads (penalty_prompt) and withUseChatTemplate (use_jinja). The request schema silently discards unknown fields, so these were invisible at runtime and uncatchable by any integration test. Seven wrote CLI flags the server argument parser does not register. #426 had already stopped them emitting -- setGrpAttnN, setGrpAttnW, enableDumpKvCache, setHfRepoV and setHfFileV became no-ops, enableMlock and disableMmap were re-pointed onto --load-mode -- which kept callers loading but left the API claiming capabilities it does not have. setLoadMode(LoadMode) is the whole replacement: LoadMode.MLOCK was --mlock, LoadMode.NONE was --no-mmap. Verified against the history rather than assumed, because it changes what this is: all eleven wire names were inherited from kherud/java-llama.cpp at fork point 49be664 (its own pin was llama.cpp b4916), and SIX of them were already non-functional at that pin -- the four request keys appear zero times in b4916's examples/server + common, and --grp-attn-n/-w carried set_examples({LLAMA_EXAMPLE_MAIN, LLAMA_EXAMPLE_PASSKEY}) there exactly as they carry {COMPLETION, PASSKEY} at b10883. A seventh, --dump-kv-cache, was alive at b4916 and already dead at b9994, this repo's first recorded commit. So the dominant cause is not version drift, it is surface that was never executed against the receiver; the docs/history writeup lands with the guard in a later commit of this series. Two follow-on deletions: ParameterJsonSerializer.buildIntArray had withPenaltyPrompt(int...) as its only caller and is gone with it, and ChatAdvancedTest's three model-backed "must produce output" tests for tfs_z and penalty_prompt are gone -- they passed for years while the server ignored the parameter, which is precisely the false confidence being removed here. Its testUseChatTemplateInGenerate is renamed to testMessagesInGenerate and says in its javadoc that it was always measuring withMessages() alone. README's InferenceParameters snippets were repaired in passing: besides the two dead calls they used a set* naming the class has not had for a long time (it has zero set* methods), so the documented examples did not compile. Verified: mvn test 1740 tests green across the reactor (19 fewer, matching the deletions exactly), PIT 320/320 killed at 100%, SpotBugs 0 bug instances, spotless and javadoc:jar clean. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH --- README.md | 18 +- .../langchain4j/LangChain4jMappingTest.java | 5 +- .../net/ladenthin/llama/args/LoadMode.java | 8 +- .../llama/parameters/InferenceParameters.java | 99 ---------- .../llama/parameters/ModelParameters.java | 181 ------------------ .../parameters/ParameterJsonSerializer.java | 13 -- llama/src/test/java/examples/ChatExample.java | 4 +- llama/src/test/java/examples/MainExample.java | 1 - .../net/ladenthin/llama/ChatAdvancedTest.java | 85 ++------ .../llama/ToolCallingIntegrationTest.java | 3 +- .../json/ParameterJsonSerializerTest.java | 26 --- .../parameters/InferenceParametersTest.java | 41 ---- .../ModelParametersExtendedTest.java | 75 +------- 13 files changed, 41 insertions(+), 518 deletions(-) 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/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..018f7ce1e 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 @@ -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\""))); } diff --git a/llama/src/main/java/net/ladenthin/llama/args/LoadMode.java b/llama/src/main/java/net/ladenthin/llama/args/LoadMode.java index 73011843d..8f8b7befd 100644 --- a/llama/src/main/java/net/ladenthin/llama/args/LoadMode.java +++ b/llama/src/main/java/net/ladenthin/llama/args/LoadMode.java @@ -14,10 +14,10 @@ * {@code --mlock}, {@code --mmap}/{@code --no-mmap} and {@code -dio}/{@code --direct-io} at b10092 * and deleted them at b10878 — the whole deprecation window opened and closed * inside eight tags. Because llama.cpp's argument parser treats an unknown option as a hard error - * rather than a warning, the deleted spellings do not degrade a model load, they prevent it; the - * {@code ModelParameters} methods that used to emit them now emit the matching mode here instead - * (see {@link net.ladenthin.llama.parameters.ModelParameters#enableMlock()} and - * {@link net.ladenthin.llama.parameters.ModelParameters#disableMmap()}). + * rather than a warning, the deleted spellings do not degrade a model load, they prevent it. The + * {@code ModelParameters} methods that used to emit them were removed together with the flags; + * pass {@link #MLOCK} (was {@code --mlock}) or {@link #NONE} (was {@code --no-mmap}) to + * {@link net.ladenthin.llama.parameters.ModelParameters#setLoadMode(LoadMode)} instead. * * @see net.ladenthin.llama.parameters.ModelParameters#setLoadMode(LoadMode) */ diff --git a/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java b/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java index b61ba55b8..d403dda72 100644 --- a/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java +++ b/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java @@ -67,7 +67,6 @@ public final class InferenceParameters extends JsonParameters { private static final String PARAM_TOP_K = "top_k"; private static final String PARAM_TOP_P = "top_p"; private static final String PARAM_MIN_P = "min_p"; - private static final String PARAM_TFS_Z = "tfs_z"; private static final String PARAM_TYPICAL_P = "typical_p"; private static final String PARAM_TEMPERATURE = "temperature"; private static final String PARAM_DYNATEMP_RANGE = "dynatemp_range"; @@ -79,21 +78,18 @@ public final class InferenceParameters extends JsonParameters { private static final String PARAM_MIROSTAT = "mirostat"; private static final String PARAM_MIROSTAT_TAU = "mirostat_tau"; private static final String PARAM_MIROSTAT_ETA = "mirostat_eta"; - private static final String PARAM_PENALIZE_NL = "penalize_nl"; private static final String PARAM_N_KEEP = "n_keep"; private static final String PARAM_SEED = "seed"; private static final String PARAM_N_PROBS = "n_probs"; private static final String PARAM_MIN_KEEP = "min_keep"; private static final String PARAM_GRAMMAR = "grammar"; private static final String PARAM_JSON_SCHEMA = "json_schema"; - private static final String PARAM_PENALTY_PROMPT = "penalty_prompt"; private static final String PARAM_IGNORE_EOS = "ignore_eos"; private static final String PARAM_LOGIT_BIAS = "logit_bias"; private static final String PARAM_STOP = "stop"; private static final String PARAM_SAMPLERS = "samplers"; private static final String PARAM_STREAM = "stream"; private static final String PARAM_CHAT_TEMPLATE = "chat_template"; - private static final String PARAM_USE_JINJA = "use_jinja"; private static final String PARAM_CHAT_TEMPLATE_KWARGS = "chat_template_kwargs"; private static final String PARAM_MESSAGES = "messages"; private static final String PARAM_TOP_N_SIGMA = "top_n_sigma"; @@ -292,24 +288,6 @@ public InferenceParameters withMinP(float minP) { return withScalar(PARAM_MIN_P, minP); } - /** - * Returns a new request with tail-free sampling z replaced (default: 1.0, 1.0 = disabled). - * - *

Ignored by the server. Upstream llama.cpp no longer reads this field — {@code tfs_z} - * appears nowhere in {@code common/} or {@code tools/server/} as of the pinned build, and the request - * schema silently discards unknown fields rather than rejecting them, so setting it has no effect on - * generation. Retained only so existing call sites keep compiling; it will be removed in a future - * release.

- * - * @param tfsZ tail-free sampling parameter z (1.0 = disabled) - * @return a new instance; this instance is unchanged - * @deprecated upstream removed tail-free sampling; the value is discarded by the server - */ - @Deprecated - public InferenceParameters withTfsZ(float tfsZ) { - return withScalar(PARAM_TFS_Z, tfsZ); - } - /** * Returns a new request with locally-typical sampling p replaced (default: 1.0, 1.0 = disabled). * @@ -434,24 +412,6 @@ public InferenceParameters withMiroStatEta(float mirostatEta) { return withScalar(PARAM_MIROSTAT_ETA, mirostatEta); } - /** - * Returns a new request with the newline-penalty flag replaced. - * - *

Ignored by the server. Upstream llama.cpp no longer reads this field — {@code penalize_nl} - * appears nowhere in {@code common/} or {@code tools/server/} as of the pinned build, and the request - * schema silently discards unknown fields rather than rejecting them, so setting it has no effect on - * generation. Retained only so existing call sites keep compiling; it will be removed in a future - * release.

- * - * @param penalizeNl whether to penalize newline tokens - * @return a new instance; this instance is unchanged - * @deprecated upstream removed the newline penalty; the value is discarded by the server - */ - @Deprecated - public InferenceParameters withPenalizeNl(boolean penalizeNl) { - return withScalar(PARAM_PENALIZE_NL, penalizeNl); - } - /** * Returns a new request with the {@code n_keep} value replaced (default: 0, -1 = all). * @@ -540,42 +500,6 @@ public InferenceParameters withResponseFormat(String responseFormatJson) { return withRaw(PARAM_RESPONSE_FORMAT, responseFormatJson); } - /** - * Returns a new request with the repetition-penalty prompt-portion override replaced. - * - *

Ignored by the server. Upstream llama.cpp no longer reads this field — {@code penalty_prompt} - * appears nowhere in {@code common/} or {@code tools/server/} as of the pinned build, and the request - * schema silently discards unknown fields rather than rejecting them, so setting it has no effect on - * generation. Retained only so existing call sites keep compiling; it will be removed in a future - * release.

- * - * @param penaltyPrompt the string portion of the prompt to penalize; {@code null} clears - * @return a new instance; this instance is unchanged - * @deprecated upstream removed the penalty-prompt override; the value is discarded by the server - */ - @Deprecated - public InferenceParameters withPenaltyPrompt(@Nullable String penaltyPrompt) { - return withOptionalJson(PARAM_PENALTY_PROMPT, penaltyPrompt); - } - - /** - * Returns a new request with the repetition-penalty prompt-portion override replaced - * (token-id form). Empty input is a no-op (returns {@code this}). - * - *

Ignored by the server — see {@link #withPenaltyPrompt(String)}.

- * - * @param tokens token ids of the prompt portion to penalize - * @return a new instance with the array set, or {@code this} if {@code tokens} is empty - * @deprecated upstream removed the penalty-prompt override; the value is discarded by the server - */ - @Deprecated - public InferenceParameters withPenaltyPrompt(int... tokens) { - if (tokens.length == 0) { - return this; - } - return withRaw(PARAM_PENALTY_PROMPT, serializer.buildIntArray(tokens).toString()); - } - /** * Returns a new request with the EOS-ignore flag replaced. * @@ -676,29 +600,6 @@ public InferenceParameters withSamplers(Sampler... samplers) { return withRaw(PARAM_SAMPLERS, serializer.buildSamplers(samplers).toString()); } - /** - * Returns a new request with the chat-template flag replaced. - * - *

Ignored by the server. Jinja templating is a launch-time setting, - * not a per-request one. {@code common_params::use_jinja} is set at parse time — by - * {@code --jinja} / {@code --no-jinja}, by the per-example defaults, and by the - * {@code --gpt-oss-*-default} presets — and the string {@code "use_jinja"} appears nowhere - * in {@code common/} or {@code tools/server/} as a request key on the pinned build. The request - * schema silently discards unknown fields, so this neither enables nor disables anything. - * Use {@link net.ladenthin.llama.parameters.ModelParameters#enableJinja()} when loading the - * model instead. Retained only so existing call sites keep compiling; it will be removed in a - * future release.

- * - * @param useChatTemplate whether to apply a chat template - * @return a new instance; this instance is unchanged - * @deprecated jinja is a load-time option; the request field is discarded by the server. Use - * {@link net.ladenthin.llama.parameters.ModelParameters#enableJinja()} - */ - @Deprecated - public InferenceParameters withUseChatTemplate(boolean useChatTemplate) { - return withScalar(PARAM_USE_JINJA, useChatTemplate); - } - /** * Returns a new request with the chat-template string replaced. * diff --git a/llama/src/main/java/net/ladenthin/llama/parameters/ModelParameters.java b/llama/src/main/java/net/ladenthin/llama/parameters/ModelParameters.java index 8c9cc7103..277f2ab2d 100644 --- a/llama/src/main/java/net/ladenthin/llama/parameters/ModelParameters.java +++ b/llama/src/main/java/net/ladenthin/llama/parameters/ModelParameters.java @@ -749,87 +749,6 @@ public ModelParameters setYarnBetaFast(float yarnBetaFast) { return putScalar("--yarn-beta-fast", yarnBetaFast); } - /** - * Set group-attention factor (default: 1). - * - *

No longer emitted — this method is a no-op. {@code --grp-attn-n} still exists in - * {@code common/arg.cpp}, but carries {@code set_examples({LLAMA_EXAMPLE_COMPLETION, - * LLAMA_EXAMPLE_PASSKEY})}, so {@code common_params_parser_init} never registers it for - * {@code LLAMA_EXAMPLE_SERVER} — the example this binding parses with. A textual sweep of - * upstream sources cannot see that; only the real option table can. Because llama.cpp's - * argument parser treats an unregistered option as a hard error rather than a warning, still - * emitting it would make {@code loadModel()} throw {@code "Failed to parse model parameters"} - * instead of loading the model. Writing nothing keeps existing call sites compiling and - * loading. The method will be removed in a future release; the contract is enforced by - * {@code src/test/cpp/test_model_flags.cpp}, which drives every emitted flag through the real - * server option table.

- * - * @param grpAttnN the group-attention factor - * @return this builder - * @deprecated upstream scopes {@code --grp-attn-n} to non-server examples, so the server - * argument parser rejects it - */ - // Error Prone's InlineMeSuggester wants an @InlineMe here because the body is a single - // expression. Inlining would be exactly wrong: the point of the deprecation is that callers - // keep calling THIS method, so a later removal is one edit here and not a code search. - @SuppressWarnings("InlineMeSuggester") - @Deprecated - public ModelParameters setGrpAttnN(int grpAttnN) { - return this; - } - - /** - * Set group-attention width (default: 512). - * - *

No longer emitted — this method is a no-op. {@code --grp-attn-w} still exists in - * {@code common/arg.cpp}, but carries {@code set_examples({LLAMA_EXAMPLE_COMPLETION})}, so - * {@code common_params_parser_init} never registers it for {@code LLAMA_EXAMPLE_SERVER} — the - * example this binding parses with. Because llama.cpp's - * argument parser treats an unregistered option as a hard error rather than a warning, still - * emitting it would make {@code loadModel()} throw {@code "Failed to parse model parameters"} - * instead of loading the model. Writing nothing keeps existing call sites compiling and - * loading. The method will be removed in a future release; the contract is enforced by - * {@code src/test/cpp/test_model_flags.cpp}, which drives every emitted flag through the real - * server option table.

- * - * @param grpAttnW the group-attention width - * @return this builder - * @deprecated upstream scopes {@code --grp-attn-w} to the completion example, so the server - * argument parser rejects it - */ - // Error Prone's InlineMeSuggester wants an @InlineMe here because the body is a single - // expression. Inlining would be exactly wrong: the point of the deprecation is that callers - // keep calling THIS method, so a later removal is one edit here and not a code search. - @SuppressWarnings("InlineMeSuggester") - @Deprecated - public ModelParameters setGrpAttnW(int grpAttnW) { - return this; - } - - /** - * Enable verbose printing of the KV cache. - * - *

No longer emitted — this method is a no-op. Upstream removed {@code --dump-kv-cache} - * with no replacement; it appears nowhere in llama.cpp at the pinned build. Because llama.cpp's - * argument parser treats an unregistered option as a hard error rather than a warning, still - * emitting it would make {@code loadModel()} throw {@code "Failed to parse model parameters"} - * instead of loading the model. Writing nothing keeps existing call sites compiling and - * loading. The method will be removed in a future release; the contract is enforced by - * {@code src/test/cpp/test_model_flags.cpp}, which drives every emitted flag through the real - * server option table.

- * - * @return this builder - * @deprecated upstream removed {@code --dump-kv-cache} with no replacement - */ - // Error Prone's InlineMeSuggester wants an @InlineMe here because the body is a single - // expression. Inlining would be exactly wrong: the point of the deprecation is that callers - // keep calling THIS method, so a later removal is one edit here and not a code search. - @SuppressWarnings("InlineMeSuggester") - @Deprecated - public ModelParameters enableDumpKvCache() { - return this; - } - /** * Disable KV offload. * @@ -915,52 +834,6 @@ public ModelParameters setLoadMode(LoadMode loadMode) { return putEnum("--load-mode", loadMode); } - /** - * Force system to keep model in RAM rather than swapping or compressing. - * - *

Now emits {@code --load-mode mlock}. Upstream deprecated {@code --mlock} - * at b10092 and deleted it at b10878; since llama.cpp's argument parser treats an unknown - * option as a hard error rather than a warning, continuing to emit it would make - * {@code loadModel()} throw {@code "Failed to parse model parameters"}. The substitution is - * upstream's own — its deprecation shim mapped {@code --mlock} to - * {@code LLAMA_LOAD_MODE_MLOCK} — so behaviour is unchanged. Prefer - * {@link #setLoadMode(LoadMode)} directly; this method will be removed in a future release.

- * - * @return this builder - * @deprecated use {@link #setLoadMode(LoadMode)} with {@link LoadMode#MLOCK} - */ - // Error Prone's InlineMeSuggester wants an @InlineMe here because the body is a single - // expression. Inlining would be exactly wrong: the point of the deprecation is that callers - // keep calling THIS method, so a later removal is one edit here and not a code search. - @SuppressWarnings("InlineMeSuggester") - @Deprecated - public ModelParameters enableMlock() { - return setLoadMode(LoadMode.MLOCK); - } - - /** - * Do not memory-map model (slower load but may reduce pageouts if not using mlock). - * - *

Now emits {@code --load-mode none}. Upstream deprecated {@code --no-mmap} - * at b10092 and deleted it at b10878; the substitution is upstream's own deprecation-shim - * mapping ({@code --no-mmap} to {@code LLAMA_LOAD_MODE_NONE}), so behaviour is unchanged. Note - * that this is a whole loading mode, not an independent switch: a later - * {@link #setLoadMode(LoadMode)} call overrides it, and combining "no mmap" with mlock is - * expressed as {@link LoadMode#MLOCK} rather than as two calls. Prefer - * {@link #setLoadMode(LoadMode)} directly; this method will be removed in a future release.

- * - * @return this builder - * @deprecated use {@link #setLoadMode(LoadMode)} with {@link LoadMode#NONE} - */ - // Error Prone's InlineMeSuggester wants an @InlineMe here because the body is a single - // expression. Inlining would be exactly wrong: the point of the deprecation is that callers - // keep calling THIS method, so a later removal is one edit here and not a code search. - @SuppressWarnings("InlineMeSuggester") - @Deprecated - public ModelParameters disableMmap() { - return setLoadMode(LoadMode.NONE); - } - /** * Set NUMA optimization type for system. * @@ -1189,60 +1062,6 @@ public ModelParameters setHfFile(String hfFile) { return this; } - /** - * Set the Hugging Face model repository for the vocoder model (default: unused). - * - *

No longer emitted — this method is a no-op. Upstream removed {@code --hf-repo-v} with the - * OuteTTS-era two-model TTS design; it appears nowhere in llama.cpp at the pinned build. The - * current TTS pipeline takes a backbone plus an mmproj GGUF — see - * {@link net.ladenthin.llama.TextToSpeech}. Because llama.cpp's - * argument parser treats an unregistered option as a hard error rather than a warning, still - * emitting it would make {@code loadModel()} throw {@code "Failed to parse model parameters"} - * instead of loading the model. Writing nothing keeps existing call sites compiling and - * loading. The method will be removed in a future release; the contract is enforced by - * {@code src/test/cpp/test_model_flags.cpp}, which drives every emitted flag through the real - * server option table.

- * - * @param hfRepoV the Hugging Face repository for the vocoder model - * @return this builder - * @deprecated upstream removed {@code --hf-repo-v}; see {@link net.ladenthin.llama.TextToSpeech} - */ - // Error Prone's InlineMeSuggester wants an @InlineMe here because the body is a single - // expression. Inlining would be exactly wrong: the point of the deprecation is that callers - // keep calling THIS method, so a later removal is one edit here and not a code search. - @SuppressWarnings("InlineMeSuggester") - @Deprecated - public ModelParameters setHfRepoV(String hfRepoV) { - return this; - } - - /** - * Set the Hugging Face model file for the vocoder model (default: unused). - * - *

No longer emitted — this method is a no-op. Upstream removed {@code --hf-file-v} with the - * OuteTTS-era two-model TTS design; it appears nowhere in llama.cpp at the pinned build. The - * current TTS pipeline takes a backbone plus an mmproj GGUF — see - * {@link net.ladenthin.llama.TextToSpeech}. Because llama.cpp's - * argument parser treats an unregistered option as a hard error rather than a warning, still - * emitting it would make {@code loadModel()} throw {@code "Failed to parse model parameters"} - * instead of loading the model. Writing nothing keeps existing call sites compiling and - * loading. The method will be removed in a future release; the contract is enforced by - * {@code src/test/cpp/test_model_flags.cpp}, which drives every emitted flag through the real - * server option table.

- * - * @param hfFileV the vocoder model file within the Hugging Face repository - * @return this builder - * @deprecated upstream removed {@code --hf-file-v}; see {@link net.ladenthin.llama.TextToSpeech} - */ - // Error Prone's InlineMeSuggester wants an @InlineMe here because the body is a single - // expression. Inlining would be exactly wrong: the point of the deprecation is that callers - // keep calling THIS method, so a later removal is one edit here and not a code search. - @SuppressWarnings("InlineMeSuggester") - @Deprecated - public ModelParameters setHfFileV(String hfFileV) { - return this; - } - /** * Set the Hugging Face access token (default: value from HF_TOKEN environment variable). * diff --git a/llama/src/main/java/net/ladenthin/llama/parameters/ParameterJsonSerializer.java b/llama/src/main/java/net/ladenthin/llama/parameters/ParameterJsonSerializer.java index 7824b22a2..6e2f48ee4 100644 --- a/llama/src/main/java/net/ladenthin/llama/parameters/ParameterJsonSerializer.java +++ b/llama/src/main/java/net/ladenthin/llama/parameters/ParameterJsonSerializer.java @@ -184,19 +184,6 @@ public ArrayNode buildSamplers(Sampler... samplers) { return arr; } - /** - * Build a JSON integer array from a primitive {@code int[]} - * (used for penalty-prompt token sequences). - * - * @param values the token IDs to include - * @return a Jackson {@link ArrayNode} of integer values - */ - public ArrayNode buildIntArray(int... values) { - ArrayNode arr = OBJECT_MAPPER.createArrayNode(); - for (int v : values) arr.add(v); - return arr; - } - // ------------------------------------------------------------------ // Logit-bias pair arrays — [[key, value], ...] // ------------------------------------------------------------------ diff --git a/llama/src/test/java/examples/ChatExample.java b/llama/src/test/java/examples/ChatExample.java index 30d9322ac..0c544b984 100644 --- a/llama/src/test/java/examples/ChatExample.java +++ b/llama/src/test/java/examples/ChatExample.java @@ -33,9 +33,7 @@ public static void main(String... args) throws Exception { String input = reader.readLine(); messages.add(new Pair<>("user", input)); StringBuilder response = new StringBuilder(); - InferenceParameters inferParams = new InferenceParameters("") - .withMessages(system, messages) - .withUseChatTemplate(true); + InferenceParameters inferParams = new InferenceParameters("").withMessages(system, messages); System.out.print("Assistant: "); for (LlamaOutput output : model.generate(inferParams)) { System.out.print(output); diff --git a/llama/src/test/java/examples/MainExample.java b/llama/src/test/java/examples/MainExample.java index 32bf331f0..627eea175 100644 --- a/llama/src/test/java/examples/MainExample.java +++ b/llama/src/test/java/examples/MainExample.java @@ -40,7 +40,6 @@ public static void main(String... args) throws IOException { prompt += "\nLlama: "; InferenceParameters inferParams = new InferenceParameters(prompt) .withTemperature(0.7f) - .withPenalizeNl(true) .withMiroStat(MiroStat.V2) .withStopStrings("User:"); for (LlamaOutput output : model.generate(inferParams)) { diff --git a/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java b/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java index d44f8f374..caf756d46 100644 --- a/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java +++ b/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java @@ -32,16 +32,15 @@ *
  • nPredict=-1 with stop string — unbounded generation terminates
    • *
  • setNProbs — streaming JSON contains probability data
    • *
  • setChatTemplate — custom Jinja template applied by applyTemplate
    • - *
  • setUseChatTemplate(true) in generate() — template applied in raw path
    • + *
  • withMessages in generate() — the model's chat template applied in the raw path
    • *
  • setRepeatPenalty + setFrequencyPenalty + setPresencePenalty
    • *
  • setSamplers — custom sampler chain
    • *
  • setMiroStat V2 — alternative sampler path
    • *
  • requestCompletion direct streaming (non-chat)
    • *
  • disableTokenIds — logit bias to negative-infinity
    • - *
  • setPenaltyPrompt(String) and setPenaltyPrompt(int[]) accepted
    • *
  • setNKeep — number of prompt tokens preserved
    • *
  • Multiple stop strings — first match terminates generation
    • - *
  • setMinP / setTfsZ / setTypicalP — alternative sampler params
    • + *
  • setMinP / setTypicalP — alternative sampler params
    • * */ @ClaudeGenerated( @@ -213,23 +212,26 @@ public void testCustomChatTemplateAcceptedWithoutError() { } // ------------------------------------------------------------------ - // 5. setUseChatTemplate(true) in generate() — template in raw path + // 5. Messages in the raw generate() path // ------------------------------------------------------------------ /** - * {@link InferenceParameters#setUseChatTemplate(boolean)} enables chat - * template application inside the raw {@code generate()} path (not via - * {@code generateChat()}). Combined with {@code setMessages()}, the - * generation must produce non-empty output and must not throw. + * {@code withMessages()} drives the raw {@code generate()} path (not + * {@code generateChat()}): the server applies the model's chat template to the + * message array, so the generation must produce non-empty output and must not throw. + * + *

    This test used to also pass {@code withUseChatTemplate(true)}. That was never + * read by the server — jinja is a launch-time option, and no {@code "use_jinja"} + * request key has existed at any pin this project has carried — so the test measured + * {@code withMessages()} alone the whole time. It says so now. */ @Test - public void testUseChatTemplateInGenerate() { + public void testMessagesInGenerate() { List> messages = new ArrayList<>(); messages.add(new Pair<>("user", "Write one word.")); InferenceParameters params = new InferenceParameters("") .withMessages(null, messages) - .withUseChatTemplate(true) .withNPredict(N_PREDICT) .withSeed(42) .withTemperature(0.0f); @@ -239,7 +241,7 @@ public void testUseChatTemplateInGenerate() { output.append(token.text); } - assertFalse(output.toString().isEmpty(), "generate() with use_chat_template must produce output"); + assertFalse(output.toString().isEmpty(), "generate() with messages must produce output"); } // ------------------------------------------------------------------ @@ -391,43 +393,7 @@ public void testDisableTokenIdsAccepted() { } // ------------------------------------------------------------------ - // 11. setPenaltyPrompt(String) and setPenaltyPrompt(int[]) accepted - // ------------------------------------------------------------------ - - /** - * Both overloads of {@code setPenaltyPrompt} must be accepted without error - * and must produce non-empty output. The string form restricts which part of - * the prompt is penalised; the token-array form does the same by ID. - */ - @Test - public void testPenaltyPromptStringAccepted() { - InferenceParameters params = new InferenceParameters(SIMPLE_PROMPT) - .withNPredict(N_PREDICT) - .withSeed(42) - .withTemperature(0.0f) - .withPenaltyPrompt("def ") - .withRepeatPenalty(1.2f); - - assertFalse(model.complete(params).isEmpty(), "setPenaltyPrompt(String) must produce output"); - } - - @Test - public void testPenaltyPromptTokenArrayAccepted() { - int[] penaltyTokens = model.encode("def "); - Assumptions.assumeTrue(penaltyTokens.length > 0, "Need at least one penalty token"); - - InferenceParameters params = new InferenceParameters(SIMPLE_PROMPT) - .withNPredict(N_PREDICT) - .withSeed(42) - .withTemperature(0.0f) - .withPenaltyPrompt(penaltyTokens) - .withRepeatPenalty(1.2f); - - assertFalse(model.complete(params).isEmpty(), "setPenaltyPrompt(int[]) must produce output"); - } - - // ------------------------------------------------------------------ - // 12. Multiple stop strings — first match terminates + // 11. Multiple stop strings — first match terminates // ------------------------------------------------------------------ /** @@ -453,12 +419,12 @@ public void testMultipleStopStringsFirstMatchTerminates() { } // ------------------------------------------------------------------ - // 13. Alternative sampler parameters: minP, tfsZ, typicalP + // 12. Alternative sampler parameters: minP, typicalP // ------------------------------------------------------------------ /** - * {@code setMinP()}, {@code setTfsZ()}, and {@code setTypicalP()} are - * alternative token-filtering parameters. Each must be individually accepted + * {@code setMinP()} and {@code setTypicalP()} are alternative token-filtering + * parameters. Each must be individually accepted * by the native layer and must produce non-empty output. */ @Test @@ -472,17 +438,6 @@ public void testMinPSamplerAccepted() { assertFalse(model.complete(params).isEmpty(), "setMinP must produce output"); } - @Test - public void testTfsZSamplerAccepted() { - InferenceParameters params = new InferenceParameters(SIMPLE_PROMPT) - .withNPredict(N_PREDICT) - .withSeed(42) - .withTemperature(0.7f) - .withTfsZ(0.95f); - - assertFalse(model.complete(params).isEmpty(), "setTfsZ must produce output"); - } - @Test public void testTypicalPSamplerAccepted() { InferenceParameters params = new InferenceParameters(SIMPLE_PROMPT) @@ -495,7 +450,7 @@ public void testTypicalPSamplerAccepted() { } // ------------------------------------------------------------------ - // 14. setNKeep — prompt token preservation + // 13. setNKeep — prompt token preservation // ------------------------------------------------------------------ /** @@ -515,7 +470,7 @@ public void testNKeepAllTokensAccepted() { } // ------------------------------------------------------------------ - // 15. disableTokens (string form) — accepted without crash + // 14. disableTokens (string form) — accepted without crash // ------------------------------------------------------------------ /** @@ -536,7 +491,7 @@ public void testDisableTokensStringFormAccepted() { } // ------------------------------------------------------------------ - // 16. MiroStat V1 — first-generation algorithm path + // 15. MiroStat V1 — first-generation algorithm path // ------------------------------------------------------------------ /** diff --git a/llama/src/test/java/net/ladenthin/llama/ToolCallingIntegrationTest.java b/llama/src/test/java/net/ladenthin/llama/ToolCallingIntegrationTest.java index 7d9417392..bd5a1a78a 100644 --- a/llama/src/test/java/net/ladenthin/llama/ToolCallingIntegrationTest.java +++ b/llama/src/test/java/net/ladenthin/llama/ToolCallingIntegrationTest.java @@ -80,8 +80,7 @@ public void requiredToolCallIsParsedFromStreamingResponse() throws IOException { .withMessagesJson(request.buildMessagesJson()) .withToolsJson(request.buildToolsJson().orElseThrow()) .withToolChoice(request.getToolChoice().orElseThrow()) - .withParallelToolCalls(request.getParallelToolCalls().orElseThrow()) - .withUseChatTemplate(true)); + .withParallelToolCalls(request.getParallelToolCalls().orElseThrow())); List chunks = new ArrayList(); model.streamChatCompletion(params, chunks::add); diff --git a/llama/src/test/java/net/ladenthin/llama/json/ParameterJsonSerializerTest.java b/llama/src/test/java/net/ladenthin/llama/json/ParameterJsonSerializerTest.java index 6ed085cdb..4db9a62d6 100644 --- a/llama/src/test/java/net/ladenthin/llama/json/ParameterJsonSerializerTest.java +++ b/llama/src/test/java/net/ladenthin/llama/json/ParameterJsonSerializerTest.java @@ -188,32 +188,6 @@ public void testBuildSamplers_single() { assertThat(arr.get(0).asText(), is("temperature")); } - // ------------------------------------------------------------------ - // buildIntArray - // ------------------------------------------------------------------ - - @Test - public void testBuildIntArray_values() { - ArrayNode arr = serializer.buildIntArray(new int[] {1, 2, 3}); - assertThat(arr.size(), is(3)); - assertThat(arr.get(0).asInt(), is(1)); - assertThat(arr.get(2).asInt(), is(3)); - } - - @Test - public void testBuildIntArray_empty() { - ArrayNode arr = serializer.buildIntArray(new int[] {}); - assertThat(arr.size(), is(0)); - } - - @Test - public void testBuildIntArray_roundtripsAsJson() throws Exception { - ArrayNode arr = serializer.buildIntArray(new int[] {10, 20}); - JsonNode parsed = serializer.OBJECT_MAPPER.readTree(arr.toString()); - assertThat(parsed.isArray(), is(true)); - assertThat(parsed.get(0).asInt(), is(10)); - } - // ------------------------------------------------------------------ // buildTokenIdBiasArray // ------------------------------------------------------------------ diff --git a/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java b/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java index 7c87cdd60..fdd1e4c84 100644 --- a/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java +++ b/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java @@ -159,12 +159,6 @@ public void testSetMinP() { assertThat(params.parameters.get("min_p"), is("0.1")); } - @Test - public void testSetTfsZ() { - InferenceParameters params = new InferenceParameters("").withTfsZ(1.0f); - assertThat(params.parameters.get("tfs_z"), is("1.0")); - } - @Test public void testSetTypicalP() { InferenceParameters params = new InferenceParameters("").withTypicalP(0.8f); @@ -231,12 +225,6 @@ public void testSetIgnoreEos() { assertThat(params.parameters.get("ignore_eos"), is("true")); } - @Test - public void testSetPenalizeNl() { - InferenceParameters params = new InferenceParameters("").withPenalizeNl(false); - assertThat(params.parameters.get("penalize_nl"), is("false")); - } - @Test public void testSetDynamicTemperatureRange() { InferenceParameters params = new InferenceParameters("").withDynamicTemperatureRange(0.5f); @@ -279,18 +267,6 @@ public void testSetJsonSchemaStoresVerbatim() { assertThat(params.toString(), containsString("\"json_schema\": " + schema)); } - @Test - public void testSetPenaltyPromptString() { - InferenceParameters params = new InferenceParameters("").withPenaltyPrompt("Hello!"); - assertThat(params.parameters.get("penalty_prompt"), is("\"Hello!\"")); - } - - @Test - public void testSetUseChatTemplate() { - InferenceParameters params = new InferenceParameters("").withUseChatTemplate(true); - assertThat(params.parameters.get("use_jinja"), is("true")); - } - @Test public void testSetChatTemplate() { InferenceParameters params = new InferenceParameters("").withChatTemplate("{{messages}}"); @@ -564,23 +540,6 @@ public void testDisableTokensEmpty() { assertThat(params.parameters, not(hasKey("logit_bias"))); } - // ------------------------------------------------------------------------- - // Penalty prompt with token ids - // ------------------------------------------------------------------------- - - @Test - public void testSetPenaltyPromptTokenIds() { - InferenceParameters params = new InferenceParameters("").withPenaltyPrompt(new int[] {1, 2, 3}); - assertThat(params.parameters.get("penalty_prompt"), is("[1,2,3]")); - } - - @Test - public void testSetPenaltyPromptTokenIdsEmpty() { - InferenceParameters params = new InferenceParameters(""); - params = params.withPenaltyPrompt(new int[] {}); - assertThat(params.parameters, not(hasKey("penalty_prompt"))); - } - // ------------------------------------------------------------------------- // setMessages // ------------------------------------------------------------------------- diff --git a/llama/src/test/java/net/ladenthin/llama/parameters/ModelParametersExtendedTest.java b/llama/src/test/java/net/ladenthin/llama/parameters/ModelParametersExtendedTest.java index 6d384c99b..9e37aae3e 100644 --- a/llama/src/test/java/net/ladenthin/llama/parameters/ModelParametersExtendedTest.java +++ b/llama/src/test/java/net/ladenthin/llama/parameters/ModelParametersExtendedTest.java @@ -367,37 +367,6 @@ public void testSetYarnBetaFast() { assertThat(p.parameters.get("--yarn-beta-fast"), is("16.0")); } - // ------------------------------------------------------------------------- - // Group attention - // ------------------------------------------------------------------------- - - /** - * Asserts that a retired builder method wrote nothing at all -- compared against a pristine - * instance rather than against emptiness, because the constructor seeds defaults of its own - * ({@code --fit} today), and a future default must not quietly weaken this assertion. - */ - private static void assertWroteNothing(ModelParameters actual) { - assertThat(actual.parameters, is(new ModelParameters().parameters)); - } - - // Retired flags: llama.cpp's server arg parser does not register these, and it treats an - // unregistered option as a hard error -- so the builder methods must write NOTHING, or every - // caller's model becomes unloadable. Asserting emptiness (not just "no longer that key") is the - // point: the old assertions pinned the mapping and would have passed forever while the flag was - // dead. src/test/cpp/test_model_flags.cpp is the upstream-facing half of this guard. - // --grp-attn-n/-w still exist upstream but are set_examples()-scoped to - // LLAMA_EXAMPLE_COMPLETION/PASSKEY, so the server example never registers them. - - @Test - public void testSetGrpAttnNIsRetiredAndEmitsNothing() { - assertWroteNothing(new ModelParameters().setGrpAttnN(4)); - } - - @Test - public void testSetGrpAttnWIsRetiredAndEmitsNothing() { - assertWroteNothing(new ModelParameters().setGrpAttnW(1024)); - } - // ------------------------------------------------------------------------- // KV cache // ------------------------------------------------------------------------- @@ -437,12 +406,6 @@ public void testDisableKvOffload() { assertThat(p.parameters.get("--no-kv-offload"), is(nullValue())); } - // --dump-kv-cache was removed upstream with no replacement; see the retired-flag note above. - @Test - public void testEnableDumpKvCacheIsRetiredAndEmitsNothing() { - assertWroteNothing(new ModelParameters().enableDumpKvCache()); - } - @Test public void testSetKvUnifiedTrue() { ModelParameters p = new ModelParameters().setKvUnified(true); @@ -600,29 +563,12 @@ public void testSetLoadModeAllValues() { } } - // enableMlock()/disableMmap() kept their names but changed what they emit: upstream deleted - // --mlock and --no-mmap at b10878, and these are the substitutions upstream's own deprecation - // shim used (LLAMA_LOAD_MODE_MLOCK / LLAMA_LOAD_MODE_NONE), so behaviour is unchanged. - - @Test - public void testEnableMlockEmitsLoadModeMlock() { - ModelParameters p = new ModelParameters().enableMlock(); - assertThat(p.parameters.get("--load-mode"), is("mlock")); - assertThat(p.parameters, not(hasKey("--mlock"))); - } - - @Test - public void testDisableMmapEmitsLoadModeNone() { - ModelParameters p = new ModelParameters().disableMmap(); - assertThat(p.parameters.get("--load-mode"), is("none")); - assertThat(p.parameters, not(hasKey("--no-mmap"))); - } - @Test public void testLoadModeIsSingleValuedSoTheLastCallWins() { - // The three used to be independent switches; they are one mutually exclusive mode now, so a - // caller wanting both mmap and mlock must say MMAP_MLOCK rather than chain two calls. - ModelParameters p = new ModelParameters().disableMmap().enableMlock(); + // --mlock, --mmap and --no-mmap used to be independent switches; they are one mutually + // exclusive mode now, so a caller wanting both mmap and mlock must say MMAP_MLOCK rather + // than chain two calls. + ModelParameters p = new ModelParameters().setLoadMode(LoadMode.NONE).setLoadMode(LoadMode.MLOCK); assertThat(p.parameters.get("--load-mode"), is("mlock")); } @@ -913,19 +859,6 @@ public void testSetHfToken() { assertThat(p.parameters.get("--hf-token"), is("hf_abc123")); } - // --hf-repo-v/--hf-file-v went away with the OuteTTS-era two-model TTS design; see the - // retired-flag note above. - - @Test - public void testSetHfRepoVIsRetiredAndEmitsNothing() { - assertWroteNothing(new ModelParameters().setHfRepoV("org/vocoder")); - } - - @Test - public void testSetHfFileVIsRetiredAndEmitsNothing() { - assertWroteNothing(new ModelParameters().setHfFileV("vocoder.gguf")); - } - // ------------------------------------------------------------------------- // Slot / cache reuse // ------------------------------------------------------------------------- From 4d55dd6c664bb8fe278075c042a456483c9044c5 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 07:25:01 +0000 Subject: [PATCH 2/7] fix!: stop a caller-supplied JSON fragment from injecting request fields The request body was assembled by string concatenation over the parameter map, and five public setters take a JSON fragment from the caller and stored it in that map verbatim: withJsonSchema, withResponseFormat, withStreamOptions, withMessagesJson and withToolsJson. A fragment carrying a top-level comma therefore did not only set its own field, it injected SIBLING fields into the body -- and because the native parser resolves duplicate keys last-wins, the injected value won over one the application had already set. Demonstrated against the built classes rather than reasoned about: InferenceParameters.of("hi").withNPredict(16).withCachePrompt(true) .withResponseFormat("{\"type\":\"json_object\"}, \"n_predict\": 999999, \"cache_prompt\": false") {"n_predict": 16, "response_format": {...}, "n_predict": 999999, ...} and nlohmann parsing that body yields n_predict=999999, cache_prompt=false. Any host that builds tools/response_format/messages JSON from something it does not fully control -- an agent tool registry, a client request, a template -- could have its own limits overridden that way. It is now rejected at the call site that supplied the fragment, with the offending key named. The fix is an invariant rather than five patches: every value stored in the map must be EXACTLY ONE well-formed JSON value, checked centrally in withPut. Note FAIL_ON_TRAILING_TOKENS is load-bearing -- plain readTree parses the first value and ignores what follows, so without it the injection payload would have been silently truncated to its first object instead of rejected, trading one quiet wrong answer for another. The invariant immediately caught a latent defect it was not written for: 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 is deleted rather than fixed. This is exactly the class of never-executed surface the previous commit removed. Two further changes follow from the same reasoning: * toJson() is now the wire serializer and LlamaModel calls it at all six payload sites. It renders through Jackson, and emits keys in sorted order -- JSON objects are unordered so the server does not care, but the old renderer walked a HashMap and produced a different byte sequence run to run, which is needlessly hard to diff, log or assert on. * toString() becomes a redacted debug view and deliberately NOT valid JSON: "InferenceParameters{keys=[...], values=redacted}". A parameter set carries the prompt, the message history and the tool definitions, so a log line built from one leaked the whole payload. Making the redacted form unparseable means a caller who still passes toString() to the native layer fails at the parser instead of quietly sending a different body -- a loud incompatibility rather than a silent one. JsonParameters joins the PIT gate (targetClasses), since it now carries a security invariant rather than plumbing. The rest of the parameters package stays off it on purpose: ~200 one-line builder setters would add cost, not signal. Getting it to 100% needed one more test than expected -- the excerpt boundary in the rejection message (the message shows a bounded prefix so a hostile fragment cannot flood a log through it) is observable only at exactly the limit, and is now pinned from both sides. Verified: reactor tests 1748/49/6 green, PIT 334/334 killed at 100% (was 320/320; the 14 new mutations are JsonParameters), SpotBugs 0 bug instances, spotless and javadoc:jar clean. The injection reproducer now prints "REJECTED: response_format must be exactly one well-formed JSON value" and a legitimate fragment still round-trips unchanged. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH --- .../langchain4j/LangChain4jMappingTest.java | 10 +- llama/pom.xml | 10 ++ llama/spotbugs-exclude.xml | 17 ++ .../java/net/ladenthin/llama/LlamaModel.java | 12 +- .../llama/parameters/JsonParameters.java | 167 ++++++++++++------ .../llama/MultimodalMessagesTest.java | 2 +- .../parameters/InferenceParametersTest.java | 93 +++++++++- .../llama/parameters/JsonParametersTest.java | 94 ++++++---- .../llama/server/OpenAiRequestMapperTest.java | 2 +- 9 files changed, 304 insertions(+), 103 deletions(-) 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 018f7ce1e..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\"")); @@ -296,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")); @@ -319,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\"")); @@ -332,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/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 + + + + + + + + +# 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 four-commit 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. 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. + +## 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. An + `OAI_LAYER`-declared key is checked only for *absence* from the schema; that it is genuinely read + by `oaicompat_*_params_parse` is documented, not asserted. +- 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. From ce47839e3c0ca9f2b02a8052ea76043a774c75d2 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 11:19:01 +0000 Subject: [PATCH 6/7] fix!: prove an OAI_LAYER request key is read, and delete the one that was not The OAI_LAYER contract exempts a key from the request-schema check because the OpenAI/task layer consumes it before make_llama_cmpl_schema sees the body. The test asserted exactly that -- the key is *not* in the schema -- on the inverted reasoning that an exemption can only rot by upstream later adopting the name. That reasoning was incomplete. Absence from the schema is satisfied just as 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 name whose only occurrence in upstream sources is the /props payload the server *emits*. Driving the parsers directly cannot close it -- the eleven keys have three different consumers and two need a live server_context -- so the generator now sweeps upstream's own sources for a reader *shape* (json_value(x, "k", ...), .contains("k"), .at("k")) over globbed tools/server/*.cpp + common/*.cpp and emits the hit count. The shape is load-bearing: chat_template does occur as a bare literal, so a token grep would have called it live. Measured at b10883, every other OAI_LAYER key has at least one reader; chat_template has zero. EveryOaiLayerKeyIsReadSomewhereUpstream fails on a count of zero, naming the key and the remedy, and fails loud if the swept set or the source corpus is empty. Verified by falsification: run against the tree that still declared the key, it printed exactly that. The dead name is deleted, not deprecated (registry rule 2). One consumer was affected: the Android app passed its chat-template override per request, where llama.cpp discarded it -- it now sets it at load time, which is where upstream reads it. Two tests pinned the dead key and went with it; one of them explained its own missing behavioural assertion with the wrong cause. Also adds the CHANGELOG entries this branch was still missing, and repoints two javadoc references to the extractor's post-rename filename. ctest 537/537, mvn test 1754/0 failures, SpotBugs 0, PIT 337/337 (100%), javadoc clean, reactor build green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH --- CHANGELOG.md | 57 +++++++++++++++ CLAUDE.md | 18 ++++- TODO.md | 23 +++--- .../android/llmservice/ChatViewModel.kt | 7 +- android-llmservice/requirements.md | 2 +- docs/history/parameter-wire-surface.md | 63 ++++++++++++++-- llama/CMakeLists.txt | 8 ++ llama/cmake/extract-java-wire-names.cmake | 73 ++++++++++++++++++- .../net/ladenthin/llama/args/ModelOption.java | 2 +- .../llama/parameters/InferenceParameters.java | 22 ------ .../llama/parameters/RequestField.java | 5 +- llama/src/test/cpp/test_wire_contracts.cpp | 31 ++++++++ .../net/ladenthin/llama/ChatAdvancedTest.java | 52 ++----------- .../parameters/InferenceParametersTest.java | 6 -- 14 files changed, 260 insertions(+), 109 deletions(-) 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 6c5f1ca6d..3cd8299a9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -659,7 +659,10 @@ satisfy: `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. +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. @@ -678,7 +681,14 @@ Either way a Java test asserting the string mapping (`hasKey("--mlock")`) passes 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. **`WireNameRegistryTest` checks the other direction**: every declared constant must be reachable +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. @@ -1519,9 +1529,9 @@ ctest --test-dir build --output-on-failure -R "ResultsToJson" | `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 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` | 5 | **The same contract for the two quieter surfaces.** `RequestField` against `server_schema::make_llama_cmpl_schema(...)` (4 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, checked the same inverted way as `PROJECT_PSEUDO` above. See [`docs/history/parameter-wire-surface.md`](docs/history/parameter-wire-surface.md). | +| `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: 536 tests (all passing).** +**Current total: 537 tests (all passing).** #### Upstream source location (in CMake build tree) diff --git a/TODO.md b/TODO.md index f77f2808b..efb3f5978 100644 --- a/TODO.md +++ b/TODO.md @@ -148,19 +148,16 @@ These are JNI plumbing items for upstream API additions. Policy: add only after 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 now countable instead of arguable: of llama.cpp's completion-request schema, the Java layer - writes 47 of its keys. Ones it does not write include `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. - -- **An `OAI_LAYER` request key is checked only for *absence* from the schema.** That it is genuinely - read by `oaicompat_*_params_parse` or the task layer is documented on the constant, not asserted. - Closing it means driving those parsers from a C++ test the way `make_llama_cmpl_schema` is driven - now — feasible, but the eleven keys are stable OpenAI-protocol names, so this is low priority. + 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 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 index d0303d87f..0593907d7 100644 --- a/docs/history/parameter-wire-surface.md +++ b/docs/history/parameter-wire-surface.md @@ -10,13 +10,14 @@ This library sends names on three wires — CLI options in the argv that loads 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 four-commit 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 +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. The interesting part is not that they were dead — it is **when** +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 @@ -169,13 +170,63 @@ now goes through a `jllama_train::keys` constant per field and `config_keys()` r 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. An - `OAI_LAYER`-declared key is checked only for *absence* from the schema; that it is genuinely read - by `oaicompat_*_params_parse` is documented, not asserted. +- `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/CMakeLists.txt b/llama/CMakeLists.txt index b519bd234..9b370e2fe 100644 --- a/llama/CMakeLists.txt +++ b/llama/CMakeLists.txt @@ -566,6 +566,9 @@ if(BUILD_TESTING) # 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 @@ -582,6 +585,11 @@ if(BUILD_TESTING) 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 diff --git a/llama/cmake/extract-java-wire-names.cmake b/llama/cmake/extract-java-wire-names.cmake index 8b6ed1445..d0a70be36 100644 --- a/llama/cmake/extract-java-wire-names.cmake +++ b/llama/cmake/extract-java-wire-names.cmake @@ -29,14 +29,34 @@ # 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[],

    _COUNT +# 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" "JAVA_SOURCES" ${ARGN}) + 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}) @@ -88,16 +108,55 @@ function(jllama_extract_java_wire_names) "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}=(.*)$") - string(APPEND _contract_body " \"${CMAKE_MATCH_1}\",\n") + 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") @@ -105,6 +164,7 @@ function(jllama_extract_java_wire_names) 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 @@ -122,5 +182,10 @@ function(jllama_extract_java_wire_names) set_property(DIRECTORY APPEND PROPERTY CMAKE_CONFIGURE_DEPENDS "${_src}") endforeach() - message(STATUS "jllama: extracted ${_count} ${ARG_ARRAY_PREFIX} names -> ${ARG_OUTPUT_HEADER}") + 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/src/main/java/net/ladenthin/llama/args/ModelOption.java b/llama/src/main/java/net/ladenthin/llama/args/ModelOption.java index e48431af6..7ddd2589f 100644 --- a/llama/src/main/java/net/ladenthin/llama/args/ModelOption.java +++ b/llama/src/main/java/net/ladenthin/llama/args/ModelOption.java @@ -13,7 +13,7 @@ * {@code common_params_parse} — the emitted set is closed by construction rather than by review. * *

    {@link ModelFlag} is the same registry for the options that take no value. Both are read at - * configure time by {@code cmake/extract-java-cli-flags.cmake} and checked against the real server + * configure time by {@code cmake/extract-java-wire-names.cmake} and checked against the real server * option table by {@code src/test/cpp/test_model_flags.cpp}, which runs on every platform. * * @see CliContract diff --git a/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java b/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java index 62db7b795..5d939a4e2 100644 --- a/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java +++ b/llama/src/main/java/net/ladenthin/llama/parameters/InferenceParameters.java @@ -547,28 +547,6 @@ public InferenceParameters withSamplers(Sampler... samplers) { return withRaw(RequestField.SAMPLERS, serializer.buildSamplers(samplers).toString()); } - /** - * Returns a new request with the chat-template string replaced. - * - *

    Ignored by the server. The chat template is chosen when the model is - * loaded, not per request: on the pinned build the only {@code "chat_template"} string in - * {@code common/} or {@code tools/server/} is the one the server emits in its - * {@code /props} response, and nothing reads it from a request body. The request schema - * silently discards unknown fields, so a template passed here is never applied. Use - * {@link net.ladenthin.llama.parameters.ModelParameters#setChatTemplate(String)} instead. - * Retained only so existing call sites keep compiling; it will be removed in a future - * release.

    - * - * @param chatTemplate the Jinja-style chat template to use; {@code null} clears - * @return a new instance; this instance is unchanged - * @deprecated the chat template is a load-time option; the request field is discarded by the - * server. Use {@link net.ladenthin.llama.parameters.ModelParameters#setChatTemplate(String)} - */ - @Deprecated - public InferenceParameters withChatTemplate(@Nullable String chatTemplate) { - return withOptionalJson(RequestField.CHAT_TEMPLATE, chatTemplate); - } - /** * Returns a new request with custom Jinja template kwargs replaced. Values must be * valid JSON. diff --git a/llama/src/main/java/net/ladenthin/llama/parameters/RequestField.java b/llama/src/main/java/net/ladenthin/llama/parameters/RequestField.java index 9962b43e1..dbb8b3b0d 100644 --- a/llama/src/main/java/net/ladenthin/llama/parameters/RequestField.java +++ b/llama/src/main/java/net/ladenthin/llama/parameters/RequestField.java @@ -12,7 +12,7 @@ * the point is a closed set, not an escape hatch — a new key is added by declaring a constant with * the contract it satisfies, which is what makes it visible to the guard. * - *

    Read at configure time by {@code cmake/extract-java-cli-flags.cmake} and checked against + *

    Read at configure time by {@code cmake/extract-java-wire-names.cmake} and checked against * llama.cpp's own request-schema field table by the C++ test suite. * * @see RequestContract @@ -22,9 +22,6 @@ enum RequestField { /** Request key {@code cache_prompt}. */ CACHE_PROMPT("cache_prompt"), - /** Request key {@code chat_template}, consumed by the OAI/task layer. */ - CHAT_TEMPLATE("chat_template", RequestContract.OAI_LAYER), - /** Request key {@code chat_template_kwargs}, consumed by the OAI/task layer. */ CHAT_TEMPLATE_KWARGS("chat_template_kwargs", RequestContract.OAI_LAYER), diff --git a/llama/src/test/cpp/test_wire_contracts.cpp b/llama/src/test/cpp/test_wire_contracts.cpp index 5ba12bb81..d0b7bb399 100644 --- a/llama/src/test/cpp/test_wire_contracts.cpp +++ b/llama/src/test/cpp/test_wire_contracts.cpp @@ -142,6 +142,37 @@ TEST(JavaRequestFieldContract, OaiLayerKeysAreStillOutsideTheSchema) { "missing, which would exempt nothing and check everything by luck"; } +// The other half of that exemption, and the half that was missing. Absence from the schema is +// satisfied just as well by a key nothing reads at all, so the check above passed for +// `chat_template` -- a public builder method writing a name whose only occurrence upstream was in +// the `/props` payload the server *emits*. There is no callable table to ask which keys +// oaicompat_*_params_parse reads, so the generator sweeps the receiver's own source for a reader +// *shape* (`json_value(x, "k", ...)`, `.contains("k")`, `.at("k")`) and reports the hit count +// here. That is weaker than driving the parser: it proves a key is read from some body, not that +// this endpoint reads it. It is enough for the failure that actually occurred, which is a count of +// zero. +TEST(JavaRequestFieldContract, EveryOaiLayerKeyIsReadSomewhereUpstream) { + int swept = 0; + for (int i = 0; i < JLLAMA_JAVA_REQUEST_COUNT; ++i) { + if (!is_oai_layer(i)) { + EXPECT_EQ(JLLAMA_JAVA_REQUEST_READERS[i], -1) + << JLLAMA_JAVA_REQUEST_NAMES[i] + << " was swept although it is not OAI_LAYER -- the generator's contract filter and " + "this test disagree about which names the sweep covers"; + continue; + } + ++swept; + EXPECT_GT(JLLAMA_JAVA_REQUEST_READERS[i], 0) + << JLLAMA_JAVA_REQUEST_NAMES[i] + << " is declared OAI_LAYER but no upstream source reads it from a request body. The " + "schema discards it and the OAI layer never looks at it, so the builder method " + "writing it is a no-op -- delete the constant and its method rather than " + "re-labelling the contract"; + } + EXPECT_GT(swept, 0) << "the sweep covered no key at all; READER_CONTRACT in CMakeLists.txt no " + "longer matches any declared contract"; +} + // The trainer contract. Both ends are ours, which makes it easy to assume it cannot drift -- but // the parser reads with `j.value(key, default)`, so a rename on either side turns a configured // knob into its default with no error, and LlamaTrainerIntegrationTest is gated on a system diff --git a/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java b/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java index caf756d46..ee913f52f 100644 --- a/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java +++ b/llama/src/test/java/net/ladenthin/llama/ChatAdvancedTest.java @@ -170,49 +170,7 @@ public void testSetNProbsStreamingJsonHasProbabilities() { } // ------------------------------------------------------------------ - // 4. setChatTemplate — custom Jinja template applied by applyTemplate - // ------------------------------------------------------------------ - - /** - * {@link InferenceParameters#setChatTemplate(String)} puts a custom Jinja2 - * template in the request JSON. The server may or may not apply it depending - * on whether the model has a compiled (peg-native) built-in template — if - * one exists, the built-in template takes precedence over the per-request - * {@code chat_template} field for the {@code applyTemplate()} code path. - *

    - * This test therefore verifies the parameter is: - *

      - *
    1. Serialised correctly by {@link InferenceParameters} (no JSON error)
      2. - *
    2. Accepted by the native layer without throwing
      3. - *
    3. Producing a non-empty result that contains the message content
      4. - *
    - * Behavioural verification that the custom filter ({@code | upper}) is - * applied is intentionally omitted because the CodeLlama model's embedded - * ChatML template overrides the per-request template for this endpoint. - */ - @Test - public void testCustomChatTemplateAcceptedWithoutError() { - List> messages = new ArrayList<>(); - messages.add(new Pair<>("user", "hello world")); - - // A custom template using Jinja2 | upper filter - String customTemplate = "{% for m in messages %}" + "{{ m.role | upper }}: {{ m.content }}" + "{% endfor %}"; - - InferenceParameters params = - new InferenceParameters("").withMessages(null, messages).withChatTemplate(customTemplate); - - // Must not throw; parameter is accepted and forwarded to native layer - String result = model.applyTemplate(params); - - assertNotNull(result, "applyTemplate with setChatTemplate must return non-null"); - assertFalse(result.isEmpty(), "applyTemplate with setChatTemplate must return non-empty result"); - assertTrue( - result.contains("hello world"), - "Result must contain the message content 'hello world' regardless of template used"); - } - - // ------------------------------------------------------------------ - // 5. Messages in the raw generate() path + // 4. Messages in the raw generate() path // ------------------------------------------------------------------ /** @@ -245,7 +203,7 @@ public void testMessagesInGenerate() { } // ------------------------------------------------------------------ - // 6. Penalty params — repeatPenalty, frequencyPenalty, presencePenalty + // 5. Penalty params — repeatPenalty, frequencyPenalty, presencePenalty // ------------------------------------------------------------------ /** @@ -269,7 +227,7 @@ public void testRepeatAndFrequencyAndPresencePenalty() { } // ------------------------------------------------------------------ - // 7. setSamplers — custom sampler chain + // 6. setSamplers — custom sampler chain // ------------------------------------------------------------------ /** @@ -292,7 +250,7 @@ public void testCustomSamplerChain() { } // ------------------------------------------------------------------ - // 8. MiroStat V2 — alternative sampler path + // 7. MiroStat V2 — alternative sampler path // ------------------------------------------------------------------ /** @@ -314,7 +272,7 @@ public void testMiroStatV2Sampling() { } // ------------------------------------------------------------------ - // 9. requestCompletion direct streaming (non-chat) + // 8. requestCompletion direct streaming (non-chat) // ------------------------------------------------------------------ /** diff --git a/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java b/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java index ce3fb028b..148a5dbc6 100644 --- a/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java +++ b/llama/src/test/java/net/ladenthin/llama/parameters/InferenceParametersTest.java @@ -332,12 +332,6 @@ public void rawJsonEntryPointsAcceptWellFormedFragments() { assertThat(params.parameters.get("json_schema"), is(schema)); } - @Test - public void testSetChatTemplate() { - InferenceParameters params = new InferenceParameters("").withChatTemplate("{{messages}}"); - assertThat(params.parameters.get("chat_template"), is("\"{{messages}}\"")); - } - @Test public void testSetChatTemplateKwargs() { java.util.Map kwargs = new java.util.LinkedHashMap<>(); From b3cac4382ec72a912391744e14f4b9d367108f94 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 20:22:27 +0000 Subject: [PATCH 7/7] docs: keep one record for the spec-synth flags, not two TODO.md carried `--spec-synth-len` / `--spec-synth-rates` twice: once as a standalone entry from the b10649 bump arguing they should never be exposed, and once as a sub-bullet of the b10878 flag-audit entry restating a shortened version of the same reasoning. Two records of one decision drift apart, and the shorter one reads like a deferral ("no consumer use case here") rather than the settled non-goal it is. The b10649 entry is now the single record and says so; the audit entry points at it. It also names why an audit keeps re-surfacing them: the sweep answers "is this name reachable from the typed Java surface", which is a different question from "should it be" -- so a future re-find is expected and is not a signal to reopen the decision. No behaviour, no code, no counts change. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH --- TODO.md | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/TODO.md b/TODO.md index efb3f5978..1f1305fa0 100644 --- a/TODO.md +++ b/TODO.md @@ -140,9 +140,9 @@ 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. @@ -203,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.).