-
Notifications
You must be signed in to change notification settings - Fork 2.2k
feat: Add support for custom environmental variables to llama.cpp #6256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
This commit adds a new setting `llamacpp_env` to the llama.cpp extension, allowing users to specify custom environment variables. These variables are passed to the backend process when it starts. A new function `parseEnvFromString` is introduced to handle the parsing of the semicolon-separated key-value pairs from the user input. The environment variables are then used in the `load` function and when listing available devices. This enables more flexible configuration of the llama.cpp backend, such as specifying visible GPUs for Vulkan. This change also updates the Tauri command `get_devices` to accept environment variables, ensuring that device discovery respects the user's settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Important
Looks good to me! 👍
Reviewed everything up to 046fe51 in 1 minute and 59 seconds. Click for details.
- Reviewed
158
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
6
draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. extensions/llamacpp-extension/settings.json:14
- Draft comment:
Consider using an empty string as the default value for 'llamacpp_env' instead of 'none'. This avoids potential confusion when parsing and makes the intent clearer. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
2. extensions/llamacpp-extension/src/index.ts:1150
- Draft comment:
The parseEnvFromString function skips any key starting with 'LLAMA'. This broad exclusion may inadvertently block legitimate custom environment variables. Consider whitelisting only sensitive keys (e.g. 'LLAMA_API_KEY') instead. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
3. extensions/llamacpp-extension/src/index.ts:1145
- Draft comment:
Consider adding validation or logging for malformed environment variable pairs. Currently, if an input segment lacks an '=' the pair is silently ignored. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
4. src-tauri/plugins/tauri-plugin-llamacpp/src/commands.rs:52
- Draft comment:
Logging the full command arguments may reveal sensitive details. Consider filtering or redacting sensitive environment variable values before logging. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
5. src-tauri/plugins/tauri-plugin-llamacpp/src/device.rs:32
- Draft comment:
The environment variables from user settings are passed directly to the child command. Ensure that this user-controlled input is properly documented and, if necessary, sanitized to prevent unintended side effects. - Reason this comment was not posted:
Confidence changes required:33%
<= threshold50%
None
6. extensions/llamacpp-extension/settings.json:16
- Draft comment:
Typo: Consider adding a space between 'llama.cpp' and the parenthesis for clarity. Currently it reads "llama.cpp(KEY=VALUE)"; it might be clearer as "llama.cpp (KEY=VALUE)". - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the comment is about changed code (new environment variables setting), it's an extremely minor formatting suggestion that doesn't impact functionality or code quality. The current format is perfectly readable and follows common technical documentation patterns where parenthetical clarifications often don't have spaces. This feels like unnecessary nitpicking. The space might make it marginally more readable for some users. Also, there might be some style guide I'm not aware of that recommends spaces before parentheses in documentation. Even if slightly more readable, this is too minor of a change to warrant a PR comment. Documentation formatting at this level of detail should be handled by style guides, not individual PR comments. Delete this comment as it's too minor and doesn't meaningfully improve code quality or functionality.
Workflow ID: wflow_0RilqR1xQoFzSSzP
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 37.1%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it!
Describe Your Changes
This commit adds a new setting
llamacpp_env
to the llama.cpp extension, allowing users to specify custom environment variables. These variables are passed to the backend process when it starts.A new function
parseEnvFromString
is introduced to handle the parsing of the semicolon-separated key-value pairs from the user input. The environment variables are then used in theload
function and when listing available devices. This enables more flexible configuration of the llama.cpp backend, such as specifying visible GPUs for Vulkan.This change also updates the Tauri command
get_devices
to accept environment variables, ensuring that device discovery respects the user's settings.Self Checklist
Important
Adds support for custom environment variables in llama.cpp, enhancing backend configuration flexibility.
llamacpp_env
setting insettings.json
for custom environment variables, parsed byparseEnvFromString()
inindex.ts
.load()
andgetDevices()
inindex.ts
for backend configuration.get_devices
incommands.rs
to accept and use environment variables.get_devices_from_backend()
indevice.rs
to utilize environment variables for device discovery.This description was created by
for 046fe51. You can customize this summary. It will automatically update as commits are pushed.