Skip to content

Conversation

@Thomas101
Copy link

testBindingBinary would throw when using a node-llama-cpp in a SEA binary. Add an exclusion to getShouldTestBinaryBeforeLoading to check for this and skip the test.

Description of change

As requested bringing this into the main repo as PR. Not sure if this is the right way to address this, but it fixed it when we were trying to bundle node-llama-cpp into a SEA binary.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run format to apply eslint formatting
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000 N/A
  • There are new or updated unit tests validating the change N/A
  • Documentation has been updated to reflect this change N/A
  • The new commits and pull request title follow conventions explained in pull request guidelines (PRs that do not follow this convention will not be merged)

testBindingBinary would throw when using a node-llama-cpp in a SEA binary. Add an exclusion to
getShouldTestBinaryBeforeLoading to check for this and skip the test.
@giladgd
Copy link
Member

giladgd commented May 21, 2025

@Thomas101 What error do you get from the testBindingBinary function?
It'd be best if we can fix the testBindingBinary to work with node SEA in your use case than to skip the test, as not testing the binary prior to loading it can cause the process to crash.

It'd be best if you can provide me with a simple reproduction code so I can investigate it.

I've recently came across a similar issue with node SEA packaged as a snap package (unrelated to node-llama-cpp) where it couldn't spawn another node child process due to an AppArmor profile, so I've added a fix that skips the binary test when running inside node SEA on Linux when snap is detected (via process.env.SNAP != null).
As a temporary workaround, you can use this code path to skip the binary test when calling getLlama on Linux via something like this:

const insideSnap = process.env.SNAP != null;
if (!insideSnap)
    process.env.SNAP = "true";

const llama = await getLlama();

if (!insideSnap)
    delete process.env.SNAP;

I'll close this PR for now since this is not the right fix for the issue, but feel free to reopen it or create a new one with the fix for the testBindingBinary function if you found the issue.
Let me know if you can create a simple reproduction for this that I can use to investigate, or whether the above code snippet helped you workaround it for now.

@giladgd giladgd closed this May 21, 2025
Thomas101 added a commit to axonzeta/node-llama-cpp that referenced this pull request May 22, 2025
After re-testing the binary on all 3 platforms, it looks like this
isn't a problem anymore. Remove our hacky patch

See withcatai#466
@Thomas101
Copy link
Author

To be honest, we added this hack sometime late last year when we were trying to ship the first version of AiBrow in a hurry and it just kind of stayed there after that. It looks like it isn't an issue now. I've done a quick test across win, linux & mac and it doesn't seem to crash, so I'll run a for-real build of AiBrow and get that tested. Thanks!

@Thomas101 Thomas101 deleted the add_sea_check branch May 22, 2025 11:09
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.

2 participants