fix(embeddings): retry install with --ignore-scripts on native build failure#249
Open
efenocchi wants to merge 3 commits into
Open
fix(embeddings): retry install with --ignore-scripts on native build failure#249efenocchi wants to merge 3 commits into
efenocchi wants to merge 3 commits into
Conversation
…failure On Node v26 macOS, sharp has no prebuilt binary and fails to build from source without node-gyp. The first npm install attempt is kept as-is so platforms with prebuilt binaries work normally. On failure we retry with --ignore-scripts (sharp is image-only; text embeddings are unaffected), then explicitly re-run onnxruntime-node's postinstall so its binary is still downloaded.
📝 WalkthroughWalkthrough
ChangesEmbedding dependency installation resilience
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Coverage ReportScope: files changed in this PR. Enforced threshold: 90% per metric (per file via
File Coverage — 1 file changed
Generated for commit bdc8c24. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/cli/embeddings.ts (2)
128-134: ⚡ Quick winConsider adding error handling for the ONNX runtime install script.
If the ONNX runtime's
install.jsscript fails to execute, the user won't get a clear indication of the failure, and text embeddings will silently break (since ONNX is required for the transformers library to function).Wrapping this in try/catch with a warning would help users understand when the fallback path has succeeded vs. when it has partially failed.
🛡️ Suggested error handling
const onnxScript = join(SHARED_NODE_MODULES, "onnxruntime-node", "script", "install.js"); if (existsSync(onnxScript)) { - execFileSync("node", [onnxScript], { - cwd: join(SHARED_NODE_MODULES, "onnxruntime-node"), - stdio: "inherit", - }); + try { + execFileSync("node", [onnxScript], { + cwd: join(SHARED_NODE_MODULES, "onnxruntime-node"), + stdio: "inherit", + }); + } catch (onnxErr) { + warn(` Embeddings onnxruntime-node install script failed — embeddings may not work`); + throw onnxErr; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/embeddings.ts` around lines 128 - 134, Wrap the execFileSync call that runs the ONNX install script (the block using onnxScript, existsSync, execFileSync and SHARED_NODE_MODULES) in a try/catch; if execFileSync throws, catch the error and log a clear warning/error (e.g., using processLogger or console.warn/error) stating the ONNX install failed and include the error message so users know embeddings may not work, but continue execution (don’t rethrow) to allow fallback behavior.
119-135: ⚖️ Poor tradeoffThe catch block is too broad and may produce misleading diagnostics.
The catch block assumes any install failure is due to native build issues (sharp requiring node-gyp), but failures can also occur due to network issues, disk space, permissions, or npm registry problems. In those cases, retrying with
--ignore-scriptswon't help, and the warning "sharp image support unavailable" will be misleading.Consider either:
- Checking the error message/code to distinguish native build failures from other errors
- Making the warning messages more generic (e.g., "install failed; retrying without build scripts")
- Logging the original error to help users diagnose non-native issues
💡 Example: Log original error for debugging
- } catch { + } catch (err) { // sharp has no prebuilt binary for some Node versions (e.g. Node v26 on macOS) // and requires node-gyp to build from source. Retry without scripts — sharp is // only used for image processing; text embeddings are unaffected. - warn(` Embeddings native build failed; retrying without install scripts`); + warn(` Embeddings install failed (${err instanceof Error ? err.message : String(err)}); retrying without install scripts`); warn(` Embeddings (sharp image support unavailable — text embeddings unaffected)`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/cli/embeddings.ts` around lines 119 - 135, Change the bare catch to catch the error (e.g., catch (err)) and log the original error via warn so users can see the underlying cause; then inspect err.message/err.code for native-build indicators (match keywords like "sharp", "node-gyp", "prebuild", "gyp") and only perform the --ignore-scripts retry and the sharp-specific warnings/onnx postinstall when those indicators are present; otherwise emit a generic failure warning (e.g., "install failed; not retrying without build scripts") and rethrow or exit to avoid masking non-native issues (update the block around execFileSync/npm retry, the onnxScript logic, and the warn calls to reference err).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/embeddings.ts`:
- Line 125: The retry execFileSync call that runs "npm" with [...npmArgs,
"--ignore-scripts"] in embeddings.ts needs a try/catch so failures produce a
clear error message instead of an unhandled exception; wrap the
execFileSync(...) call in try { ... } catch (err) { console.error or
processLogger.error with a descriptive message including err.message (and the
npmArgs/SHARED_DIR context), then call process.exit(1) } to ensure graceful
failure handling.
---
Nitpick comments:
In `@src/cli/embeddings.ts`:
- Around line 128-134: Wrap the execFileSync call that runs the ONNX install
script (the block using onnxScript, existsSync, execFileSync and
SHARED_NODE_MODULES) in a try/catch; if execFileSync throws, catch the error and
log a clear warning/error (e.g., using processLogger or console.warn/error)
stating the ONNX install failed and include the error message so users know
embeddings may not work, but continue execution (don’t rethrow) to allow
fallback behavior.
- Around line 119-135: Change the bare catch to catch the error (e.g., catch
(err)) and log the original error via warn so users can see the underlying
cause; then inspect err.message/err.code for native-build indicators (match
keywords like "sharp", "node-gyp", "prebuild", "gyp") and only perform the
--ignore-scripts retry and the sharp-specific warnings/onnx postinstall when
those indicators are present; otherwise emit a generic failure warning (e.g.,
"install failed; not retrying without build scripts") and rethrow or exit to
avoid masking non-native issues (update the block around execFileSync/npm retry,
the onnxScript logic, and the warn calls to reference err).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 123519d0-04f8-4455-9a31-8a1d3406da08
📒 Files selected for processing (1)
src/cli/embeddings.ts
embeddings/embed-daemon.js is built by esbuild but was missing from the files array, so it was never published to npm. hivemind embeddings install copies this file to ~/.hivemind/embed-deps/embed-daemon.js — without it the daemon is absent and no embeddings are generated.
…Rabbit fixes Extract install-with-fallback logic into exported _installWithFallback so it can be tested with an injected exec function. Tests cover all 5 branches: happy path, retry on native build failure, onnxruntime script re-run, onnxruntime script failure (warns, no throw), and hard-fail (throws). Also applies CodeRabbit feedback: log original error in catch, wrap retry execFileSync in its own try/catch with a descriptive throw, and wrap the onnxruntime script execution in try/catch with a user-visible warning.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli/cli-embeddings.test.ts (1)
353-355: ⚡ Quick winUse exact path/message assertions in fallback tests.
These checks currently accept generic substrings, so they can pass on incorrect values. Please assert exact expected args/path/message for stronger contract coverage.
Suggested test-tightening diff
@@ it("retry path: first call fails → retries with --ignore-scripts", () => { @@ _installWithFallback("/fake/shared", "/fake/nm", exec); expect(exec).toHaveBeenCalledTimes(2); - const [, secondArgs] = exec.mock.calls[1] as [string, string[]]; - expect(secondArgs).toContain("--ignore-scripts"); + expect(exec).toHaveBeenNthCalledWith( + 2, + "npm", + ["install", "--omit=dev", "--no-package-lock", "--no-audit", "--no-fund", "--ignore-scripts"], + expect.objectContaining({ cwd: "/fake/shared" }), + ); }); @@ // npm install (fail) + npm install --ignore-scripts + node onnxScript expect(exec).toHaveBeenCalledTimes(3); - const [thirdCmd, thirdArgs] = exec.mock.calls[2] as [string, string[]]; - expect(thirdCmd).toBe("node"); - expect(thirdArgs[0]).toContain("onnxruntime-node"); - expect(thirdArgs[0]).toContain("install.js"); + const expectedOnnxScript = join(sharedNm, "onnxruntime-node", "script", "install.js"); + expect(exec).toHaveBeenNthCalledWith( + 3, + "node", + [expectedOnnxScript], + expect.objectContaining({ cwd: join(sharedNm, "onnxruntime-node") }), + ); }); @@ it("hard-fail path: throws when retry with --ignore-scripts also fails", () => { const exec = vi.fn(() => { throw new Error("disk full"); }) as any; expect(() => _installWithFallback("/fake/shared", "/fake/nm", exec)) - .toThrow("Embedding deps install failed even without scripts"); + .toThrowError(/^Embedding deps install failed even without scripts: disk full$/); });As per coding guidelines,
tests/**: Prefer asserting on specific values (paths, messages) over generic substrings.Also applies to: 372-375, 394-395
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/cli/cli-embeddings.test.ts` around lines 353 - 355, Replace loose substring assertions like expect(secondArgs).toContain("--ignore-scripts") with exact-value assertions that compare the entire args array or the exact argument string; locate the uses around exec.mock.calls (e.g., the destructuring into secondArgs) and change them to assert equality (e.g., expect(secondArgs).toEqual([...expected exact args...]) or expect(secondArgs).toContainEqual("--ignore-scripts") if matching a single exact element), and apply the same tightening for the other occurrences mentioned (lines with expects at 372-375 and 394-395) so tests verify exact paths/messages instead of generic substrings.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/cli/cli-embeddings.test.ts`:
- Around line 353-355: Replace loose substring assertions like
expect(secondArgs).toContain("--ignore-scripts") with exact-value assertions
that compare the entire args array or the exact argument string; locate the uses
around exec.mock.calls (e.g., the destructuring into secondArgs) and change them
to assert equality (e.g., expect(secondArgs).toEqual([...expected exact
args...]) or expect(secondArgs).toContainEqual("--ignore-scripts") if matching a
single exact element), and apply the same tightening for the other occurrences
mentioned (lines with expects at 372-375 and 394-395) so tests verify exact
paths/messages instead of generic substrings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: fc6dcccd-d871-4899-911e-9440eb6738e2
📒 Files selected for processing (3)
package.jsonsrc/cli/embeddings.tstests/cli/cli-embeddings.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cli/embeddings.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sharphas no prebuilt binary and fails to build from source withoutnode-gyp, causinghivemind embeddings installto abortnpm installinensureSharedDeps()in a try/catch: on failure, retries with--ignore-scriptsto skip sharp's build--ignore-scriptsretry, explicitly re-runsonnxruntime-node's postinstall script so its prebuilt binary is still downloadedsharpis image-only in@huggingface/transformers; text embeddings are unaffectedRelated
Follows up on #248 (moved
@huggingface/transformerstooptionalDependencies).Test plan
hivemind embeddings installsucceeds on Node v26 macOS withoutnode-gypinstalledSummary by CodeRabbit
Bug Fixes
Chores
Tests