Skip to content

Commit 027d887

Browse files
committed
fix!: retire the CLI flags llama.cpp's server parser rejects, and guard the contract
`LlamaModel.loadModel(parameters.toArray())` hands the ModelParameters map to `common_params_parse(..., LLAMA_EXAMPLE_SERVER)` as argv, where an unregistered option is a hard error rather than a warning. Seven flags the Java layer emitted are no longer registered, so every caller of the matching builder method got "Failed to parse model parameters" instead of a loaded model. Two of them are fresh: b10878 deleted `--mlock` and `--no-mmap` (deprecated at b10092 -- the whole deprecation window opened and closed inside eight tags). Those have a faithful replacement, so nothing is lost: a new `args.LoadMode` enum + `ModelParameters.setLoadMode(LoadMode)` expose upstream's `-lm`/ `--load-mode`, and `enableMlock()`/`disableMmap()` keep working, re-pointed to `LoadMode.MLOCK` / `LoadMode.NONE` -- the exact mapping upstream's own deprecation shim used. Both are now `@Deprecated`. Five were already dead: `--dump-kv-cache`, `--hf-repo-v` and `--hf-file-v` are gone from llama.cpp entirely; `--grp-attn-n` and `--grp-attn-w` are still in `arg.cpp` but `set_examples()`-scoped to LLAMA_EXAMPLE_COMPLETION/PASSKEY, so the server parser rejects them exactly like a deleted flag. Their setters are now `@Deprecated` no-ops that write nothing, keeping call sites compiling *and* loading. `ModelFlag.MLOCK`/`NO_MMAP`/`DUMP_KV_CACHE` are removed from the enum so the broken argv is not reachable through `setFlag` either -- the same reasoning that already excluded `FLASH_ATTN`. No Java test could catch any of this: `ModelFlagTest` and `ModelParametersExtendedTest` assert the string mapping (`hasKey("--mlock")`), never that llama.cpp still accepts the string, so they stayed green while the flags were dead. The guard closes that: - `cmake/extract-java-cli-flags.cmake` extracts every `"--flag"` literal `ModelFlag.java`/`ModelParameters.java` can emit into a generated header at configure time, so the Java sources stay the single source of truth. - `src/test/cpp/test_model_flags.cpp` asserts each one is registered in the real `common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options`, exempting only `--vocab-only` (a project pseudo-flag `strip_flag_from_argv` removes before the parse). It also checks the oracle and the exemption list themselves, so an empty option table or a stale exemption cannot make it pass vacuously. A grep over `arg.cpp` would not have worked -- it is structurally blind to example scoping, which is exactly where `--grp-attn-n`/`-w` hid. Run against the pre-fix sources the test named all seven; after the fix it is green. Verified locally: ctest 531/531 (was 527), `mvn test` 1759 tests green, PIT 320/320 killed with 0 NO_COVERAGE, spotbugs clean (`setLoadMode` added to the design-intent OCP suppression list), spotless, clang-format and `javadoc:jar` all clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AnNYn8W1xuVxVJtyL34GyH
1 parent 3eff05d commit 027d887

13 files changed

Lines changed: 634 additions & 68 deletions

File tree

‎CLAUDE.md‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1460,8 +1460,9 @@ ctest --test-dir build --output-on-failure -R "ResultsToJson"
14601460
| `src/test/cpp/test_tts_wav.cpp` | 2 | The in-memory WAV writer `pcm_to_wav16_bytes` in `tts_wav.hpp` (WAV header/payload + little-endian clamping) — our own code, not upstream. The Qwen3-TTS pipeline it pairs with (`mtmd_helper::gen_audio`) is entirely upstream-owned (no project-side DSP to unit-test here). The load path is additionally covered by `test_tts_params.cpp` (3 tests over `tts_params.hpp`'s `build_tts_params`, plus 2 pinning the upstream `-1` default it depends on), which pins the CPU-thread resolution whose absence used to crash the JVM on every platform — see the `TODO.md` entry for the mechanism. End-to-end coverage is `TtsIntegrationTest`, which is model-gated. |
14611461
| `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. |
14621462
| `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. |
1463+
| `src/test/cpp/test_model_flags.cpp` | 4 | **The contract between the Java flag surface and llama.cpp's server argument parser.** CMake extracts every `"--flag"` literal `ModelFlag.java` + `ModelParameters.java` can emit (`cmake/extract-java-cli-flags.cmake` → a generated header), and this file asserts each one is in `common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options`. It exists because **no Java test can catch this class**: `ModelFlagTest`/`ModelParametersExtendedTest` pin the *string mapping* (`hasKey("--mlock")`), never that llama.cpp still accepts the string, so they stay green forever while the flag is dead — and `common_params_parse` treats an unregistered option as a hard error, so the affected builder method makes the model **unloadable**, not merely ineffective. **A grep over `arg.cpp` is not a substitute**: `--grp-attn-n`/`-w` are present there at every pinned tag but `set_examples()`-scoped to `LLAMA_EXAMPLE_COMPLETION`/`PASSKEY`, so the server parser rejects them exactly like a deleted flag — only the real option table sees that. `--vocab-only` is the one exemption (a project pseudo-flag `strip_flag_from_argv` removes before the parse); the exemption list is itself asserted to stay live. |
14631464

1464-
**Current total: 527 tests (all passing).**
1465+
**Current total: 531 tests (all passing).**
14651466

14661467
#### Upstream source location (in CMake build tree)
14671468

@@ -1722,7 +1723,7 @@ This has actually shipped twice. Most recently the `--flash-attn` / `--lazy-mode
17221723
`llama/spotbugs-exclude.xml` lists methods **by name**, so renaming `setTensorReadLazy` to
17231724
`setLazyMode` left a dead entry while the new `setLazyMode` and `setFlashAttn` were uncovered. **Any
17241725
rename or addition of an enum-valued `ModelParameters` setter needs that list updated in the same
1725-
commit** — the same "FQN not updated after a rename" class as the stale PIT `targetClasses` and
1726+
commit** — `setLoadMode` was added to it for exactly this reason — the same "FQN not updated after a rename" class as the stale PIT `targetClasses` and
17261727
`CMakeLists.txt` OSInfo repairs.
17271728

17281729
## Spotless Formatting

‎TODO.md‎

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -354,21 +354,22 @@ into that PR.
354354
Each item below was verified against pristine upstream tags and is real, but none is a regression
355355
introduced by the version bump — they were deferred to keep that PR landable.
356356

357-
- **`ModelParameters` emits five CLI flags the server arg parser rejects, so any caller of them
358-
cannot load a model.** `--dump-kv-cache`, `--hf-repo-v` and `--hf-file-v` no longer exist anywhere
359-
in llama.cpp (absent at both b10456 and b10649); `--grp-attn-n` and `--grp-attn-w` still exist but
360-
are `set_examples({LLAMA_EXAMPLE_COMPLETION, ...})`, so `add_opt` never registers them for
361-
`LLAMA_EXAMPLE_SERVER` — the example jllama parses with. An unregistered flag is not ignored:
362-
`arg.cpp` throws, `common_params_parse` returns false, and `load_model_impl` throws
363-
`LlamaException("Failed to parse model parameters")`. Four existing tests pin the dead literals and
364-
would pass forever. Fix: deprecate the five members the way this PR handled
365-
`withTfsZ`/`withPenalizeNl` (keep source compatibility, never write the map), and add a hermetic
366-
`jllama_test` contract test that walks `common_params_parser_init(params, LLAMA_EXAMPLE_SERVER)`'s
367-
`ctx.options` (upstream's own `test-arg-parser` pattern; the symbols already link into
368-
`jllama_test`) and asserts every flag `ModelParameters`/`ModelFlag` can emit is in that set,
369-
excluding only `--vocab-only`, which `strip_flag_from_argv` removes on purpose. A grep-based sweep
370-
is **not** sufficient — it is structurally blind to example scoping, which is exactly how
371-
`--grp-attn-w` hides.
357+
- ~~**`ModelParameters` emits five CLI flags the server arg parser rejects, so any caller of them
358+
cannot load a model.**~~ **DONE** — and it turned out to be **seven**, not five. The fix is the one
359+
this entry prescribed: `cmake/extract-java-cli-flags.cmake` extracts every `"--flag"` literal
360+
`ModelFlag.java`/`ModelParameters.java` can emit into a generated header, and
361+
`src/test/cpp/test_model_flags.cpp` asserts each is registered in
362+
`common_params_parser_init(params, LLAMA_EXAMPLE_SERVER).options`, exempting only `--vocab-only`
363+
(which `strip_flag_from_argv` removes on purpose). Run against the pre-fix Java sources it reported
364+
exactly the predicted set, which is how the count grew: the five named here plus `--mlock` and
365+
`--no-mmap`, deleted upstream at b10878 while this entry was open. Those two have a faithful
366+
replacement, so `enableMlock()`/`disableMmap()` were **repointed** to upstream's own deprecation-shim
367+
mapping (`--load-mode mlock` / `--load-mode none`) behind a new `setLoadMode(LoadMode)` rather than
368+
retired — no API loss. The other five became no-ops (`@Deprecated`, never write the map), and
369+
`ModelFlag.MLOCK`/`NO_MMAP`/`DUMP_KV_CACHE` were removed from the enum so a broken argv is not
370+
reachable through `setFlag` either — the same reasoning that already excluded `FLASH_ATTN`. The
371+
replaced Java assertions now compare against a pristine `ModelParameters`, not against the old
372+
"still has this key" shape that would have passed forever.
372373

373374
- **`acquire_jllama_context_impl` / `release_jllama_context_impl` / `jllama_context_guard` have no
374375
model-free unit guard.** These three (`jni_helpers.hpp`) are the whole `close()`-vs-inference

0 commit comments

Comments
 (0)