Skip to content

QVAC-24975 feat[translation-nmtcpp]: align with main-gpu selection contract - #4557

Open
Alok-Ranjan23 wants to merge 20 commits into
mainfrom
feat/QVAC-24975-translation-nmtcpp-main-gpu
Open

Alok-Ranjan23 wants to merge 20 commits into
mainfrom
feat/QVAC-24975-translation-nmtcpp-main-gpu

Conversation

@Alok-Ranjan23

Copy link
Copy Markdown
Contributor

🎯 What problem does this PR solve?

  • translation-nmtcpp had its own bespoke GPU-selection surface, out of step with the shared main-gpu contract introduced across the other ggml/whisper addons under QVAC-24975.

📝 How does it solve it?

  • Accepts main-gpu (and case-alternative main_gpu alias — exactly one) as either a raw ggml registry index (number or integer string) or "dedicated" / "integrated" (case-insensitive).
  • Rejects combining the new selector with the pre-existing gpu_backend/gpu_device (or camelCase aliases).
  • Raw in-range index preserves its registry identity through backend/safety filtering; excluded targets fall back to CPU. Out-of-range indices warn and use the default dedicated-first selection. Class selectors are strict — unavailable classes fall back to CPU.
  • Adds the surface to the JS wrapper, native forwarding, AddonJs, TypeScript types, README, and consumer type tests.

🧪 How was it tested?

  • packages/translation-nmtcpp/addon/tests/nmt_gpu_selection_test.cpp + additions to translation_tests.cpp cover the new native selector paths.
  • packages/translation-nmtcpp/test/unit/wrapper-lifecycle.test.js exercises the JS validation branches.
  • packages/translation-nmtcpp/test/types/consumer-cjs.test-d.ts type-tests both the widened number | string accepted type and the @ts-expect-error rejection surface.

🔌 API Changes

```typescript
new TranslationNmtcpp({
modelType: "IndicTrans",
"main-gpu": "dedicated",
});

new TranslationNmtcpp({
modelType: "IndicTrans",
main_gpu: 1,
});
```

Refs: QVAC-24975.

Alok-Ranjan23 and others added 2 commits September 17, 2026 15:06
Widens the TypeScript accepted-value type for main-gpu / main_gpu to
number | string so runtime coercion of integer strings and case-alt
class names is not blocked at the type layer, matching the shape used
by ocr-ggml and the other sibling packages. Updates the consumer type
test to reflect the widened contract.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@Alok-Ranjan23
Alok-Ranjan23 requested review from a team as code owners September 17, 2026 15:46
@github-actions

github-actions Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Review Status

Current Status: ❌ PENDING
Approvals so far: none

Pending reviews: Needs 1 Management or Team Lead, and 1 more from Management, Team Lead, or Member.

@github-actions

Copy link
Copy Markdown
Contributor

License compliance — clean

No 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):

  • ./docs/website
  • ./packages/fabric/test/integration
  • ./packages/llm-llamacpp/benchmarks/server
  • ./packages/llm-llamacpp/benchmarks/performance
  • ./packages/inference-addon-cpp/mobile
  • ./packages/asr-ggml/benchmarks/server
  • ./packages/embed-llamacpp/benchmarks/server
  • ./packages/embed-llamacpp/benchmarks/performance
  • ./packages/sdk/e2e
  • ./packages/vla-ggml/sim/server
  • ./.github/actions/release-merge-guard

Numeric-branch validation in TranslationModel::setConfig threw with a
shorter error string that omitted the class-name suffix present on the
string-branch and JS/AddonJs layers. That drift meant callers could see
two different diagnostics for the same semantic constraint depending on
how the value was typed. Match the other message so users always learn
that dedicated / integrated are valid alternatives.
Aligns the new and modified translation-nmtcpp sources with the shared
lint-cpp/.clang-format (PointerAlignment: Left, AlwaysBreak
open-bracket) so the on-pr cpp-lint step passes.
Local clang-format-18 diverged from CI's LLVM 22 on the AlwaysBreak
policy for open-parenthesis in the Adreno-guard QLOG hunk. Re-run
git-clang-format with the pinned CI version to match.
Aligns the throw text with the canonical wording used by ocr-ggml so
callers see one consistent message across every addon that ships the
shared main-gpu selection contract. Test matchers use /main-gpu/ and
still pass.
Mirrors the diagnostic pattern PR #4327 established for llm-llamacpp so
operators can see which specific GPU-type registry slots the addon's
family allowlist (and, on whisper packages, the Adreno-Vulkan safety
guard) refused when GPU execution was requested but no eligible device
was available. Extends the native gtest with a case that captures the
new observable identity strings.
freddy311082
freddy311082 previously approved these changes Sep 17, 2026

@GustavoA1604 GustavoA1604 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[medium] packages/translation-nmtcpp/addon/src/model-interface/TranslationModel.cpp — line 719 (if (selector != config.end()) {)

mainGpu_ is only updated when the replacement config contains a selector. Calling setConfig({use_gpu: 1, main-gpu: "dedicated"}), loading, then
replacing it with setConfig({use_gpu: 1}) leaves "dedicated" active and can incorrectly fall back to CPU on an integrated-only host. Reset
mainGpu_ and its selection state when the incoming map omits the selector, and add a reconfiguration regression test.

[low][qvac-packages] packages/translation-nmtcpp/src/index.ts — line 753 ("main-gpu"?: number | string;)

This changes the published NPM API and device-selection behavior, but packages/translation-nmtcpp/CHANGELOG.md has no [Unreleased] entry. Add a
concise consumer-facing entry describing the selector and its fallback/conflict behavior.

GustavoA1604
GustavoA1604 previously approved these changes Sep 18, 2026
ishanvohra2
ishanvohra2 previously approved these changes Sep 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants