Skip to content

QVAC-24073 feat[bc]: adopt fabric b10297 consumers and replace no_mmap with load_mode - #4078

Merged
donriddo merged 22 commits into
mainfrom
feat/QVAC-24073-sdk-load-mode
Aug 28, 2026
Merged

QVAC-24073 feat[bc]: adopt fabric b10297 consumers and replace no_mmap with load_mode#4078
donriddo merged 22 commits into
mainfrom
feat/QVAC-24073-sdk-load-mode

Conversation

@donriddo

@donriddo donriddo commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

  • @qvac/llm-llamacpp@0.47.0 removed the no_mmap load-config key and added load_mode in its place, but the SDK contract still declares no_mmap. It is no longer consumed by the addon, so a config still carrying it falls through to llama.cpp's argument parser and fails the load with invalid argument: --no-mmap.
  • The SDK is pinned to the fabric 10069 train while all six b10297 consumer releases are published, so it never picks up the new contract at all.

📝 How does it solve it?

  • Pin the six b10297 consumer releases in packages/inference/package.json (peer + dev) and packages/sdk/package.json:

    Package From To
    @qvac/classification-ggml ^0.20.0 ^0.22.0
    @qvac/embed-llamacpp ^0.34.0 ^0.36.0
    @qvac/llm-llamacpp ^0.45.0 ^0.47.0
    @qvac/ocr-ggml ^0.18.0 ^0.20.0
    @qvac/translation-nmtcpp ^0.10.0 ^0.12.0
    @qvac/vla-ggml ^0.21.1 ^0.23.0

    The set moves together because these share the libqvac-ggml-* backend build — a 10297 addon beside a 10069 sibling collides, and a mixed addon set crashes on iOS. A caret on a 0.x locks the minor, so every range had to move explicitly.

  • Replace no_mmap with load_mode in the llamacpp completion config, as a Zod enum of the addon's five accepted values (none, mmap, mlock, mmap+mlock, dio), checked against kLoadModes in the addon's LoadFitNormalization.cpp. Left unset, the addon applies its own default of mmap, so the field carries no SDK-side default and does not join LLM_CONFIG_DEFAULTS.

  • Regenerate contract/schema.json and the Python client from the schema.

  • transformLlmConfig needs no change: its camelCase-to-snake rewrite only matches all-letter keys, so load_mode reaches the addon verbatim. That is now covered by a test rather than assumed.

no_mmapload_mode is the only breaking API change across the whole 0.45 → 0.47 span; every other bump requires no additional SDK contract work. embed-llamacpp@0.35.0 adds IdMapIndex and IdMapIndexFilter, which are additive and unconsumed by the SDK.

Also renames n_ctx to ctx_size at eight call sites in the conformance fixtures, the Python transport and notebook tests, and the shipped notebook example. n_ctx has never been a declared LLM config key, so those loads were silently running at the default 1024 rather than the window their comments claim.

How a retired key behaves

No new validation is introduced. This matches how toolsMode was retired:

  • JS/TS callers get a hard failure. loadBuiltinToRequestSchema already validates modelConfig strictly, so passing no_mmap raises RequestValidationFailedError (code 50010) before a request is built. It is also a compile-time type error.
  • Python and raw-wire callers have the unknown key stripped, and the model loads with the addon's default mmap. That is the existing behaviour for a removed key, unchanged by this PR.

🧪 How was it tested?

  • inference: build, lint and 1482/1482 unit tests green. New cases cover every valid load_mode, an invalid one, the field staying unset when omitted, and load_mode surviving transformLlmConfig as an underscore key.
  • sdk: build green, unit tests green, contract:check and the sdk-python generate.py --check both clean.
  • sdk-python: 7/7 in a new tests/test_llm_load_mode.py, plus black and ruff clean.
  • Two model-load e2e cases — load_mode: 'none', and legacy no_mmap rejected on error code 50010 with an explicit failure if a native --no-mmap error appears instead. They register through ModelLoadingExecutor, so they also run under the electron and mobile consumers, and are not tagged smoke since model loading already carries smoke coverage.
  • An earlier run on this branch exercised both cases on real hardware: Windows GPU (load_mode: 'none' 4214 ms, legacy rejected 3 ms) and Android Pixel 9 Pro (6463 ms / 4 ms). Failures in that run were unrelated (logging-timestamp-accuracy, kv-cache-remove-thinking-compaction, cancel-broad-embeddings). The current head has not been exercised yet.

Not covered locally:

  • mlock, mmap+mlock and dio are exposed but untested on device. The addon validates locally (load-mode must be one of ...), so a bad value fails cleanly. dio is accepted by the addon but does not take effect in the pinned fabric v10297.0.0 — the loader sets use_direct_io, while gguf_file_load constructs llama_file_disk(path, "rb") without passing it. A native defect, tracked separately; the SDK exposes the value because the addon's API does.
  • Electron packaging and the docs build are blocked in this environment by an outbound firewall.
  • Addon integration tests need Bare ≥ 1.28 for bare-fs; the local runtime is 1.26.

💥 Breaking Changes

no_mmap is no longer accepted in the llamacpp model config, and it is gone from the exported contract and the generated Python client. Use load_mode instead.

BEFORE:

await loadModel({
  modelSrc: MODEL,
  modelType: 'llm',
  modelConfig: { ctx_size: 2048, no_mmap: true }
})

AFTER:

await loadModel({
  modelSrc: MODEL,
  modelType: 'llm',
  modelConfig: { ctx_size: 2048, load_mode: 'none' }
})

Full mapping — do not mechanically rename the key and keep a boolean value:

Before After
no_mmap: true load_mode: 'none'
no_mmap: false omit load_mode, or load_mode: 'mmap'
omitted omitted (addon default mmap)
any other value validation error

The same mapping applies to deviceDefaults.llm and deviceDefaults['llamacpp-completion'] in a config file.

load_mode also reaches modes no_mmap never could: 'mlock', 'mmap+mlock' and 'dio'.

Notes for reviewers

…p with load_mode

The llm-llamacpp addon dropped no_mmap in 0.47.0 and added load_mode in its
place. An unrecognised key is not ignored: it falls through to llama.cpp's
argument parser, so a config still carrying no_mmap fails the model load with
"invalid argument: --no-mmap".

Pin the six fabric b10297 consumer releases in packages/inference and
packages/sdk. The set moves together because they share the libqvac-ggml-*
backend build; a 10297 addon beside a 10069 sibling collides, and a mixed
addon set crashes on iOS. A caret on a 0.x version locks the minor, so each
range had to move explicitly.

Replace no_mmap with load_mode in the llamacpp completion config, typed as the
addon's own enum: none, mmap, mlock, mmap+mlock, dio. Left unset, the addon
applies its default of mmap, so the field carries no SDK-side default and does
not join LLM_CONFIG_DEFAULTS. transformLlmConfig needs no change: its
camelCase-to-snake rewrite only matches all-letter keys, so load_mode reaches
the addon verbatim, which is now covered by a test rather than assumed.

Regenerate contract/schema.json and the Python client from the schema.

Add three model-load e2e cases: load_mode 'none', explicit load_mode 'mmap',
and a legacy no_mmap rejected by strict validation. None are tagged smoke;
model loading already carries smoke coverage.

Update the llm-llamacpp README and the website addon page, whose parameter
tables still documented no_mmap.
@donriddo
donriddo requested review from a team as code owners August 26, 2026 05:21
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Review Status

Current Status: ✅ APPROVED
Approvals so far: Team Lead: 2, Member: 2

@github-actions

Copy link
Copy Markdown
Contributor

License compliance — clean

No new dependency license findings in this PR.

Warn-only (shadow) mode — this check does not block merges yet.

Updated automatically by the canonical license compliance workflow.

NOTICE presence (advisory)

Missing NOTICE (advisory, does not block):

  • ./.github/actions/release-merge-guard
  • ./docs/website
  • ./packages/ggml-coload-smoke
  • ./packages/fabric/test/integration
  • ./packages/inference-addon-cpp/mobile
  • ./packages/sdk/e2e
  • ./packages/llm-llamacpp/benchmarks/performance
  • ./packages/llm-llamacpp/benchmarks/server
  • ./packages/vla-ggml/sim/server
  • ./packages/embed-llamacpp/benchmarks/performance
  • ./packages/embed-llamacpp/benchmarks/server
  • ./packages/asr-ggml/benchmarks/server

@socket-security

socket-security Bot commented Aug 26, 2026

Copy link
Copy Markdown

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff Package Supply Chain
Security
Vulnerability Quality Maintenance License
Addednpm/​@​qvac/​translation-nmtcpp@​0.12.0911001009870
Addednpm/​@​qvac/​classification-ggml@​0.22.0881001009780
Addednpm/​@​qvac/​ocr-ggml@​0.20.0891001009780

View full report

…e models through ResourceManager

The streamx entry was never intended. It arrived from a local `bun update
streamx` run while confirming that streamx 2.28.1 carries the Writable typing
fix, and it contradicts how that fix actually reaches the SDK: streamx is
transitive through tar-stream and bare-stream, whose ranges already admit
2.28.1, so a plain install resolves it with no direct dependency. The same
command also reordered two addon entries; both are reverted.

The load-mode e2e cases unloaded their model directly, which skips
ResourceManager.evict() and with it the mobile unloadSettleMs pause. That pause
exists because iOS does not release a worklet's pages promptly, and the next
load can abort inside the GGML allocator on the residue. The two cases run
back to back, and 'none' loads a private copy of the whole model rather than
mapping it, so they are the pair most likely to hit it. Both now register their
model under their own dep and evict in a finally.

The legacy-no_mmap case leaked its model on the branch where validation
unexpectedly admits the retired key. It now evicts before reporting failure.
…wire

The wire schema's nested modelConfig was not strict, so the exported contract
omitted additionalProperties for it and every generated client dropped an
unrecognised key instead of refusing it. A caller passing the now-retired
no_mmap got a default mmap load and no error, which is the opposite of what
removing the field is meant to communicate.

The JS client is unaffected: loadBuiltinToRequestSchema already validates
modelConfig strictly, so an unknown key never reached the wire from there.
What changes is the generated-client and raw-wire path, where the key now
fails validation rather than being discarded.

Rejection for a Python caller currently lands server-side, not in root client
validation. The generated LoadModelRequest is a union, and the custom-plugin
arm is guarded by a zod .refine() that excludes built-in model types; a
refinement is runtime-only and does not survive the JSON Schema export, so the
arm still admits llamacpp-completion and swallows the request. The server runs
the same schemas with refinements intact and rejects it there. Closing that
gap means changing the custom-plugin arm, which is tracked separately.

Cover the config type directly: every accepted load mode, a rejected one, the
retired no_mmap raising extra_forbidden, and the field staying optional.

The other model types keep non-strict nested configs; aligning them is a
separate change because it turns previously ignored input into errors across
the whole fleet.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

The sdk-python format check runs black over tests/, and the new file was not
formatted to it.
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Comment thread packages/inference/src/schemas/llamacpp-config.ts Outdated
Comment thread packages/inference/src/schemas/llamacpp-config.ts Outdated
Comment thread packages/inference/src/dispatch.ts
…ance callers

Eight call sites asked for a context window with `n_ctx`, which the LLM config
has never declared. The key was silently stripped, so those loads ran at the
default 1024 rather than the window they requested. Strict validation turns
that into a load failure, so the sdk-python real-worker leg goes red and the
shipped notebook quickstart raises for anyone who copies it.

The declared key is `ctx_size`. Renaming is the correct change on its own
terms: `test_bare_rpc_transport.py` explains that Qwen3's reasoning trace
overflows the metadata default and an explicit window is needed, and that
window was never actually applied.

Sites: conformance `cases.json`, `test_bare_rpc_transport.py` (four),
`test_notebook.py`, `examples/notebook.py`, `examples/notebook.ipynb`.

Only the python conformance runner reached this: the JS runner skips
`completionOrchestrate` before loadModel. Its skip comment blamed snake_case
versus camelCase, but `n_ctx` is not a casing variant of anything — that half
of the rationale is dropped, the worker-driven reason stands.
The two config-reload `wrongModelType` cases passed a whisper modelId with an
LLM modelType and `n_ctx: 2048`, expecting the type mismatch to be rejected.
With `n_ctx` no longer a declared key, the unknown key alone is enough to
reject, so the case could pass without ever exercising the mismatch it exists
for. They now pass `ctx_size`, leaving the mismatch as the only reason to fail.

`errors.py` told callers to raise `n_ctx` on a context overflow, naming a key
the config has never accepted; the field is `ctx_size`. Same for the stale
mention in the transport test's comment.
…hema

Making the base object strict was more than this change needed, and it undid a
deliberate design. The base was permissive and `.strict()` was applied at the
call sites that wanted it, which is why the device-defaults surface accepts
`.partial()` schemas for whisper, parakeet, ocr, diffusion and vla, and a bare
`z.record(z.string(), z.unknown())` for nmtcpp and tts-ggml. Nothing on that
surface was ever strict. A strict base made one retired LLM key abort
initialisation for every model type, which was a consequence of removing that
layering rather than a defect being fixed.

Some server-side strictness is still required. On a plain object Zod strips the
retired key, so the addon receives no loading mode and defaults to mmap,
silently reversing `no_mmap: true`. Strict therefore stays in two places: the
resolver schema, which is what dispatch parses against, and the llm wire
schema, so the exported contract and the generated Python client keep
`additionalProperties: false`.

The base returns to `z.object`, so config files and the public
`llamacppCompletionConfigSchema` export behave as before. The device-default
and base-schema tests written for the strict base go with it; the resolver,
dispatch, transform, contract, Python and e2e coverage is unchanged.
Removing a config key has been done twice before, and neither time added
strictness anywhere: toolsMode in #3380 and n_discarded in #3999 each deleted
the field and stopped. The client options schema has been strict since long
before this change, so a JS or TS caller passing a retired key already fails
validation with code 50010; other callers get it stripped, which is what
happened to toolsMode and what n_discarded will do. Holding load_mode to a
different standard was not justified.

Both .strict() calls go, and the exported contract and generated Python client
return to their previous shape. With them go the tests that only existed to
prove them: the resolver rejection case and the Python extra_forbidden case.

The dispatch changes go too. Coercing a malformed modelConfig into a defaults
object, and a raw ZodError escaping the error normalisation, are both real, but
both predate this change and neither has anything to do with load_mode - every
existing enum field behaves the same way. They belong in their own change if
anyone wants them.

The compile-time addon union guard goes as well. No other config field has one.
A bun install reordered two unrelated entries; the diff should carry the six
pin bumps only.
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

QVAC E2E — windows⚠️ no results

Config: suite=(none) · filter=(none) · exclude=(none)
Inference: branch:f81b2c687a43f412fbb9d6f3bd07b44b0bdb11c0:@qvac/inference@0.18.2
View run

The test job did not produce a results artifact (e.g. no device started within the start-timeout, or a job-level failure). Check the run and the device pool status above.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

QVAC E2E — android⚠️ no results

Config: suite=(none) · filter=(none) · exclude=(none)
Inference: branch:f81b2c687a43f412fbb9d6f3bd07b44b0bdb11c0:@qvac/inference@0.18.2
View run

The test job did not produce a results artifact (e.g. no device started within the start-timeout, or a job-level failure). Check the run and the device pool status above.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

QVAC E2E — ios⚠️ no results

Config: suite=(none) · filter=(none) · exclude=(none)
Inference: branch:f81b2c687a43f412fbb9d6f3bd07b44b0bdb11c0:@qvac/inference@0.18.2
View run

The test job did not produce a results artifact (e.g. no device started within the start-timeout, or a job-level failure). Check the run and the device pool status above.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

QVAC E2E — linux⚠️ no results

Config: suite=(none) · filter=(none) · exclude=(none)
Inference: branch:f81b2c687a43f412fbb9d6f3bd07b44b0bdb11c0:@qvac/inference@0.18.2
View run

The test job did not produce a results artifact (e.g. no device started within the start-timeout, or a job-level failure). Check the run and the device pool status above.

@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

QVAC E2E — macos⚠️ no results

Config: suite=(none) · filter=(none) · exclude=(none)
Inference: branch:f81b2c687a43f412fbb9d6f3bd07b44b0bdb11c0:@qvac/inference@0.18.2
View run

The test job did not produce a results artifact (e.g. no device started within the start-timeout, or a job-level failure). Check the run and the device pool status above.

The comment edit was not needed for this change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-e2e-full Triggers full e2e test suite [Currently SDK-only]

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants