2 commits

Author	SHA1	Message	Date
ACT900	5aada28da5	fix(embeddings): use system-matched onnxruntime-node CUDA build so CUDA 13 hosts use the GPU (#2341) ... * fix(embeddings): use system-matched onnxruntime-node CUDA build so CUDA 13 hosts use the GPU transformers.js exact-pins a CUDA-12 onnxruntime-node while gitnexus' own dep floats to a CUDA-13 build. npm/pnpm cannot dedupe an exact pin against a range, so npm i -g installs two copies and the gitnexus overrides block (root-only) is inert. On a CUDA-13-only host the nested CUDA-12 provider cannot load libcublasLt.so.12, the CUDA EP fails, and embeddings silently fall back to CPU (isCudaAvailable() also only probed .so.12). Add onnxruntime-node-resolver.ts (module.registerHooks redirect to the host-matching build, no-op elsewhere) mirroring onnxruntime-common-resolver.ts; probe libcublasLt .so.12 OR .so.13 against the copy that actually loads; unit test with 12 cases. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(embeddings): wire CUDA-13 build-match resolver into MCP query embedder The MCP query-time embedder (src/mcp/core/embedder.ts) has its own, separate initEmbedder() used for semantic search — it only called ensureOnnxRuntimeCommonResolvable() before importing transformers.js, so the CUDA-13 build-matching redirect added for the analyze/CLI embedder never applied here. A CUDA-13 host running MCP search with --embedding-device cuda still loaded the mismatched default onnxruntime-node build. Wire ensureOnnxRuntimeNodeMatchesSystem() into the same call site, mirroring the core embedder's ordering (registered after the common-resolver fallback, before the dynamic transformers import). * fix(embeddings): gate CUDA redirect decision on registerHooks availability decide() computed the CUDA-major redirect independent of whether Node's module.registerHooks API actually exists — only ensureOnnxRuntimeNodeMatchesSystem() checked that. On Node 22.0-22.14 (allowed by this package's engines floor; registerHooks needs >=22.15), isCudaAvailable() could therefore report a redirect target that ensureOnnxRuntimeNodeMatchesSystem() then silently failed to install, so transformers.js loaded the mismatched default onnxruntime-node build while the embedder still requested device:'cuda' against it — reintroducing the uncatchable native crash this probe exists to prevent. Move the registerHooks check to the top of decide() so the probe and the loader can never disagree, and skip CUDA-major probing entirely in that case (a redirect could never install anyway). Also fixes a related test-helper bug found while writing this unit's tests: loadResolver's destructuring default (`registerHooks = vi.fn()`) silently substituted a real mock function even when a test passed `registerHooks: undefined` to simulate Node < 22.15 — meaning the existing 'no-ops... when registerHooks is unavailable' test never actually exercised that path. Distinguish 'omitted' from 'explicitly undefined' via an 'in' check. * test(embeddings): drive decide() -> redirect:true and assert the resolve() closure The PR's actual shipped behavior — the installed registerHooks resolve() closure, and the full redirect-active decision path — had zero executed test coverage. All 4 prior ensureOnnxRuntimeNodeMatchesSystem tests avoided driving decide() into redirect:true because require.resolve/createRequire were never mocked, so the two-distinct-directory comparison decide() depends on always resolved against whatever's actually installed in this test's real node_modules (a single real copy, not the PR's two-copy scenario). Extend loadResolver()'s existing node:module mock to also fake createRequire, keyed by call origin, so resolveOurOrtNodeDir/resolveDefaultOrtNodeDir can be driven to two distinct fake directories with distinct CUDA majors — reaching redirect:true without adding any injection points to production code. Then capture the installed resolve() closure (mirroring the sibling onnxruntime-common-resolver.test.ts's captureResolve() pattern) and assert its three branches directly: onnxruntime-node redirect, onnxruntime-common redirect, and passthrough for any other specifier. * fix(embeddings): distinguish ldd detection-failure from no-CUDA-provider ortCudaMajor treated any execFileSync('ldd', ...) failure with no usable stdout (missing ldd binary, permission-denied .so, sandboxed exec) identically to 'CUDA provider genuinely absent'. The pre-PR detection (hasOrtCudaProvider) only used existsSync, never ldd, so this is a regression: a CUDA-12 host that worked fine before this PR can now silently fall back to CPU if ldd itself can't run, even though the provider .so and system CUDA libs are both genuinely present. readSoNeeded now reports whether ldd produced any usable output at all, distinct from 'ldd ran and just found no matching NEEDED entry' (the existing, already-handled '=> not found' case). When detection genuinely fails, log a warning so an operator can tell 'CPU fallback because detection itself failed' apart from 'CPU fallback because no CUDA build shipped' — the return value stays null either way (the type can't distinguish a third state), but the two cases are now observably different via the log. * fix(embeddings): check ourDir independently of whether defaultDir resolved decide()'s ourDir fallback lookup was nested inside 'if (systemMajor != null && defaultDir)', so a null defaultDir (transformers' own onnxruntime-node resolution failing outright, e.g. a partial/broken install) skipped checking ourDir entirely — getEffectiveOnnxRuntimeNodeDir() returned null even when gitnexus' own matching CUDA-13 copy would have resolved fine and worked. defaultDir resolving is not a precondition for the comparison: an unresolvable default already counts as 'the default doesn't match', so the ourDir check now runs whenever systemMajor is known, regardless of whether defaultDir resolved. * fix(embeddings): prefer CUDA 13 globally across the env-var directory scan detectSystemCudaMajor's CUDA_PATH/LD_LIBRARY_PATH scan returned on the first CUDA-major match within a single dir/sub pair, so a stale .so.12 found early (e.g. a leftover CUDA_PATH entry from a prior install) shadowed a genuine .so.13 found later in the search path, even though the scan's own ordering (checking 13 before 12 within each pair) was clearly intended to prefer 13 wherever possible. Keep scanning the full search space once a 12 is found, only returning early once a 13 is found (the best possible answer) or the space is exhausted. * fix(embeddings): have onnxruntime-common-resolver defer to the effective onnxruntime-node dir onnxruntime-common-resolver.ts independently re-derived transformers' default onnxruntime-node dir (its own copy of the 'resolve transformers' main entry, then onnxruntime-node' walk) to compute which onnxruntime-common to pair with — duplicating onnxruntime-node-resolver.ts's own walk, and capable of disagreeing with it: when the CUDA-major redirect is active, this hook would still pair onnxruntime-common with transformers' default (unredirected) onnxruntime-node, not the redirected copy the other hook just switched onnxruntime-node itself to. Have it call the already-exported getEffectiveOnnxRuntimeNodeDir() instead — the same decision the CUDA-major redirect hook uses — so both hooks always agree on which onnxruntime-node they're pairing onnxruntime-common against, and the duplicated resolve-walk is removed entirely rather than merely factored out. * fix(embeddings): cache the effective CUDA major to remove redundant subprocess spawns isCudaAvailable() in embedder.ts re-invoked ortCudaMajor/detectSystemCudaMajor directly even though decide() (via getEffectiveOnnxRuntimeNodeDir) had already computed both to make its redirect decision — a second, wasted ldconfig + up to 2 ldd spawns on every initEmbedder() call. Add effectiveMajor to the memoized Decision, computed once inside decide() alongside effectiveDir/systemMajor, and export a single isEffectiveCudaAvailable() that reads straight from the cached decision. embedder.ts's local isCudaAvailable() wrapper (and its now-unused getEffectiveOnnxRuntimeNodeDir/ortCudaMajor/detectSystemCudaMajor imports) is replaced by this one exported function. * fix(embeddings): surface CUDA redirect state at info level and in doctor A successful CUDA-build redirect logged only at logger.debug (filtered by the default 'info' level), and gitnexus doctor's embeddings section never mentioned the redirect at all — leaving no diagnostic path for 'why is my CUDA-13 host still on CPU' after this PR ships. Log the successful-redirect line at info (no-redirect/failure paths stay at debug, since those are the common, expected case). Add cudaRedirectDoctorStatus(), a pure summary of decide()'s already-computed decision mirroring doctor.ts's existing localEmbeddingDoctorStatus shape, and print it as a new literal (non-i18n) 'CUDA:' line in doctor's embeddings section alongside the existing 'Support:' line, matching that line's established convention. * test(embeddings): register onnxruntime-node-resolver.test.ts in the cross-platform subset The new test file guards on process.platform (linux/darwin cases) but was absent from cross-platform-tests.ts's PLATFORM_LOGIC list, which TESTING.md says platform-sensitive tests should be added to — so it never ran on the Windows/macOS CI matrix, only Ubuntu. Note: the sibling onnxruntime-common-resolver.test.ts has the identical, pre-existing gap (it predates this PR) — left as-is here, since fixing unrelated pre-existing test-registration debt is out of scope for this PR's own follow-up fixes. * test(embeddings): strengthen weak assertions, add garbled-output and CUDA_PATH coverage Three of the four ensureOnnxRuntimeNodeMatchesSystem tests only asserted 'doesn't throw' rather than a concrete outcome — including one literally named 'idempotent' that never asserted a call count on its own spy. Strengthen each to assert real outcomes (module stays functional after a no-op; spy call counts; return-value shape), while keeping the true install-once idempotency proof in the redirect-active test added earlier (this file's no-redirect scenario can't exercise it, since registerHooks is never called either way). Add the missing edge cases flagged in review: a CUDA_PATH-only fallback scan test (mirroring the existing LD_LIBRARY_PATH one), and garbled/ unrecognized ldconfig and ldd output cases for both detectSystemCudaMajor and ortCudaMajor, confirming neither falsely matches a CUDA major on unparseable input. Also parameterize the non-linux platform test across both darwin and win32 rather than darwin alone. Not changed: the process.env reassignment vs. Object.defineProperty 'inconsistency' flagged in review — process.env, unlike process.platform, has no getter-only restriction, so plain reassignment is already correct and switching it to Object.defineProperty would be unnecessary ceremony. * docs(embeddings): note the npm link/symlinked dev-checkout resolution caveat resolveOurOrtNodeDir/resolveDefaultOrtNodeDir anchor to this module's own real (post-symlink) location via import.meta.url, so a linked local dev checkout may resolve against its own node_modules rather than the consuming app's. Narrow, dev-only blast radius (regular npm/pnpm installs are unaffected) — document-only, no structural fix warranted. * fix(test): point the windowsHide spawn-family registry at the file that actually spawns hooks.test.ts's windowsHide regression check still listed gitnexus/src/core/embeddings/embedder.ts as a child_process-spawning file, but this PR itself already moved all execFileSync usage out of embedder.ts and into the new onnxruntime-node-resolver.ts — without updating this registry. The check was silently failing at the PR's own head commit (confirmed: 0 spawn-family calls found in embedder.ts, 'expected 0 to be greater than 0'), a pre-existing gap this fix-pass surfaced via a full-suite run rather than something introduced by any of the preceding follow-up commits. Swap the registry entry to onnxruntime-node-resolver.ts, which does import execFileSync (ldd + ldconfig, both already correctly passing windowsHide: true). * fix(test): make onnxruntime-node-resolver.test.ts path comparisons OS-agnostic Registering this file in cross-platform-tests.ts's PLATFORM_LOGIC (a prior commit in this series) means it now runs on the Windows CI matrix, not just Ubuntu — and several of the fakeDirs-based tests (redirect:true, ourDir-independent, subprocess-count, doctor-status) compared the resolver's real join()/dirname() output against hardcoded forward-slash fixture strings via exact-match or .startsWith(). Node's module is bound to path.win32 (or path.posix) based on the REAL host OS at process start — stubbing process.platform later, as these tests already do for the resolver's own platform branching, has no effect on it. So on a genuine Windows runner, join(effectiveDir, 'package.json') backslash-normalizes even under a faked platform:'linux', silently breaking every forward-slash comparison in this file: the createRequire dispatch would route to the wrong fake require, throw MODULE_NOT_FOUND, get swallowed by ensureOnnxRuntimeNodeMatchesSystem's outer try/catch, and registerHooks would never fire — the redirect-active tests would fail outright on Windows CI. Normalize every comparison point (the createRequire dispatcher, and the shared execFileSync/existsSync mocks) with a single toPosix() helper. Added a forceWin32Path test option (using path.win32's real join/dirname behavior) to prove this holds without needing an actual Windows runner — confirmed by temporarily reverting the fix and observing the new test fail with the exact predicted mismatch before restoring it. * chore(autofix): apply prettier + eslint fixes via /autofix command * fix(embeddings): keep CUDA auto-detect working on Node < 22.15 when the default build already matches The registerHooks guard in decide() returned effectiveMajor: null unconditionally, so on Node 22.0-22.14 / 23.0-23.4 (engines floor is >=22.0.0) isEffectiveCudaAvailable() was always false and a CUDA-12 host whose default onnxruntime-node build already matched — which needs no hook at all to use the GPU — silently regressed from CUDA to CPU on the auto device path (pre-PR isCudaAvailable() behavior). Probe the system and the default copy regardless of registerHooks availability; only the ourDir redirect branch stays gated on it, so the probe still never reports a redirect target that cannot be installed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>	2026-07-02 07:53:32 +01:00
Gergő Magyar	7f0ab87782	fix(embeddings): resolve onnxruntime-common under pnpm-strict / pnpm dlx (#307) (#2139) ... * fix(embeddings): resolve onnxruntime-common under pnpm-strict / pnpm dlx (#307) `@huggingface/transformers` does a bare `import 'onnxruntime-common'` from its shipped `dist/transformers.node.mjs`, but never declares onnxruntime-common in its own `dependencies`. npm's flat node_modules (and pnpm with hoisting) place it on transformers' resolution path by accident; pnpm's isolated store only links a package's declared deps into its scope, so under pnpm-strict / `pnpm dlx` / `pnpx` the import dies with ERR_MODULE_NOT_FOUND before `analyze --embeddings` can run. Declaring onnxruntime-common in gitnexus' own deps (#2074) does not fix this under pnpm: Node resolves the bare specifier from transformers' module scope, not ours, and overrides/resolutions can only re-version an existing edge, never add the missing one (verified against a real `hoist=false` install — the declaration only changes which version wins the hoist, never whether the import resolves). Fix: install a synchronous, in-thread ESM resolution hook (`module.registerHooks`) right before the lazy transformers import that redirects `onnxruntime-common` to the copy gitnexus depends on — but only when the default resolver fails. On npm / hoisted layouts the default resolver succeeds first and the hook never fires, so working setups are unchanged. The hook only intercepts the exact `onnxruntime-common` specifier on failure, so it can never mask an unrelated resolution error; onnxruntime-node's native binding still loads normally from transformers' own scope. `registerHooks` (sync, in-thread, single inline closure) is preferred over the older `module.register` (async, off-thread, now deprecated — DEP0205, removed in Node 26): the redirect is a one-line conditional that needs no worker thread, no separate hook module, and no `data` marshalling. It is available on Node >= 22.15; on older runtimes the helper is a graceful no-op (the gitnexus engines floor is >= 22.0.0, and the import still resolves on hoisted layouts there). Chosen over bundling transformers (the build is tsc-only, and transformers carries native onnxruntime-node + WASM onnxruntime-web assets that bundle poorly). Installation is idempotent, best-effort, and lazy — only on the local-embedding path, so it never affects analysis, the parse workers, or HTTP embedding mode. Validated end-to-end: the compiled resolver fixes a real pnpm `hoist=false` transformers install (ERR_MODULE_NOT_FOUND -> resolved). The separate `@ladybugdb/core` native-binary path under pure `pnpm dlx` is unchanged (#1967 handles that gracefully). Refs #307, #2069 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(embeddings): version-match the onnxruntime-common redirect target (#307) Prefer the onnxruntime-common that onnxruntime-node (the native binding transformers actually loads) depends on, so the redirected copy is version- matched to that binding even under `pnpm dlx` — where gitnexus' npm-style `overrides` block does not apply, because it is honoured only from a root manifest and gitnexus is a transitive dependency there. The walk resolves transformers' main entry (not its `exports`-blocked package.json) -> onnxruntime-node -> its onnxruntime-common, and falls back to gitnexus' own direct dependency when the chain can't be walked. Also corrects the doc comment that claimed the gitnexus copy was already "version-aligned". Addresses a PR #2139 tri-review finding (P2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(embeddings): narrow the onnxruntime-common resolve fallback to absence errors (#307) The resolve closure's `catch` swallowed every error from `nextResolve` and redirected, which would silently paper over a genuinely present-but-broken onnxruntime-common install. Only substitute gitnexus' copy when the specifier is actually absent (ERR_MODULE_NOT_FOUND, or ERR_PACKAGE_PATH_NOT_EXPORTED for an exports-broken copy); rethrow anything else. Adds a test that an unrelated error code rethrows. Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(embeddings): cover the onnxruntime-common resolver best-effort swallow path (#307) The outer try/catch in ensureOnnxRuntimeCommonResolvable() was untested. A throwing registerHooks spy drives it; the call must not throw (initEmbedder does not guard the return, so a throw would break `analyze --embeddings`). The vitest quirk that surfaced an earlier attempt applies to throwing mock factories, not a throwing spy implementation, so this is testable cleanly. Addresses a PR #2139 tri-review finding (P2). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(embeddings): tighten the onnxruntime-common redirect-URL assertion (#307) `/^file:\/\/.*onnxruntime-common/` matched the substring anywhere, so a lookalike path (e.g. `/x/onnxruntime-common-fake/`) would pass. Require an actual `/node_modules/onnxruntime-common/...js` segment so the assertion proves the redirect resolves to the real package, not just a string match. Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * refactor(embeddings): drop the no-op __resetOnnxRuntimeCommonResolverForTests seam (#307) The test helper reloads the resolver via vi.resetModules() + a fresh import(), which already re-initialises the module-level one-shot `attempted` flag to false. The __reset export it then called was therefore a no-op. Remove the test-only export and its call; isolation now rests solely on vi.resetModules(). Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(embeddings): correct the onnxruntime-common resolver isolation comment (#307) The doc comment claimed the hook "never affects other tools' resolution". Once installed, `module.registerHooks` is process-global and its resolve closure runs for every subsequent resolution — it passes them all through untouched and only substitutes the exact `onnxruntime-common` specifier on genuine absence, at a cost of one string comparison. Also note `registerHooks` is @experimental and requires Node >= 22.15 (graceful no-op below that). Comment-only. Addresses a PR #2139 tri-review finding (P3). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-06-10 12:00:43 +01:00