QVAC-17434 chore[bc]: migrate SDK plugins to new addon constructor shape#1688
Conversation
|
review |
Tier-based Approval Status |
|
review |
|
/review |
1 similar comment
|
/review |
4079bde to
ffade03
Compare
|
/review |
Root cause of the SDK unit test failureLooking at run 24882238163, the failing step is Run unit tests with this crash (not a test assertion failure):
Why this regressed nowIt's caused by commit
The earlier successful Suggested fixRevert the test's import to the direct file path: import { expandGGUFIntoShards } from "@/server/utils/expand-gguf-shards";The plugin imports ( As a broader cleanup (optional, separate PR), |
Dismissing my own approval — CI is failing due to the barrel import change in 991686b; PR needs the fix before merging.
The unit test runs under Bun. Importing through the `@/server/utils` barrel drags in `checksum.ts` and `formatting.ts`, both of which `import crypto from "bare-crypto"`. Bun does not implement Bare's `require.addon()`, so evaluating that import chain throws `TypeError: require.addon is not a function` at load time and the test run exits non-zero before any assertion executes. The plugin files (`llamacpp-completion/plugin.ts` and `llamacpp-embedding/plugin.ts`) keep the barrel import because they only evaluate under Bare. Only the Bun-loaded unit test needs to go through the module path directly.
|
/review |
The plugin's `transformLlmConfig` was missing the SDK-to-addon translation for `toolsMode`. Without it, the addon receives `tools_mode` as a CLI-style flag, which it does not recognize, and load fails with `commonParamsParse: invalid argument: --tools-mode`. This translation existed in commit 2840f4d but was lost during a later main merge that brought in the new addon constructor shape (PR #1688). Reinstating the original mapping: toolsMode: "dynamic" → tools_compact: "true" toolsMode: "static" → tools_compact: "false"
…r-audio (QVAC-14392) (#1761) Two manifest-only changes that together unblock the SDK-side removal of @qvac/dl-* packages tracked in QVAC-14392. @qvac/infer-base - Drop the vestigial peerDependencies."@qvac/dl-hyperdrive": "^0.1.0" declaration. The runtime moved off DataLoaders in #1688 (Loader interface lives in infer-base, ready()/close() optional), so this peer-dep is no longer required by anything in the package. Lint, test:dts, and brittle-bare unit tests (118/118) all pass with the declaration removed. @qvac/decoder-audio - Bump "@qvac/infer-base" from "^0.1.0" to "^0.4.0". This stops pulling the published infer-base@0.1.1 (which still carries the old dl-hyperdrive peer-dep on its 0.1.x line) into consumers' install trees. Local install resolves @qvac/infer-base@0.4.0, decoder-audio's BaseInference subclassing in index.js continues to work unchanged, lint and brittle-bare unit tests (9/9) pass. Effect on the SDK: once both packages are republished (infer-base @ 0.4.1, decoder-audio @ 0.3.8 -- or whichever versions ship), the "overrides": { "@qvac/dl-hyperdrive": "^0.2.0" } block in packages/sdk/package.json can be removed and @qvac/dl-hyperdrive plus @qvac/dl-base fall out of the SDK's dep tree entirely. That cleanup is tracked in #1754. NOTICE files regenerated for both packages.
PATCH bump. Drops the vestigial peerDependencies."@qvac/dl-hyperdrive" declaration from the published manifest. The runtime moved off DataLoaders in #1688 (Loader interface lives in @qvac/infer-base, ready()/close() optional), so the peer-dep was no longer required by anything in the package. Lint clean + 118/118 brittle-bare unit tests pass. Refs: #1761
…r-audio (QVAC-14392) (#1761) Two manifest-only changes that together unblock the SDK-side removal of @qvac/dl-* packages tracked in QVAC-14392. @qvac/infer-base - Drop the vestigial peerDependencies."@qvac/dl-hyperdrive": "^0.1.0" declaration. The runtime moved off DataLoaders in #1688 (Loader interface lives in infer-base, ready()/close() optional), so this peer-dep is no longer required by anything in the package. Lint, test:dts, and brittle-bare unit tests (118/118) all pass with the declaration removed. @qvac/decoder-audio - Bump "@qvac/infer-base" from "^0.1.0" to "^0.4.0". This stops pulling the published infer-base@0.1.1 (which still carries the old dl-hyperdrive peer-dep on its 0.1.x line) into consumers' install trees. Local install resolves @qvac/infer-base@0.4.0, decoder-audio's BaseInference subclassing in index.js continues to work unchanged, lint and brittle-bare unit tests (9/9) pass. Effect on the SDK: once both packages are republished (infer-base @ 0.4.1, decoder-audio @ 0.3.8 -- or whichever versions ship), the "overrides": { "@qvac/dl-hyperdrive": "^0.2.0" } block in packages/sdk/package.json can be removed and @qvac/dl-hyperdrive plus @qvac/dl-base fall out of the SDK's dep tree entirely. That cleanup is tracked in #1754. NOTICE files regenerated for both packages.
PATCH bump. Drops the vestigial peerDependencies."@qvac/dl-hyperdrive" declaration from the published manifest. The runtime moved off DataLoaders in #1688 (Loader interface lives in @qvac/infer-base, ready()/close() optional), so the peer-dep was no longer required by anything in the package. Lint clean + 118/118 brittle-bare unit tests pass. Refs: #1761
…r-audio (QVAC-14392) (#1761) Two manifest-only changes that together unblock the SDK-side removal of @qvac/dl-* packages tracked in QVAC-14392. @qvac/infer-base - Drop the vestigial peerDependencies."@qvac/dl-hyperdrive": "^0.1.0" declaration. The runtime moved off DataLoaders in #1688 (Loader interface lives in infer-base, ready()/close() optional), so this peer-dep is no longer required by anything in the package. Lint, test:dts, and brittle-bare unit tests (118/118) all pass with the declaration removed. @qvac/decoder-audio - Bump "@qvac/infer-base" from "^0.1.0" to "^0.4.0". This stops pulling the published infer-base@0.1.1 (which still carries the old dl-hyperdrive peer-dep on its 0.1.x line) into consumers' install trees. Local install resolves @qvac/infer-base@0.4.0, decoder-audio's BaseInference subclassing in index.js continues to work unchanged, lint and brittle-bare unit tests (9/9) pass. Effect on the SDK: once both packages are republished (infer-base @ 0.4.1, decoder-audio @ 0.3.8 -- or whichever versions ship), the "overrides": { "@qvac/dl-hyperdrive": "^0.2.0" } block in packages/sdk/package.json can be removed and @qvac/dl-hyperdrive plus @qvac/dl-base fall out of the SDK's dep tree entirely. That cleanup is tracked in #1754. NOTICE files regenerated for both packages.
…ape (#1688) * chore[bc|notask]: migrate SDK plugins to new addon constructor shape The three addon refactors (#1493 embed, #1494 LLM, #1496 diffusion) landed on main without the matching SDK plugin migration. Their published releases (`@qvac/embed-llamacpp@0.14.0`, `@qvac/llm-llamacpp@0.16.0`, `@qvac/diffusion-cpp@0.3.0`) dropped the `BaseInference` / `WeightsProvider` loader-and-disk-path constructor and replaced it with a single-argument `{ files, config, logger, opts }` shape that takes pre-resolved absolute paths. Plugin changes (llamacpp-completion, llamacpp-embedding, sdcpp-generation): - Construct the addon with the new single-argument shape. LLM and embed pass `files.model: string[]`; diffusion passes `files.model: string` plus renamed companion keys (`clipL`/`clipG`/`t5Xxl`/`llm`/`vae`). - Drop `FilesystemDL`, `parseModelPath`, and the `asLoader` adapter from each plugin. Addons now receive absolute paths directly. - Return `{ model }` instead of `{ model, loader }`. Loader field removal across the plugin-registry contract: - `PluginModelResult.loader?:` removed from the interface. - `LocalOptions.loader?: FilesystemDL` and the `registerModel` options slot for it both dropped. - `unloadAllModels` and `unloadModel` no longer call `entry.local.loader.close()`. - `loadModel`'s cast tightened to `{ model: AnyModel }` and the conditional loader spread into `registerModel` removed. - Non-migrated plugins (nmt, whisper, ocr, tts, parakeet) also simplified to `return { model }`. Their addons already stopped accepting a loader in earlier refactors; the pass-through was dead code. Parakeet drops its now-unused `new FilesystemDL({ dirPath })` + `parseModelPath` call. - `server/bare/utils/loader-adapter.ts` (asLoader) and `server/utils/model-path.ts` (parseModelPath) deleted. Both had no remaining callers after the plugin cleanup. Sharded-GGUF helper and tests: - New `packages/sdk/server/utils/expand-gguf-shards.ts` turns a single sharded GGUF path into the ordered list the new addon contract expects (`.tensors.txt` companion first, then `-NNNNN-of-NNNNN.gguf` shards). Pure string manipulation, POSIX and Windows separator handling. - 9 unit tests cover non-sharded paths, first-shard input, non-first-shard input, nested directories, single-shard (1-of-1), relative paths, Windows backslash separators, and a substring-match regression test (filename containing a shard-like pattern mid-basename must not match). Dependency changes: - Bump SDK deps to the published addon versions: `@qvac/diffusion-cpp ^0.3.0`, `@qvac/embed-llamacpp ^0.14.0`, `@qvac/llm-llamacpp ^0.16.0`. - Drop `@qvac/dl-filesystem` from `dependencies`; no remaining consumer in the SDK. - `bun.lock` refreshed; `bun install --frozen-lockfile` clean. Docs: the custom-plugin example in `docs/website/.../write-custom-plugin.mdx` drops the stale `loader: null` return to match the new `{ model }` shape. Test fixtures updated: `plugin-system.test.ts` and `sdcpp-plugin.test.ts` drop `loader: {}` / `loader: undefined` from mock `createModel` returns and from `registerModel` calls (the field no longer exists on either interface). Verified: `bun run build` clean (`--max-warnings=0`), 423/423 unit tests pass, all three diffusion examples and 19 LLM examples and 4 RAG examples run end-to-end against the real addons. Supersedes #1510, which carried stale merge history from the addon-refactor side-branches and could not be rebased onto current main cleanly. * fix(examples): set FLUX guidance params on diffusion examples Both `diffusion-txt2img.ts` and `diffusion-flux2-klein.ts` default to FLUX.2 Klein but neither was sending the right guidance knobs. stable- diffusion.cpp gates the unconditional inference branch on `guidance.txt_cfg != 1.0` (`stable-diffusion.cpp:3304`) and logs "use cfg-scale=1 for distilled models" (`:1667`); FLUX is a distilled model. The addon's `GenParams.cfgScale` defaults to `7.0f` (`SdGenHandlers.hpp:44`) and is assigned straight into `sample_params.guidance.txt_cfg` at `SdModel.cpp:499`, so omitting or leaving `cfg_scale` at the default forces the full CFG path on FLUX every step for zero quality benefit. `guidance: 3.5` is the FLUX distilled-guidance default and also needs to be set explicitly. Setting `guidance: 3.5, cfg_scale: 1` in both examples halves generation cost per step on FLUX. Measured on an RTX 5080 (Vulkan): 20-step txt2img drops from 17.1s to 9.1s; flux2-klein drops from 17.4s to 9.1s. * fix: simplify SDK shard expansion helpers Keep `expandGGUFIntoShards()` Bun-testable without introducing a separate shared shard-pattern module. This reduces refactor churn while preserving the sharded GGUF behavior expected by the SDK plugins. * fix(examples): remove unintended docs and FLUX comment churn Restore the custom-plugin docs example so this PR stays scoped to the SDK addon integration work. Remove the extra FLUX.2 guidance comments from the diffusion examples as requested. * fix: restore shard-utils JSDoc * fix: address review feedback from PR #1688 - `packages/sdk/server/bare/plugins/nmtcpp-translation/plugin.ts`: inline `path.dirname(modelPath)` and `path.basename(modelPath)` in `deriveColocatedBergamotVocabPaths` instead of importing `parseModelPath`. After rebasing onto main, `parseModelPath` no longer exists (this PR deletes `server/utils/model-path.ts`) but `#1707` reintroduced its usage here. `bare-path` is already imported in this file, so the inline form is a direct swap. - `packages/sdk/server/utils/expand-gguf-shards.ts`: drop the redundant `totalDigits = String(totalShards).padStart(5, "0")` round-trip and use the regex capture `match[3]` directly (it is already a 5-digit zero-padded string). Also drop the `!Number.isFinite(totalShards)` guard: a 5-digit regex match plus base-10 `parseInt` always produces a finite integer, so the check is dead defense. The `<= 0` guard is kept (pins the `00000-of-00000.gguf` edge case). - `packages/sdk/test/unit/expand-gguf-shards.test.ts`: add a test that pins the zero-total shard-count branch; `expandGGUFIntoShards` must return the input path unchanged for `empty-00000-of-00000.gguf` rather than an empty shard list. * chore: route expand-gguf-shards import through server/utils barrel - `packages/sdk/server/utils/index.ts`: add `expand-gguf-shards` to the barrel re-exports alongside the other utility modules. - `packages/sdk/server/bare/plugins/llamacpp-completion/plugin.ts` and `llamacpp-embedding/plugin.ts`: import `expandGGUFIntoShards` from `@/server/utils` instead of the module path directly. - `packages/sdk/test/unit/expand-gguf-shards.test.ts`: same. No behavior change; keeps a single re-export point for server utils. * fix: import expandGGUFIntoShards directly in unit test The unit test runs under Bun. Importing through the `@/server/utils` barrel drags in `checksum.ts` and `formatting.ts`, both of which `import crypto from "bare-crypto"`. Bun does not implement Bare's `require.addon()`, so evaluating that import chain throws `TypeError: require.addon is not a function` at load time and the test run exits non-zero before any assertion executes. The plugin files (`llamacpp-completion/plugin.ts` and `llamacpp-embedding/plugin.ts`) keep the barrel import because they only evaluate under Bare. Only the Bun-loaded unit test needs to go through the module path directly. --------- Co-authored-by: Yury Samarin <yuri.a.samarin@gmail.com>
…r-audio (QVAC-14392) (#1761) Two manifest-only changes that together unblock the SDK-side removal of @qvac/dl-* packages tracked in QVAC-14392. @qvac/infer-base - Drop the vestigial peerDependencies."@qvac/dl-hyperdrive": "^0.1.0" declaration. The runtime moved off DataLoaders in #1688 (Loader interface lives in infer-base, ready()/close() optional), so this peer-dep is no longer required by anything in the package. Lint, test:dts, and brittle-bare unit tests (118/118) all pass with the declaration removed. @qvac/decoder-audio - Bump "@qvac/infer-base" from "^0.1.0" to "^0.4.0". This stops pulling the published infer-base@0.1.1 (which still carries the old dl-hyperdrive peer-dep on its 0.1.x line) into consumers' install trees. Local install resolves @qvac/infer-base@0.4.0, decoder-audio's BaseInference subclassing in index.js continues to work unchanged, lint and brittle-bare unit tests (9/9) pass. Effect on the SDK: once both packages are republished (infer-base @ 0.4.1, decoder-audio @ 0.3.8 -- or whichever versions ship), the "overrides": { "@qvac/dl-hyperdrive": "^0.2.0" } block in packages/sdk/package.json can be removed and @qvac/dl-hyperdrive plus @qvac/dl-base fall out of the SDK's dep tree entirely. That cleanup is tracked in #1754. NOTICE files regenerated for both packages.
Note: be concise and prefer bullet points.
🎯 What problem does this PR solve?
main, but the matching SDK plugin migration did not.📝 How does it solve it?
llamacpp-completion,llamacpp-embedding, andsdcpp-generationplugins to the new single-argument addon constructor:{ files, config, logger, opts }.{ model }from migrated plugins and remove the old loader adapter path that is no longer used by those addons.packages/sdk/server/utils/expand-gguf-shards.tsso a single sharded GGUF path expands into the ordered.tensors.txt+ shard list expected by the addonfiles.modelcontract.FilesystemDL,loader-adapter,model-path) and bump the SDK's addon dependency versions to the refactored releases.🧪 How was it tested?
bun run buildinpackages/sdk.bun run test:unitinpackages/sdk.expand-gguf-shards.test.tscoverage for sharded and non-sharded paths, nested and relative paths,1-of-1shards, and Windows-style separators.bun install --frozen-lockfileafter the dependency bumps to confirm the lockfile resolves cleanly.💥 Breaking Changes
PluginModelResultno longer includesloader.loadermust update to return onlymodel.BEFORE:
AFTER: