QVAC-23075 feat[api]: accept image_no_upscale in the addon load config - #3725
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):
|
Workflow security (shadow mode)zizmor found 1108 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/ |
|
Depends on tetherto/qvac-fabric-llm.cpp#205 and the registry publish that follows it. CI here stays red until then, by design. Build and device evidence is on bench/QVAC-23075-visionpsy-vlm, which carries the vcpkg overlay. It was PR #3726, now closed, and those run records are still readable there. |
7d4b2ce to
2cc8d8e
Compare
Review StatusCurrent Status: ✅ APPROVED |
Review follow-up: remaining benchmark hardening + QA gapsI see the QA gaps I would like covered before treating the benchmark plumbing as review-ready:
|
tobi-legan
left a comment
There was a problem hiding this comment.
Requesting changes based on the benchmark plumbing issues and QA gaps already posted.
Blocking item: the cliArgs validation can be bypassed through the join/split handoff, allowing benchmark-fixed CLI options to be overridden in the fabric CLI leg. Please preserve arg boundaries or revalidate after splitting.
Also please address the URL logging hardening and add focused coverage for models.cjs, resolve-cli-model.cjs, cli-fixture-runner.cjs, CLI-only several-sources, and the stdout/aggregate parser fixtures before this is merge-ready.
|
The cliArgs bypass is fixed in ddc0e63, one element can no longer carry whitespace. The provenance issue is fixed in 3c607e6. Fabric side has the review commits on tetherto/qvac-fabric-llm.cpp#205, ending at 295226e2d. Ready for another look when you are. The SHAs here are post rebase onto main. |
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
1 similar comment
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
VisionPsy answers count-fingers wrong. This does not fix that. It asks a wording
the model gets right instead of asserting on the wording it fails, which is a
workaround and worth calling one.
It is defensible here because the test is
"continuous batching MTMD: mixed image+text batch processes all slot types
correctly". It covers batch admission and slot scheduling. One text-only slot
returning a wrong answer says nothing about either, so gating this test on the
answer to a general-knowledge question was testing the wrong thing. It is not
evidence the model is fine, and the failure is real.
Why a separate vlmUser instead of editing user. CASES feeds two paths with
different system prompts: buildPrompt() to Llama-3.2-1B with the verbose 64-word
instruction, buildVlmBatchItem() to the VLM pair with the one-word instruction.
The two have no wording in common, so editing user just moves the failure.
Measured with llama-cli at the same greedy settings the tests use, holding the
frame at "How many fingers are on X? Answer with one word.":
one typical human hand metamorphs a human hand 5
one human hand metamorphs the human hand 5
a typical human hand metamorphs an adult human hand 5
one hand metamorphs a normal human hand 5
a single human hand 5
your human hand 5
"one" and "typical" break VisionPsy and are exactly what Llama-3.2-1B needs,
which answers "Ten" with them and "Fifty" without. SmolVLM2 answers 10 either
way. The frame matters as much as the modifier: "are there on a single human
hand" returns "No fingers" where "are on a single human hand" returns 5, so
re-measure against both models before editing either wording.
Reproduced on the research team's own llama.cpp build, upstream 08023072e plus
visionpsy-nano.diff, identical on all 18 cells of a 6-wording by 3-model sweep,
so this is the model and not the port. Reported separately to research.
Verified locally against a fresh addon build, both pairs green:
continuous-batching.test.js 13/13 tests and 167/167 asserts for the default
visionpsy pair and for QVAC_VLM_MODEL=smolvlm2.
Mobile integration tests — @qvac/model-fit (Android)Result: passed
|
Mobile integration tests — @qvac/translation-nmtcpp (Android)Result: passed
|
Mobile integration tests — @qvac/vla-ggml (Android)Result: passed
|
Mobile integration tests — @qvac/ocr-ggml (Android)Result: passed
|
Mobile integration tests — @qvac/model-fit (iOS)Result: passed
|
Mobile integration tests — @qvac/translation-nmtcpp (iOS)Result: passed
|
Mobile integration tests — @qvac/ocr-ggml (iOS)Result: passed
|
Mobile integration tests — @qvac/vla-ggml (iOS)Result: passed
|
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
Mobile integration tests — @qvac/embed-llamacpp (Android)Result: passed
|
Mobile integration tests — @qvac/embed-llamacpp (iOS)Result: passed
|
Mobile integration tests — @qvac/llm-llamacpp (iOS)Result: failed
|
….1.0 is published Reverts the two overlay-validation commits (7210e53 and its re-pin 40a1576). qvac-fabric v10069.1.0 is tagged and published to the registry, so the 7 fabric consumers resolve it from the registry again instead of the local overlay portfile. Note: until the bundled consumer bump lands, the version>= floors here still read 10069.0.0, so this branch builds against the previously published fabric.
Security baseline — findings detected
See the job summary and the repository Security tab for details. This comment is updated automatically by the canonical security workflow. |
Brings the branch up to date with origin/main (41 commits) ahead of the qvac-fabric 10069.1.0 dependency bump. Clean merge, no conflicts. Needed because the branch predated the @qvac/llm-llamacpp 0.43.0 release: the fabric bump has to be applied on top of the current versions, or the bumped numbers collide with main and the 0.43.0 changelog entry goes missing.
Bumps the qvac-fabric vcpkg dependency floor from 10069.0.0 to 10069.1.0 for all 7 fabric consumers, with the matching package version bumps and changelog entries. qvac-fabric 10069.1.0 adds VisionPsy Nano support and its Flash preprocessing rule (tetherto/qvac-fabric-llm.cpp#205) — the fabric side this PR's image_no_upscale load option depends on. - embed-llamacpp 0.32.0 -> 0.33.0 - fabric 0.4.0 -> 0.5.0 - llm-llamacpp 0.43.0 -> 0.44.0 - model-fit 0.1.0 -> 0.2.0 - ocr-ggml 0.16.0 -> 0.17.0 - translation-nmtcpp 0.8.0 -> 0.9.0 - vla-ggml 0.19.0 -> 0.20.0 llm-llamacpp's entry also documents image_no_upscale, since this bump is what creates the 0.44.0 release that publishes it. The other 6 are fabric-only with no API change. Registry publish: tetherto/qvac-registry-vcpkg#317. CI cannot resolve version>= 10069.1.0 until that merges.
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
🧪 C++ Test Coverage ReportCoverage: 📊 Detailed Coverage |
|
/review |
🎯 What problem does this PR solve?
📝 How does it solve it?
image_no_upscaleon the addon load config, forwarded to the vision context, so a caller can override the idefics3-style preprocessing rule the GGUF declares instead of being stuck with it.LoadConfigHandlers.cppparses0/off/falseor1/on/trueintocommon_params, and rejects anything else withInvalidArgument.MtmdLlmContext::initVisionContextcopies it intomtmd_context_params, next toimage_tile_mode.-1model default, so existing callers are unaffected.common_paramsandmtmd_context_params. That has now merged and been tagged asv10069.1.0— see the rollout section below for the current build state.🧪 How was it tested?
test_load_config_handlers.cppcovers the parse alongside the other load-config cases.🔌 API Changes
🚚 qvac-fabric 10069.1.0 rollout rides this PR
Phase B of the fabric rollout landed here rather than on a separate bump PR, so the consumer code and
the fabric version it needs meet on one PR.
v10069.1.0at50713f4dfc54b1fe09826a343527e83d90f5df62(head oftemp-10069)Three commits were added on top of the feature work:
qvac-fabricoverlay port and itsoverlay-portskeysare reverted; the 7 consumers resolve fabric from the registry again.
mainmerged in — the branch predated the@qvac/llm-llamacpp0.43.0 release, so the versionbumps had to be applied on top of current
main. Clean merge, no conflicts.version>=10069.0.0→10069.1.0, plus packageversions and changelog entries:
@qvac/embed-llamacpp@qvac/fabric@qvac/llm-llamacpp@qvac/model-fit@qvac/ocr-ggml@qvac/translation-nmtcpp@qvac/vla-ggmlNo
default-registry.baselinechanges anywhere.qvac-fabric10069.1.0 is published, soversion>=: 10069.1.0resolves and CI has been retriggered. cpp and desktop are green.
test-darwin-x64fails onQwen3 sliding context hard-fails stale reasoning compactioninreasoning.test.js, a knowndarwin-x64 issue on
mainthat this PR does not touch and that also reproduces ontmp-ext-rebase-fabric-b10297.Dispatched through the CI router at the branch ref, so the phones run this PR's own prebuilds
rather than the published package. Android passed. On iOS every shard passed except
lightBoniPhone 16, where the Device Farm host could not reach PyPI and failed to install its own tooling
before any test ran. The same shard passed on iPhone 17, and the iOS leg has been rerun.
@qvac/llm-llamacpp0.44.0 is what shipsimage_no_upscale, and its changelog entry documents boththat option and the fabric bump.