QVAC-22629 feat: run an advisory llama.cpp fit check before loadModel - #4010
Conversation
License compliance — cleanNo 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):
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Review StatusCurrent Status: ✅ APPROVED |
801f318 to
be4a88b
Compare
…adModel Squash of the branch history (supervisor port to protocol v2, request partitioning, advisory orchestration, load-model call site, env gate, tests, example, and the packages/inference relocation after #3595) into one commit for a clean rebase onto current main.
- Raise the `@qvac/model-fit` floor to ^0.8.0, the first release built against the qvac-fabric#214 memory-reporting fix. Verified on the M4 Pro regression fixtures: the honest budget (total - wired - compressor) now drives the verdicts, the idle-machine false negative stays conservative, and the true positives hold. - Classify `load_mode` in place of the removed `no_mmap` (renamed on main by #4078): without it, any load setting the mode would be refused as an unclassifiable key and produce no verdict. - Remove the QVAC_ADVISORY_MODEL_FIT gate: the check now runs on every completion/embedding llama.cpp load. The env schema entry, the enabled seam, and the inertness test go with it. - Reword the resident-reserve and CPU-refusal rationale for 0.8.0: the fit child now sees system-wide wired memory, so the reserve covers only the unwired remainder and deliberately errs conservative.
fff2b07 to
2d70d39
Compare
Review finding from a local verification run: 'flash-attn' is a completion schema key (since split-mode 'tensor' landed) but was in none of the request builder's partitions, so any completion load setting it refused the check and produced no verdict — the safe direction, but silently switching the check off for a memory-load-bearing setting, and with it every 'tensor' split load, which requires flash attention. model-fit's allowlist already accepts the key, so forward it as evidence.
There was a problem hiding this comment.
advisory llama.cpp fit check on every completion/embedding load, fail-open, no public API. partitioning + supervisor look careful; the flash-attn classify follow-up was the right catch.
title still says "opt-in" after 2d70d39 dropped QVAC_ADVISORY_MODEL_FIT — changelog will ship that wording. rest is nits on the example.
- Match the examples/ log style: '▸' for the reprinted advisory lines and a quickstart-style 'console.error(✖', ...)' catch. - Correct the stale unload rationale: since the resident reserve and the 0.8.0 system-wide wired budget, leaving the first model loaded WOULD shift the second verdict; the unload keeps the demo comparable to the fixture tables.
There was a problem hiding this comment.
Implementation is good; placement seems wrong — the SDK already has a public API for this question and this PR doesn't use it:
assessModelFitexists: clientpackages/sdk/src/client/api/assess-model-fit.ts, handlerpackages/inference/src/registry.ts:78, enginepackages/inference/src/resources/model-fit/(vs. this PR's
src/model-fit/).- It already returns
verdict/basis/budget/estimate/estimatorVersionwith a pluggable estimator registry (estimators/{llm,whisper}.ts) — where a native probe would slot in. - This PR's verdict has no return path at all:
examples/advisory-model-fit.ts:83reads it vialog.message.includes('[advisory-fit:'). - And it invents a second margin policy —
1024 MiB + Σ(resident bytes)(advisory-fit.ts:220) vsinteractive-v1's 20%/2 GiB — so the two can disagree on the same model with nothing saying which wins.
Other two blockers: NOTICE missing bare-runtime + transitives (qv-notice-generate); probably inert in bare-pack bundles (build one and confirm).
Rest is minor (opt-out flag, log level, margin constant, tail trim).
…ail, NOTICE - Restore QVAC_ADVISORY_MODEL_FIT as an operator opt-out, default on (0/false/off/no disable). Removing the flag fixed the accuracy story but left no switch for a load-heavy startup path or a runtime where the disposable child cannot spawn. - Always send marginMiB (base + reserve) instead of omitting it at zero reserve: the omitted case relied on the addon default matching ADVISORY_FIT_BASE_MARGIN_MIB — two sources of truth that would diverge silently. - Log unsupported-load refusals at debug: every non-llama and mobile load takes that path, and at info it tagged every whisper/tts/ocr load. Child failures stay at info. - Keep the stderr tail from opening mid-codepoint: after the byte trim, walk forward past UTF-8 continuation bytes. - Regenerate packages/inference/NOTICE: bare-runtime is a new third-party prod dependency and pulls bare-subprocess plus a platform binary package; the regen also restores missing model attributions.
…fit check Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…elInfo The verdict was only reachable by substring-matching an engine log line, so nothing could consume or assert it. `loadModel` now keeps the outcome, stores it on the registry entry, and `getLoadedModelInfo` returns it as `fitProbe`. Also collapses the two near-identical fit namespaces: the native probe moves from `src/model-fit/` to `src/resources/model-fit/native-probe/`, beside the `assessModelFit` estimators, and its outcome now carries `basis` and `estimatorVersion` like the pre-download API. The headroom policy it applies is named `native-probe-v1` with the precedence against `interactive-v1` written down. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
7c42a61
Only the new `LoadedModelInfo.fitProbe` block. A full `contract:export` on this machine also rewrites three unrelated `type: [...]` unions as `anyOf`, which is local toolchain drift, not this change: `contract:check` reports the same staleness at the parent commit with a clean tree, where CI passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Generated from the committed contract by scripts/generate.py. Additive only: the NativeProbeFit models and the LoadedModelInfo field. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Back out the public exposure of the probe verdict: getLoadedModelInfo no longer returns fitProbe, the contract schema entry is removed, and the example reads the log stream again. The internal scoping stays — the probe lives under resources/model-fit/native-probe/ beside the assessModelFit estimators, its outcome carries basis/estimatorVersion, and loadModel still stores it on the registry entry for internal use. Wiring it into a public API is a separate decision once the shape is confirmed.
9196c5d removed the fitProbe schema entry but left the generated NativeProbeFit models behind, so the sdk-python contract check failed. Regenerated from the committed contract; deletions only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
🎯 What problem does this PR solve?
loadModelhas no idea whether the llama.cpp load it is about to run fits in memory. A model that doesn't fit either fails late or loads fine and then fails at the first decode.📝 How does it solve it?
@qvac/model-fitin one disposable Bare child and log the verdict (fit/does-not-fit/ no evidence).QVAC_ADVISORY_MODEL_FIT=0opts out. Cost: one child (~0.5 s) per llama.cpp load.assessModelFitestimators (resources/model-fit/native-probe/,basis/estimatorVersion); exposing it publicly is a follow-up decision.packages/sdk/examples/advisory-model-fit.ts.🧪 How was it tested?
test/native-probe-*: supervisor against real disposable children (crash, hang, kill), request partitioning, fail-open on every failure mode. typecheck/lint clean.does-not-fitload proceeded to a working completion. Verdicts are machine-dependent by design, so no e2e pins a specific one.bare-pack --linkedinvocationqvac bundleuses, all hosts — bundles clean, no--deferneeded.