QVAC-17481 feat[api]: integrate @qvac/classification-ggml into SDK#2236
Conversation
simon-iribarren
left a comment
There was a problem hiding this comment.
CI Status
Red and blocking. validate-pr fails because the [api] PR body does not include a fenced code block showing new API usage. SDK checks and CodeQL pass; test/build dispatch jobs are skipped.
API Surface & Tagging
[api] is only appropriate if this PR exposes classification through the SDK. The current body does not satisfy the required API evidence, and the diff appears to add only the addon dependency rather than wiring a public SDK classification surface.
Findings
-
Blocking: this is currently a dependency-only change, not an SDK integration. Adding
@qvac/classification-ggmltopackages/sdk/package.jsondoes not expose model types/aliases, plugin exports, schemas, client API, default registration, docs, or tests. Users still cannot call a classification SDK API from@qvac/sdkbased on this diff. -
Blocking:
[api]body evidence is missing, and CI is already failing on that validator error. Please include a fenced usage example for the actual SDK API once the integration is wired. -
Major: no SDK tests or docs cover the claimed integration. The PR should prove model constants/plugin registration/runtime loading/client behavior, or narrow the title/body to the actual dependency maintenance change.
Recommendation
Request changes. Red PR validation alone blocks approval, and the implementation does not currently deliver the claimed SDK API integration.
Tier-based Approval Status |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…cation-ggml-sdk-v2 # Conflicts: # packages/sdk/client/api/index.ts # packages/sdk/index.ts
7a089a5
|
/review |
2 similar comments
|
/review |
|
/review |
…cation-ggml-sdk-v2 # Conflicts: # packages/sdk/e2e/tests/classification-tests.ts # packages/sdk/e2e/tests/desktop/executors/classification-executor.ts # packages/sdk/e2e/tests/mobile/executors/classification-executor.ts
72f028f
Preview deployments for qvac-docs-staging ⚡️
Commit: Deployment ID: Static site name: |
🎯 What problem does this PR solve?
@qvac/classification-ggmladdon (ImageClassifier, bundled MobileNetV3-Small that returnsfood/report/other) shipped in0.1.0, but the SDK had no way to load or run it — consumers had to depend on the addon directly and bypassloadModel/unloadModel, the plugin registry, the worker lifecycle, and the cross-runtime client surface.loadModel({ modelType: "classification" })and runclassify({ modelId, image }); the model uses the GGUF bundled inside the addon package (no download required), and the existing per-runtimeloadModel/ RPC / Pear / mobile-bundle plumbing handles it the same way it handles every other built-in addon.Reopened from #2056 after the May-24 history rewrite invalidated the original branch. Foundational plugin code recovered verbatim from @DmitryMalishev's original commit; catalog wirings re-applied against the new
main(which had since addedparakeet-transcription,ggml-vla, etc.).📝 How does it solve it?
server/bare/plugins/ggml-classification/(plugin.ts+ops/classify.ts) following the same pattern asonnx-ocr/ggml-vla. The plugin wrapsImageClassifierand exposes a single streaming handler (classify) declared withcancel: { scope: "none" }— classification is a single forward pass with no addon-side cancel surface.skipPrimaryModelPathValidation: truebecause the model is bundled inside the addon, soloadModeldoes not require amodelSrc.classify()client API underclient/api/classify.ts— accepts aUint8Arrayimage (JPEG/PNG, or raw RGB withwidth/height/channels: 3), encodes to base64 for RPC transport, and returnsClassificationResult[]({ label, confidence }, sorted descending)."classify"routed through a newserver/rpc/handlers/classify.ts(uses the shareddispatchPluginStreamhelper) and registered inhandler-registry.ts. This makesclassify()work with the same RPC layer the other built-in clients use, not justinvokePluginStream.loadModelsupports optionalmodelSrcfor classification — the union arm inloadModelSrcRequestSchemaplus the corresponding parser arm inloadModelOptionsBaseSchemaboth accept the emptymodelSrccase. Reuse with a custom GGUF works too: passmodelSrc: "/path/to/my-classifier.gguf"andmodelConfig.modelPathbecomes the source of truth.topKflows through load config — the plugin wraps the addon so a load-timetopK(loadModel({ modelType: "classification", modelConfig: { topK: 3 } })) becomes the default for everyclassify()on that model, while per-calltopKoverrides via the standard{ ...defaults, ...opts }precedence.classificationConfigSchema,classifyRequestSchema,classifyResponseSchema,classificationResultSchema, plus theClassifyClientParams/ClassificationResulttypes.ModelType.ggmlClassification+"classification"alias,classificationModelTypeSchema,ENGINE_TO_ADDON/LEGACY_ENGINE_TO_CANONICALentries,modelRegistryEntryAddonSchema+modelRegistryEngineSchemaenum extensions,modelInfoSchema.addonenum extension,CANONICAL_TO_ALIAS+MODEL_CONFIG_SCHEMASentries,requestSchema/responseSchemaunion extensions.PLUGIN_CLASSIFICATION = "@qvac/sdk/ggml-classification/plugin"added toSDK_DEFAULT_PLUGINS;ADDON_CLASSIFICATION = "@qvac/classification-ggml"added to the addon-constants block.pear/pre.tsBUILTIN_PLUGINS+BUILTIN_PLUGIN_EXPORTSupdated so generated Pear workers import and registerclassificationPluginfrom the default-plugin set../ggml-classification/pluginexposed inpackage.jsonexports.@qvac/classification-ggml: ^0.1.0(now published; was previouslyfile:..during local development).🧪 How was it tested?
test/unit/classification-schemas.test.ts(25 tests): config / result / request / response schemas including the strict-unknown-key rejection, raw-RGBchannels: 3constraint,loadModelSrcRequestSchema+loadModelOptionsBaseSchemaintegration (canonical + alias paths),modelInfoSchema.addonenum coverage, plugin registration + dispatch through the realhandlePluginInvoke, andclassifyop base64 round-trip +model.classify()opts forwarding + missing-method error path.test/unit/load-model-schema.test.ts(3 new cases):loadModelSrcRequestSchemaaccepts classification load with emptymodelSrc(bundled weights), withmodelConfig.topK, and with a custom GGUFmodelSrc.test/unit/plugin-cancel-capability.test.ts(1 truth-table row):classify: { scope: "none" }, guards against silent regressions where a future plugin tweak forgets to keepcancelin sync with the addon. Validated againstpluginHandlerDefinitionRuntimeSchema.bun run test:unit; full unit suite stays green.tests-qvac/:tests-qvac/tests/classification-tests.tsdefines 4 cases:classification-results-shape(smoke): non-empty{label, confidence}array, every confidence in[0, 1], sorted descending.classification-confidence-sum: softmax probabilities sum to ≈1 within1e-3.classification-topk(smoke):topK: 1truncates to exactly one result.classification-invalid-image: 4-byte garbage payload rejects cleanly, and a follow-up validclassify()proves the addon does not wedge on the error path.tests-qvac/tests/desktop/executors/classification-executor.tswires those to the SDK'sclassify()against the bundled MobileNetV3-Small usingtests-qvac/assets/images/elephant.jpg(already in repo).consumer.tsregisters the"classification"resource and the executor;test-definitions.tsaddsclassificationTeststo the master list so the consumer-desktop test runner actually picks them up.ResourceManager.ModelDefinition.constantis now optional, withaddConstant/ensureLoadedguarded so addons that ship bundled weights (like classification) can be registered without a registry constant or pre-download entry.bun run lint+bun run buildclean againstupstream/main; tests-qvac typechecks clean against the local SDK snapshot.🔌 API Changes
Additive only — no breaking changes:
See
packages/sdk/examples/classification/classify-image.tsfor the full demo.Review comments addressed (carried from #2056)
All four points raised by @maxim-smotrov on the original PR are fixed in this revision:
loadModelSrcRequestSchemaarm —loadClassificationModelRequestSchemanow in the union, soloadModel({ modelType: "classification" })parses throughrequestSchema.parsebefore transport.server/rpc/handlers/classify.tsdispatches viadispatchPluginStream, registered inhandler-registry.tsas{ type: "stream" }.topKin load config silently ignored — the plugin wrapsImageClassifierso the load-timetopKbecomes the default forclassify()calls; per-calltopKstill overrides.pear/pre.tsBUILTIN_PLUGINSandBUILTIN_PLUGIN_EXPORTSboth include the classification plugin; generated Pear workers now import and register it.