QVAC-23995: migrate vla-ggml to shared @qvac/fabric runtime - #3998
QVAC-23995: migrate vla-ggml to shared @qvac/fabric runtime#3998jpgaribotti wants to merge 3 commits into
Conversation
Move ggml off the static qvac-fabric vcpkg port onto @qvac/fabric (^0.6.0): use qvac-addon fabric helpers, preload fabric in binding.js, resolve backends from node_modules on desktop, and stage fabric for C++ unit tests. Remove qvac-fabric[hip-backend] from vcpkg (ROCm/HIP now comes from fabric prebuilds). Register vla-ggml in fabric-consumers npm_runtime and wire on-pr-vla fabric-stack overlay through cpp-tests, prebuilds, and integration jobs. Bump @qvac/vla-ggml to 0.23.0.
Review StatusCurrent Status: ❌ PENDING Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member. |
License compliance — findings detected (warn-only)Critical: 0 · High: 3 · Medium: 0
How to resolve a blocking finding:
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 1095 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/ |
… ASAN Port classification-ggml's addon-test runner so ASAN_OPTIONS relaxes alloc-dealloc-mismatch and leak detection at the @qvac/fabric boundary. Wire test:cpp:run and cpp-tests-vla through the script instead of invoking addon-test directly with a workflow env var that did not reach Linux CI.
| qvac_addon_use_fabric() | ||
|
|
||
| add_bare_module(qvac-lib-infer-vla EXPORTS ${BACKEND_DL_LIBS}) | ||
| add_bare_module(qvac-lib-infer-vla EXPORTS) |
There was a problem hiding this comment.
The mobile branch of resolveBackendsDir() returns <pkg>/prebuilds, and this PR removes the only code that ever populated it with ggml backend modules.
The base branch staged them explicitly, with Android as the named target:
set(BACKEND_DL_LIBS "")
if((ANDROID OR UNIX) AND NOT APPLE)
foreach(_backend ${GGML_AVAILABLE_BACKENDS})
list(APPEND BACKEND_DL_LIBS INSTALL TARGET ggml::${_backend})
endforeach()
endif()
add_bare_module(qvac-lib-infer-vla EXPORTS ${BACKEND_DL_LIBS})Nothing replaces it: there is no INSTALL TARGET left anywhere in cmake/qvac-addon/qvac-addon.cmake. qvac_addon_use_fabric() only resolves the package and computes BACKENDS_SUBDIR_VALUE, qvac_addon_link_fabric() only links, and the sole thing that copies backends is the test-only qvac_addon_stage_fabric_for_test(). qvac_addon_finalize() still defines GGML_BACKEND_DL for Android, so Android must dlopen real .so files, and mobile:copy-prebuilds only fans android-arm64 out to the other arch names.
Impact: on Android, native appends android-arm64/qvac__fabric to the returned root, ggml_backend_load_all_from_path() registers nothing, and the first load throws "no CPU backend available". packages/fabric/INTEGRATION.md lists that companion install as mandatory checklist item 5. Mobile integration is workflow_dispatch-only, so no PR check catches it.
Suggested fix: re-add an install of the fabric companion and its backend modules into prebuilds/<host>/qvac__fabric/, so the path the mobile fallback resolves to is populated at pack time. Worth a dispatch of integration-mobile-test-vla.yml first — if mobile staging happens outside this repo, this is a false alarm and the comment at index.js:20-22 should say where.
| "bugs": "https://github.com/tetherto/qvac/issues", | ||
| "homepage": "https://qvac.tether.io", | ||
| "dependencies": { | ||
| "@qvac/fabric": "^0.6.0", |
| - The `qvac-fabric[hip-backend]` vcpkg feature is removed from this package; ROCm/HIP | ||
| backends are provided by `@qvac/fabric` when present in its prebuilds. |
There was a problem hiding this comment.
The CHANGELOG states ROCm/HIP backends "are provided by @qvac/fabric when present in its prebuilds"; no build path in the monorepo puts them there, so HIP execution is silently dropped.
Two verified facts against the premise:
packages/fabric/vcpkg.jsondeclares only avk-profilingfeature. It never requestship-backend..github/workflows/prebuilds-fabric.ymlpasses onlyartifact-name-prefix: fabric-andinclude-vulkan-sdk: true. It never setsinclude-rocm, whose default isfalseinreusable-prebuilds.yml.
Repo-wide, hip-backend appears in no other consumer's manifest — vla-ggml was the only one requesting it — and include-rocm has no enabling caller anywhere under .github/.
Impact: libqvac-ggml-hip.so ships in no @qvac/fabric prebuild, so the HIP-preference branch in addon/src/utils/BackendSelection.cpp:121-148 becomes unreachable — hipDev is always null and pickBestGpuDevice() falls through to Vulkan. Nothing breaks, but the ~18% warm-path win #2781 was built for (479 → 384 ms on gfx1151) is given back on that target, and the ROCm build and lint steps were removed in the same change, so CI cannot detect it.
Suggested fix: either add hip-backend to packages/fabric/vcpkg.json and include-rocm: true to prebuilds-fabric.yml, so the shared runtime actually ships what this line promises; or drop the claim, state plainly that HIP is dropped, and remove the now-dead branch in BackendSelection.cpp. Either way this line needs changing — as written it asserts something untrue about where the backend comes from.
🎯 What problem does this PR solve?
@qvac/vla-ggmlstill compiled and bundled the staticqvac-fabricvcpkg port, duplicating the ggml runtime and compute backends that@qvac/fabricalready ships once per process..github/fabric-consumers.jsonnpm_runtime, so fabric-stack PRs could not validate VLA against PR-built@qvac/fabricprebuilds.on-pr-vlastill ranverify-qvac-fabric-lockstepand built ROCm/HIP viaqvac-fabric[hip-backend]in vcpkg/prebuilds — patterns superseded by the shared npm runtime model used by@qvac/classification-ggml.📝 How does it solve it?
packages/vla-ggml,0.22.0→0.23.0):@qvac/fabric(^0.6.0); removeqvac-fabric(andhip-backend) fromvcpkg.json.CMakeLists.txttoqvac-addonfabric helpers (qvac_addon_use_fabric, dynamicDT_NEEDED: qvac__fabric@0.bare); drop the static ggml backend install loop.@qvac/fabricinbinding.js; resolve ggml backends fromnode_modules/@qvac/fabric/prebuilds/on desktop viaresolveBackendsDir()(mobile fallback to localprebuilds/).@qvac/fabricfor C++ unit tests (qvac_addon_stage_fabric_for_test, relaxed ASAN options).vla-ggmlin.github/fabric-consumers.jsonnpm_runtime.verify-fabric-lockstepwithdetect-fabric-stack/resolve-fabric-prebuildsonon-pr-vla.yml(mirrors classification-ggml).fabric-overlay-artifactthroughcpp-tests-vla,prebuilds-vla, andintegration-test-vla; remove vla-side ROCm prebuild/lint wiring (HIP comes from fabric prebuilds).🧪 How was it tested?
Local (linux-x64):
npm install→@qvac/fabricprebuilds present undernode_modules/.bare-make generate && bare-make build && bare-make install→prebuilds/linux-x64/qvac__vla-ggml.bareonly (no bundled ggml backend.sofiles);readelfshowsNEEDED: qvac__fabric@0.bare.npm run test:unit— 6/6 JS tests passed.npm run test:cpp— 63 passed, 30 skipped (GR00T/Pi05 oracle fixtures not staged locally); fabric backends staged next toaddon-test.bare -e "require('@qvac/fabric'); require('./binding')"— binding loads successfully.CI:
verify+cpplabels onon-pr-vla(sanity, cpp-lint, cpp-tests matrix).prebuildslabel — fresh native prebuilds without bundled ggml backends.desktoplabel — integration tests with@qvac/fabricavailable.packages/fabric+prebuildslabel to confirm overlay path viaresolve-fabric-prebuilds.Updated How was it tested section for PR #3998:
🧪 How was it tested?
Local (linux-x64)
npm install—@qvac/fabricprebuilds present undernode_modules/.bare-make generate && bare-make build && bare-make install—prebuilds/linux-x64/qvac__vla-ggml.bareonly (no bundled ggml backend.sofiles);readelfshowsNEEDED: qvac__fabric@0.bare.npm run test:unit— 6/6 JS tests passed.npm run test:scripts— 12/12run-cpp-tests.jsrunner regression tests passed.npm run test:cpp— 63 passed, 30 skipped (GR00T/Pi05 oracle fixtures not staged locally); fabric backends staged next toaddon-testviaqvac_addon_stage_fabric_for_test. Run withASAN_OPTIONS=alloc_dealloc_mismatch=0:detect_leaks=0:abort_on_error=1(now applied automatically byscripts/run-cpp-tests.js).bare -e "require('@qvac/fabric'); require('./binding')"— binding loads successfully.CI (
verify+cpp+prebuilds+desktoplabels onon-pr-vla)Migration commit (
8471146) — run 32499686737:alloc-dealloc-mismatchat@qvac/fabricteardown (instrumentedaddon-testvs non-ASan fabric prebuild)ASAN fix commit (
f503c97) — run 32731496250 (autopull_request_targeton push):pull_request_targetran the base-branchcpp-tests-vla.yml(direct./addon-test, notnpm run test:cpp:run), so the ASAN fix was not exercised on that auto-triggered runFull pipeline validation (
f503c97) — run 32739988913 (manualworkflow_dispatchon branch workflow):on-pr-vlapipeline success (19m 22s)npm run test:cpp:run/run-cpp-tests.js; no ASAN fabric-boundary abort;cpp-test-results-linux-x64artifact uploadedFabric-stack overlay
packages/fabric+prebuildslabel to confirmresolve-fabric-prebuildsoverlay path: Test/fabric stack #4033Actions validated wiring and smoke tests pass.
Note for reviewers
Linux cpp ASAN is handled the same way as
@qvac/classification-ggml:scripts/run-cpp-tests.jssets relaxedASAN_OPTIONSat the fabric boundary. Auto PR checks usepull_request_targetand load reusable workflow YAML frommainuntil this PR merges; the manual dispatch above validates the branch workflow end-to-end onf503c97.