QVAC-23075 feat: add VisionPsy Nano to the VLM benchmark - #3855
QVAC-23075 feat: add VisionPsy Nano to the VLM benchmark#3855yingying0906 wants to merge 14 commits into
Conversation
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
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):
|
Workflow security (shadow mode)zizmor found 1093 finding(s) in Findings are annotated inline on the changed files and listed in the job summary. Reproduce locally: pipx run zizmor==1.27.0 --offline .github/ |
Parses the idefics3-style preprocessing override out of the load config and forwards it to the vision context, so a caller can say "on" or "off" instead of being stuck with whatever the GGUF declares. Unset leaves the model's own value alone. This is what separates the VisionPsy Flash checkpoint from the base one, whose mmprojs are otherwise indistinguishable, so a Flash checkpoint loaded without it silently runs base preprocessing. It changes the image token count, so it moves both accuracy and encode time. LoadConfigHandlers parses the string into common_params, and MtmdLlmContext::initVisionContext copies it into mtmd_context_params next to image_tile_mode. Unit coverage for the parse sits with the other load-config cases. Needs the fabric side, tetherto/qvac-fabric-llm.cpp#205, which adds image_no_upscale to common_params and mtmd_context_params. cpp-lint stays red here until that merges and the registry publishes the next fabric version. Split out at Gianfranco's request. The SDK schema is #3854 and the VLM benchmark is #3855.
Parses the idefics3-style preprocessing override out of the load config and forwards it to the vision context, so a caller can say "on" or "off" instead of being stuck with whatever the GGUF declares. Unset leaves the model's own value alone. This is what separates the VisionPsy Flash checkpoint from the base one, whose mmprojs are otherwise indistinguishable, so a Flash checkpoint loaded without it silently runs base preprocessing. It changes the image token count, so it moves both accuracy and encode time. LoadConfigHandlers parses the string into common_params, and MtmdLlmContext::initVisionContext copies it into mtmd_context_params next to image_tile_mode. Unit coverage for the parse sits with the other load-config cases. Needs the fabric side, tetherto/qvac-fabric-llm.cpp#205, which adds image_no_upscale to common_params and mtmd_context_params. cpp-lint stays red here until that merges and the registry publishes the next fabric version. Split by area. The SDK schema is #3854 and the VLM benchmark is #3855.
Adds the idefics3-style preprocessing switch to the llamacpp completion config so a caller can override what the GGUF says. "on" rounds the image's long side up to a whole number of slices and caps it, so an image below the cap keeps its own resolution and becomes far fewer slices; "off" always stretches the long side to the cap. Unset keeps the model's own value. This is what separates the VisionPsy Flash checkpoint from the base one, whose mmprojs are otherwise indistinguishable, so a Flash checkpoint loaded without it silently runs base preprocessing. It changes the image token count, so it moves both accuracy and encode time. Additive and optional, so [api] rather than [bc]. The generated Python client is regenerated in the same commit because pr-checks-sdk-python.yml runs generate.py --check and fails the merge guard on a stale client. Split by area. The addon side stays in #3725 and the VLM benchmark is #3855.
5781c9c to
e66f4c6
Compare
Adds the base and Flash VisionPsy checkpoints to the model catalog, with their manifest entries, and the plumbing the comparison needed to be fair: - `resolve-cli-model.cjs` resolves a spec to the blob the CLI legs load, so an addon leg and a CLI leg run the same bytes at the same ctx_size. - `cli-args.cjs` carries a catalog entry's `cliArgs` to the CLI legs, since model-specific flags such as VisionPsy Flash's `--image-no-upscale` are fabric-fork additions that upstream-cli aborts on. - `stdout-parser.js` and `aggregate.js` read vision-encode timing and score the new rows. `package.json` registers the new `__tests__` in `test:prestage` so they run in CI. The manifest gains the VisionPsy blobs the catalog points at; without them the catalog resolves to keys that do not exist.
#3195 made models.manifest.json the only source of model URLs, keyed by modelName, but the vlm-benchmark catalog kept its own reg-* names, and those are not manifest keys, so resolveModelEntry throws before an addon leg reaches the disk. CLI legs never hit it because the workflow curls those blobs itself, which is why it stayed green. Three of the five blobs are byte-identical to manifest entries that already exist under other keys, so repointing modelName migrates them with no new pins. The two mmprojs need real entries, both warm: false so they stay out of every cache-models warm step. Only the qwen and gemma entries are affected. The VisionPsy entries were pinned correctly when they were added.
resolve-cli-model.cjs joins cliArgs with a space into the env file and cli-fixture-runner.cjs splits it back on whitespace, so an element carrying a space passed the allowlist as one token and then became two. `["--image-no-upscale=on --ctx-size=1"]` was accepted, and since extra args are appended after the fixed ones in cli-case-runner.js it overrode the benchmark's own --ctx-size. Reject whitespace inside an element, which is the same rule the join and split already assume. Verified with a rejection harness over the json: path: the space and tab smuggling forms are accepted before this change and rejected after, while the split, equals and underscore forms of the legitimate flag stay accepted. The committed catalog entries already pass flags as separate elements, so nothing in tree changes.
displayUrl() reported the caller's downloadUrl into the VLMMETA provenance marker, but
ensureBlob() on that path calls ensureModel({ modelName }), which fetches and sha256-verifies
the models.manifest.json entry and never reads downloadUrl. So a json: spec could pair
modelName visionpsy-nano-460m-q8_0.gguf with any URL and the report would name bytes that were
never fetched. Report the manifest entry's URL instead, falling back to the supplied one only
when there is no entry, which ensureBlob already rejects with its own message.
The CLI leg printed the whole cli-model.env, and a json: spec can point a blob at a presigned S3 link whose signature lives in the query string, so the run log published it. Print every line except the URLs. Names, origins and sources still print, so the log still says what ran and where it came from.
…arsers Three suites under benchmarks/vlm-benchmark/__tests__, wired into test:prestage so they run in the existing unit job. cli-args covers the property the flag allowlist depends on: one array element is one CLI argument. It checks the accepted spellings survive the env round trip, split, equals, underscore and a negative-number value, and that whitespace-bearing elements are rejected, plus the forbidden-flag cases. The join and split halves moved into cli-args.cjs so resolve-cli-model.cjs and cli-fixture-runner.cjs cannot drift apart on the format. stdout-parser uses log lines copied verbatim from llama-mtmd-cli, VisionPsy Flash q8_0 on Metal, and from the addon marker sample: batch timing summed, chunk counts read from n_chunks rather than line count, helper lines summed per slice, and a stream carrying both kinds not double counted. The real timing block pins prompt eval against decode eval, which the two regexes are easy to swap. aggregate runs the committed markers-v2.sample.txt end to end and locks its quality, speed and delta numbers, checks the warmup block stays out of the averages, and checks that passing the same log twice does not move the result.
parseInt(...) || 4096 read a parsed 0 as missing, but 0 is a valid context size meaning "let the engine pick the model default". A json: spec carrying ctx_size '0' survives normalizeSpec, since models.cjs:208 tests !spec.ctx_size and the string is truthy, resolve-cli-model.cjs:64 writes it to the env file, and the workflow passes it through as --ctx-size 0. The addon leg then ran at 0, because LlamaModel.cpp:2329 guards on n_ctx != 0, while the CLI leg ran at 4096, so the two engines were compared under different context sizes. That is the mismatch the comment above this line says the flag prevents. Only a non-number falls back now. Found by maxim-smotrov.
modelName becomes a path segment under $MODEL_DIR and is handed to curl -o, and the comment above MODEL_NAME_RE says a caller-supplied value must not escape that directory. The character class has no slash, but it does allow a name made only of dots, and ".." is a path segment that walks up one level. curl -o on a directory path fails today rather than clobbering anything, so this closes the gap against the stated rule, not a working exploit. Adds model-name.test.js covering the traversal shapes that carry no slash alongside the ordinary names that must keep working, including a leading dot, which is a hidden file and not a traversal.
…e.co fetch_blob only attaches the token to a huggingface.co URL, but -L carried it past that check. curl sends a -H header on every hop of a redirect chain, its own docs for -H say so, and the credential scoping that --location-trusted gates covers -u only, not -H. HF resolve URLs always 302 to a separate CDN host, so the token was going to whatever host the redirect named, and a json: spec can point downloadUrl at any huggingface.co path. Resolve the hop with the token and no -L, then fetch the target without it. The CDN link is presigned so nothing is lost, and the no-redirect case downloads directly with no hop for the token to leak on.
cliArgs validation was accepting two forms neither engine can honour. The equals form passed the allowlist because canonicalCliFlag split on `=`, but common/arg.cpp looks the whole argv token up in arg_to_options and never splits, so `--image-no-upscale=on` aborts the CLI leg; the workflow logs that as a warning, so the only symptom was an engine leg with no rows. `--image-max-tiles` was on ALLOWED_CLI_FLAGS with no ALLOWED_ADDON_KEYS twin, so a spec setting it on both sides had the addon half rejected at parse time and the CLI half applied, comparing different preprocessing under one model label. Both are now rejected, and isFlagToken's negative-number exemption is anchored to a complete number so a token like `-1--ctx-size` is still checked. ensureBlob reported all four resolveModelEntry failures as a missing manifest key, so an entry added without `bytes` sent the reader to the wrong file. It now only rewrites the message when the entry is genuinely absent, and keeps the cause either way. A registry source has no download URL and the addon leg leaves it in benchmarks/model/, not MODEL_DIR, so the CLI step called it missing and exited with advice that could not work. It now looks in the addon's directory first, and says plainly that registry sources are addon-only. CLI-only downloads are checked against a sha256 pin, taken from the manifest or from a `sha256` field on a json: blob, and the two blobs are fetched concurrently rather than one after the other. cli-model.env moves to RUNNER_TEMP and is removed after sourcing, because a presigned URL is a bearer credential and a self-hosted runner's workspace outlives the job. The warmup assertion in aggregate.test.js could not fail: aggregate.js scores `pred` but never prints it, so no output can contain "warmup". It now asserts the measured row count, which does fail when the block-0 filter is removed. Also validates a json: spec's hf repo, sha and file before they reach the token-bearing URL, uses an own-property check on catalog lookup, aligns the unknown-source placeholder with harness.cjs, and corrects the rss_mb docs, which promised macOS and Windows coverage the /usr/bin/time wrapper cannot give. Three spots in the workflow also substituted a dispatch input into code rather than passing it as data, all predating this change. matrix_preset was interpolated into the source of six `node -e` scripts inside a string literal, so a value carrying a quote closes the literal and the rest runs as JavaScript on the runner; it now reads process.env.PRESET, which the step already sets. The Aggregate step substituted matrix_mode and matrix_preset straight into a run: script, where a double quote closes the shell string it lands in; both move to the step's env block, which is what the step above already does for matrix_models. The context step wrote `ref` to GITHUB_OUTPUT as a plain key=value line, so a newline in the ref input could append step outputs of the dispatcher's choosing; both outputs use the delimited form now.
| // with no addon twin cannot be set on both legs, so a spec using it would put the two | ||
| // legs on different preprocessing under one model label. --image-max-tiles is the case in | ||
| // point: arg.cpp takes it, the addon has no handler, so it stays off both lists. | ||
| const ALLOWED_CLI_FLAGS = new Set([ |
There was a problem hiding this comment.
Could we define the CLI/addon preprocessing options from one shared descriptor instead of maintaining ALLOWED_CLI_FLAGS and ALLOWED_ADDON_KEYS separately? The comments require these lists to stay synchronized, but that invariant is currently manual. A single option map could derive both allowlists and make missing addon twins impossible; each option could describe its CLI spelling and addon key, with the validators deriving the two sets.
There was a problem hiding this comment.
const MODEL_OPTIONS = Object.freeze({
IMAGE_NO_UPSCALE: {
cli: '--image-no-upscale',
addon: 'image-no-upscale'
},
IMAGE_TILE_MODE: {
cli: '--image-tile-mode',
addon: 'image-tile-mode'
}
})
There was a problem hiding this comment.
Done, close to your sketch. MODEL_OPTIONS is one entry per option with its CLI spelling and addon key, and both allowlists plus the twin lookup derive from it, so a missing addon twin is not expressible. A null side marks the one-sided ones, currently mmproj-use-gpu. --image-max-tiles falls off both lists as a result, since arg.cpp takes it and the addon has no handler.
DmitryMalishev
left a comment
There was a problem hiding this comment.
Requesting changes on two blocking items (one inline on the workflow, one below with no file to anchor to) and one major test-wiring gap (inline on package.json). A non-blocking nice-to-have list follows as a separate comment.
I checked the existing reviews first to avoid duplication: the nested-HF-path rejection in resolve-cli-model.cjs, the cliArgs/addonConfig twin enforcement, and the static __EOF_REF__ delimiter are already raised by @maxim-smotrov, and the shared option-descriptor idea by @aegioscy — I'm not repeating those.
Critical — required check is red. run-integration-tests / test-darwin-x64 failed with a C++ TextLlm abort ([TextLlm] context overflow at batch prefill step: prompt tokens 2527, max context tokens 256, exit 134, plus common_fit_params: failed to fit params to free device memory). It looks unrelated to this PR (no native code is touched) and resembles a darwin-x64 flake, but it needs a green rerun before merge — and if it reproduces on main, please surface it as its own issue rather than rerunning past it.
For confidence, what I could verify offline all checked out: all six new models.manifest.json sha256/byte pins match the HuggingFace paths-info API at the pinned revisions, every catalog modelName now resolves to a manifest key (which also fixes the currently broken default addon leg — the old reg-* names were never manifest keys), the MODEL_DIR/benchmarks/model path math is consistent with ensureModel()/ensureBlob(), stage.cjs auto-stages the new .cjs files so mobile is unaffected, and the token-scoping / https-only / path-traversal / prototype-pollution hardenings are correct.
| # which is a bearer credential, and the workspace on a self-hosted runner outlives | ||
| # this job. Removed as soon as the values are in the environment. | ||
| ENV_FILE="$RUNNER_TEMP/cli-model.env" | ||
| node resolve-cli-model.cjs > "$ENV_FILE" |
There was a problem hiding this comment.
Critical: this reworked CLI-model path has never been dispatch-validated in its final form.
The latest successful benchmark-vlm-model-comparison dispatches (Aug 11–13) ran on bench/QVAC-23075-visionpsy-vlm, which has diverged from this branch — and diffing that branch's workflow against this head shows the validated iteration predates: the sha256 verify_blob gate, the parallel background fetches + wait error handling, the HEAD-redirect-then-unauthenticated-GET HF token flow (it still used plain curl -fL with the token), adopt_addon_blob, the RUNNER_TEMP env-file handling, the delimited GITHUB_OUTPUT outputs, and the env-hardened node -e/Aggregate steps.
So every new shell path in this step is currently unexercised, and shell-level behavior here (curl redirect semantics, background-job failure propagation, sha256sum vs shasum across the three OS runners) is exactly what the unit tests cannot cover.
Before merge, please dispatch on this branch's head and link the runs:
- CLI-only several-sources (
matrix_sources=fabric@<ref>) — exercises the fetch/verify path end to end; - mixed addon+CLI several-sources — exercises
adopt_addon_bloband the no-addon-default change; - a default two-models run — regression proof for the
reg-*→ manifest-key rename; - ideally a mobile smoke for the
addonConfig/mmproj-gpupath (needs a fabric addon build).
There was a problem hiding this comment.
Covered in the comment below: three dispatches green with links. adopt_addon_blob is the one path they do not reach, so it is covered locally instead.
There was a problem hiding this comment.
Update on the red check, and a correction to what I said earlier: it failed again on ed5e7c0c6 but with a different cause, not the paddleocr timeout. Details in the comment below. Short version, it is the known poisoned-Metal pattern on darwin-x64 from QVAC-23752, and the test involved is removed by #3938 along with sliding context.
| "test:integration:generate": "npm run generate:benchmark-shards && brittle -r test/integration/all.js test/integration/*.test.js && npm run test:mobile:generate", | ||
| "test:unit:generate": "brittle -r test/unit/all.js test/unit/*.test.js", | ||
| "test:prestage": "node --test scripts/__tests__/*.test.js ../../.github/actions/cache-models/warm-models.test.mjs", | ||
| "test:prestage": "node --test scripts/__tests__/*.test.js benchmarks/vlm-benchmark/__tests__/*.test.js ../../.github/actions/cache-models/warm-models.test.mjs", |
There was a problem hiding this comment.
Major: these tests don't actually run in CI, contrary to the PR description.
The description says registering the new __tests__ in test:prestage makes them run in CI, but as far as I can tell nothing in CI invokes them for this package: test:prestage is only reachable via this package's test:unit, and no workflow under .github/ runs test:unit, test:prestage, or a node --test targeting packages/llm-llamacpp (checked on-pr-llm-llamacpp.yml, the sanity-checks action, the integration/cpp-tests workflows, and on-pr-shared-ci-infra.yml).
That means the 39 tests guarding the flag allowlist, the env round-trip, the filename validation and the parsers only run on developer machines, and a future regression in those guards won't be caught by CI. Please wire them into a PR-time job (e.g. the sanity-checks action, or an on-pr-shared-ci-infra-style step) — or correct the PR description if I've missed the invocation.
There was a problem hiding this comment.
Answered in the comment below, with the test:prestage line from this PR's sanity-checks (llm-llamacpp) job.
Nice-to-have (non-blocking — not mandatory to address in this PR)Deduplicated against the existing reviews: the
|
…enforce the twin The CLI and addon allowlists were two hand-maintained sets that a comment asked to be kept in step. They now come off a single MODEL_OPTIONS descriptor, one entry per option with its CLI spelling and addon key, so a missing twin is not expressible. `addon: null` and `cli: null` mark the deliberately one-sided ones, currently only mmproj-use-gpu. The pairing is enforced per spec rather than documented. cliArgs and addonConfig were validated independently, so a json: spec could set --image-no-upscale for the CLI legs with no matching addonConfig, and the addon leg then ran base preprocessing under the same model label. Setting one side without the other, or setting both to different values, is now a parse-time error. A test holds the committed catalog to the same rule, since normalizeSpec only ever sees json: specs. The GITHUB_OUTPUT block used a fixed __EOF_REF__ delimiter with an untrusted input, so a ref of `main\n__EOF_REF__\nrepository=attacker/repo` closed the block and appended an output the downstream checkout would honour. A git ref cannot contain whitespace, so it is rejected outright, which removes the injection route rather than encoding around it, and the delimiter is per-run random as a second line of defence. hfUrl required `file` to be one bare segment, but HF repos nest and the pair form accepts paths like tinyllamas/stories260K.gguf, so a valid spec threw before download. Nested paths are allowed and each segment is checked, rejecting empty, `.` and `..`. Also from review: the tokenized inline GET now passes --max-redirs 0 and reports the redirect, instead of saving a redirect body that fails confusingly at the sha check later; a blob failing its sha256 is deleted and fetched once more, so a truncated file from a cancelled run stops failing every rerun on a self-hosted runner; a blob with no pin fails the leg unless the new allow_unverified_models input is set, since an unverified GGUF's chat template reaches --chat-template; the catalog notes that these checkpoints need a fabric addon, so a two-models dispatch on the published one fails at load after the downloads; and the longest comments lose an internal run ID and some restatement.
…the default mmproj Twin enforcement stops a json: spec from setting a flag on one leg only, but it cannot fix the asymmetry that is there by design: cliArgs are fabric-fork flags, so upstream-cli never receives them and runs the weights under base preprocessing while the addon leg applies the model's own. The numbers gave no hint of that, under one model label. [VLMMETA] now carries `preproc`, the preprocessing that leg actually applied, in one canonical sorted key=value form on the addon spelling. The addon leg builds it from addonConfig and the CLI legs from the argv they were handed, so two legs configured through different mechanisms are directly comparable. The origins table gains a column for it, and where the legs of one model disagree the report says which ran what and that those rows are not like for like. An absent field, from a log predating this, does not read as base preprocessing and raises nothing. mmproj-Qwen3.5-0.8B-Q8_0.gguf was warm: false while being part of the default two-models pair, so every default benchmark leg re-downloaded 116MB. It is warmed now, which is +0.6% on an 18.7GB warm set. The gemma and VisionPsy mmprojs stay cold: they only run when a dispatch names them.
The longest comment blocks had grown to restate what the code and CONTRACT.md already say. Cut the restatement and keep the mechanism and the gotchas, the parts a reader cannot recover from the code: - models.cjs: why an allowlist rather than a blocklist, and why a one-sided option can never be paired. Dropped the spelling examples and the Mali rationale, which CONTRACT.md and the mmproj-gpu catalog entry already carry. - harness.cjs: the two modes in one line, keeping only the gotcha that a URL pair parses but reaches the CLI legs alone. - resolve-cli-model.cjs: kept the usage line, why the env file lives outside the workspace, and the empty-URL contract for registry sources. Dropped the list of emitted variables, which the code below it already is. - config.cjs: kept why base and Flash are separate entries, the manifest-key rule, and the fabric-addon requirement. The mmproj-gpu entry keeps its reason for existing and the log line to check. - the workflow's HF token block keeps the redirect mechanism, since that is what makes two curl calls with no -L look deliberate rather than accidental. Longest block goes from 16 lines to 9, and comments from 33% of added non-blank lines to 28%.
VLM Matrix BenchmarkRun #342 — full report VLM Matrix — several-sources / smoke (run #342)Mode: several sources (engine varies; model fixed) · Engine: addon Preset: one fixed model across inference engines · quality = lmms-eval (VQA / ANLS / relaxed / MC), equal-weight mean across tasks. 1 · HighlightsInference engines on the same model: fabric-cli. Quality — overall % per source
Speed — mmproj-encode ms per source (lower = faster)
2 · DetailsEngine versions — llama.cpp build per sourceVersions set manually — refs were pinned per source, so builds may differ.
Sources — resolved versions
Models & origins (Source = Registry / HF / S3 / URL · pinned commits)
Provenance — hardware & softwarelinux · cpu (runner
Quality (%)
Quality by task (% — higher better, mean across platforms; one column per source)
Speed
Peak memory (RSS)
3 · Test Results (per platform)
4 · Image samples
|
VLM Matrix BenchmarkRun #344 — full report VLM Matrix — two-models / smoke (run #344)Mode: two models (qwen3.5-f16 vs qwen3.5-q8; engine fixed) · Engine: addon Preset: comparing two models — base = qwen3.5-f16, candidate = qwen3.5-q8 · quality = lmms-eval (VQA / ANLS / relaxed / MC), equal-weight mean across tasks. 1 · Highlights
Two models — qwen3.5-f16 (base) vs qwen3.5-q8 (candidate), per platform · device. Quality — overall %: qwen3.5-f16 vs qwen3.5-q8
Speed: qwen3.5-f16 vs qwen3.5-q8 (lower = faster; metric is mmproj-encode on desktop, TTFT on mobile)
2 · DetailsSources — resolved versions
Models & origins (Source = Registry / HF / S3 / URL · pinned commits)
Provenance — hardware & softwarelinux · cpu (runner
Quality (%)
Quality by task (% — higher better, mean across platforms; one column per model)
Speed
Peak memory (RSS)
3 · Test Results (per platform)
4 · Image samples
|
VLM Matrix BenchmarkRun #343 — full report VLM Matrix — several-sources / smoke (run #343)Mode: several sources (engine varies; model fixed) · Engine: addon Preset: one fixed model across inference engines · quality = lmms-eval (VQA / ANLS / relaxed / MC), equal-weight mean across tasks. 1 · HighlightsInference engines on the same model: addon, fabric-cli. Quality — overall % per source
Speed — mmproj-encode ms per source (lower = faster)
2 · DetailsEngine versions — llama.cpp build per sourceVersions set manually — refs were pinned per source, so builds may differ.
Sources — resolved versions
Models & origins (Source = Registry / HF / S3 / URL · pinned commits)
Provenance — hardware & softwarelinux · cpu (runner
Quality (%)
Quality by task (% — higher better, mean across platforms; one column per source)
Speed
Peak memory (RSS)
3 · Test Results (per platform)
4 · Image samples
|
…nPsy The comment said the published prebuild has neither the VisionPsy projector type nor the image-no-upscale load-config key, so a two-models dispatch would fail at load. Both halves are wrong. The projector arrived in qvac-fabric 10069.1.0 and vcpkg.json pins >= 10069.1.1 (#3929), and the addon has accepted image-no-upscale since #3725, both merged. A two-models dispatch on the published addon works, so the note was steering readers away from a supported path.
|
Thanks, both blocking items were real. Dispatches, all green. Ran on
Run 1 exercised the new shell paths for real: both blobs fetched in parallel at 14:47:50, both verified against their sha256 pins, I pinned
Mobile is dispatchable, and a comment of mine said otherwise. The projector arrived in On the test wiring, they do run. On the red check, it is a 30 minute brittle timeout on a PaddleOCR CPU test, exit 134. The All eight nice-to-haves are in except one. On the shared descriptor: |
|
darwin-x64 failed again on |
| for (const [k, v] of Object.entries(spec.addonConfig || {})) { | ||
| addon.set(k.replace(/_/g, '-'), String(v)) | ||
| } |
There was a problem hiding this comment.
ALLOWED_ADDON_KEYS admits both the hyphen and underscore spelling of every key, so a spec carrying both passes the twin check while the addon resolves the collision the opposite way.
ALLOWED_ADDON_KEYS (:113) lists both spellings via addonSpellings, so {"image_no_upscale":"on","image-no-upscale":"off"} clears the allowlist. assertTwinsMatch then collapses the two into one Map entry, keeping whichever came last in JS insertion order. The addon resolves the same collision by handler order instead: applyLoadConfigHandlers iterates image-no-upscale (addon/src/handlers/LoadConfigHandlers.cpp:101) then image_no_upscale (:102), and each handler overwrites params — so the underscore spelling wins there.
Run against the real module:
addonConfig {"image_no_upscale":"on","image-no-upscale":"off"}
-> ACCEPTED by the twin rule
-> preproc reported for both legs: "image-no-upscale=off"
-> addon actually applies: image_no_upscale = on
Reversing the two keys is rejected, so the check is order-sensitive too.
Impact: preproc exists to surface leg divergence, and here it certifies that two legs match when they do not — a wrong comparison reported as a valid one. mmproj-use-gpu already hard-fails on dual spellings in LoadFitNormalization.cpp; the image-* keys do not.
Suggested fix: canonicalise before the allowlist check and reject any key present under more than one spelling, so the collision can never reach assertTwinsMatch — mirroring the dual-key error LoadFitNormalization.cpp already raises. One check in normalizeSpec:
const canon = new Map()
for (const k of Object.keys(cfg)) {
const c = k.replace(/_/g, '-')
if (canon.has(c)) throw new Error(`addonConfig sets both '${canon.get(c)}' and '${k}'`)
canon.set(c, k)
}| if (catalog && catalog[t]) return catalog[t] | ||
| // Own-property check, so a name like `constructor` or `toString` falls through to the | ||
| // unknown-model error below instead of resolving to an Object.prototype member. | ||
| if (catalog && Object.prototype.hasOwnProperty.call(catalog, t)) return catalog[t] |
There was a problem hiding this comment.
parseModels returns a catalog entry untouched, so of all the validation in normalizeSpec only the twin rule reaches committed specs, and then only via a unit test.
The comment above this line is honest that catalog entries bypass normalizeSpec, and __tests__/cli-args.test.js does hold all 11 entries to assertTwinsMatch. Still unreachable for a catalog entry: assertNoWhitespace (:140), assertNoEqualsForm (:151), MODEL_NAME_RE (:232), the cliArgs allowlist (:243) and the addonConfig key allowlist (:253). The https check is effectively re-applied downstream by resolve-cli-model.cjs, so that one is covered.
Impact: whitespace is the one that bites, and cli-args.cjs's own comment already names it — "an element with a space would pass the flag allowlist as one token and arrive as two". A catalog cliArgs element containing a space survives serializeCliArgs → CLI_EXTRA_ARGS → parseCliArgs and arrives as two argv tokens appended after the fixed ones (cli-case-runner.js:154-156). llama.cpp keeps the last duplicate (arg.cpp: "only last value will be used", then out_map[opt] = val), so ['--image-no-upscale on --ctx-size 512'] silently resets the context on the fabric leg while the addon leg stays at spec.ctx_size — a wrong comparison reported as a valid one.
Suggested fix: run the committed catalog through the real parser instead of just the twin rule, which closes every gap above at once rather than one assertion at a time. One line in the existing test:
parseModels('json:' + JSON.stringify([structuredClone(spec)]), null, null)All 11 committed entries pass that today, so it is free to adopt. Worth also iterating config.models / config.sourcesModel, which are the same objects today with nothing enforcing it.
🎯 What problem does this PR solve?
📝 How does it solve it?
config.cjsgains seven catalog entries, three quants per checkpoint plus one Flash entry that forces the projector onto the GPU. That last one exists to reach Mali Vulkan for the vision encoder, which the addon's auto-default sends to CPU, so no other entry can measure it.reg-prefix and become the manifest keysQwen3.5-0.8B-Q8_0.gguf,mmproj-Qwen3.5-0.8B-F16.ggufandmmproj-Qwen3.5-0.8B-Q8_0.gguf.models.manifest.jsoncarries noreg-qwenkey at all, so the names the CLI step used were never the ones the addon leg verifies against.resolve-cli-model.cjsresolves a spec to the blob the CLI legs load, so an addon leg and a CLI leg run the same bytes at the samectx_size.cli-fixture-runner.cjstakes the names, origins, label andctx_sizeas arguments instead of the hardcoded qwen values, which had mislabelled every several-sources run of anything else. It also restoresrss_mb, which was being dropped so the report's peak-RSS row was empty for every CLI leg, and emits per-row vision encode time and slice count.rss_mbcomes from the/usr/bin/time -vwrapper, so a CLI leg reports it on linux only.--image-no-upscale, travels ascliArgsfor the native CLIs andaddonConfigfor the addon. Both come off oneMODEL_OPTIONSdescriptor inmodels.cjs, so the two allowlists cannot drift apart, and an option with a twin must be set on both legs with the same value or the spec is rejected at parse time. Without that check a leg silently runs base preprocessing under the same model label as one that applied the flag.[VLMMETA]carriespreproc, the preprocessing each leg actually applied, in one canonical form for both mechanisms. The report shows it per leg and says so when the legs of one model disagree.upstream-cliis the honest case:cliArgsare fabric-fork flags, so it never receives them.benchmark-vlm-model-comparison.ymlstops defaulting a CLI-only dispatch to the published addon. That default forced an addon leg into every comparison, and for a model the published prebuild cannot load it failed before the CLI step ran. A CLI-only run now fetches the two blobs itself, both at once, and checks each against its sha256 pin, taken frommodels.manifest.jsonor from asha256field on ajson:blob. A blob with neither fails the leg, since an unverified GGUF's own chat template reaches--chat-template; the newallow_unverified_modelsinput overrides that. A blob failing the check is discarded and fetched once more, so a truncated file left by a cancelled run recovers instead of failing every rerun.HF_TOKENis attached only to ahttps://huggingface.co/URL, so ajson:spec pointing elsewhere cannot carry the secret with it. The token resolves the redirect hop without-Land the CDN target is fetched unauthenticated, because curl sends a-Hheader on every hop; the inline path uses--max-redirs 0so an unexpected redirect fails loudly rather than being saved and failing later at the sha check. The resolved URLs stay out of the log, and the env file holding them is written underRUNNER_TEMPand removed after use, because a presigned link holds its signature in the query string and a self-hosted runner's workspace outlives the job.benchmarks/model, not the CLI step's model dir, so the CLI step looks there before calling a blob missing, and says so plainly instead of pointing at a remedy that cannot work.harness.cjsrejects a blob the manifest does not pin with a message saying so, since the addon leg verifies againstmodels.manifest.jsonand never reads the supplied URL, and it reports the manifest URL as provenance so the marker names the bytes that actually ran. That message is used only when the name is really absent, so an entry missing a sha256 or byte-size pin still reports its own problem.stdout-parser.jsandaggregate.jsread vision-encode timing and score the new rows.CONTRACT.mddocuments the CLI-only dispatch, the new arguments, the twin rule and thepreprocfield.matrix_presetwent into the source of sixnode -escripts, the Aggregate step putmatrix_modeandmatrix_presetstraight into arun:script, and the context step wrotereftoGITHUB_OUTPUTas a plainkey=valueline. The first two go throughenvnow;refis rejected outright if it carries whitespace, which a git ref cannot, and the output delimiter is randomised per run.package.jsonregisters the new__tests__intest:prestage, whichon-pr-llm-llamacpp.ymlreaches through therun-lint-and-unit-testsaction, so they run on every PR. The manifest gains the VisionPsy blobs the catalog points at; without them the catalog resolves to keys that do not exist.mmproj-Qwen3.5-0.8B-Q8_0.ggufis warmed because it is part of the default pair, so every default run was re-downloading it.--image-no-upscale, but only as data passed through to the CLI, so it builds and unit-tests without the addon change. The addon leg can run these checkpoints already: the projector arrived inqvac-fabric10069.1.0 andvcpkg.jsonpins>= 10069.1.1, and the addon acceptsimage-no-upscalesince QVAC-23075 feat[api]: accept image_no_upscale in the addon load config #3725. So two-models works, not only several-sources against a fabric branch.🧪 How was it tested?
__tests__/covers thecliArgsround trip through the env file and back to an argv array, the flag allowlist, the twin rule including every committed catalog entry, themodelNamecheck, hf URL construction with nested and traversing paths, the log parsers and the scoring. 58 test cases, all passing locally and insanity-checks (llm-llamacpp).preproccases assert both the per-leg column and the mismatch callout, and that an older log without the field raises nothing.fabric@v10069.1.0withvisionpsy-flash-q4(run), mixed addon and CLI (run), and default two-models (run). The first fetched both blobs in parallel, verified both sha256 pins against live downloads, passed--image-no-upscale onto the fabric CLI and reportedimage-no-upscale=onin the new preprocessing column.sha256sumandshasumagreement, the discard-and-refetch-once flow, andadopt_addon_blob, which only a registry source hits. Everyrun:block in the workflow parses underbash -n.run-desktop.cjs --selfcheck,validate-mobile-manifest.js, prettier and lunte all pass, and every catalog blob resolves to a pinned manifest entry.