fix: support load model configurations

[urmauur]

Describe Your Changes

This pull request introduces enhancements to the AI model configuration and loading process, focusing on improved flexibility and support for additional model settings. Key changes include updates to the AIEngine class, expanded configuration options in the llamacpp_extension, and adjustments to the web application to handle new settings.

Core Enhancements to Model Loading

• core/src/browser/extensions/engines/AIEngine.ts: Updated the load method to accept an optional settings parameter, enabling dynamic configuration of models during loading.

Expanded Model Configuration Options

• extensions/llamacpp-extension/src/index.ts: Added new settings to LlamacppConfig, such as temp, top_k, top_p, min_p, repeat_last_n, repeat_penalty, presence_penalty, and frequency_penalty. These settings allow for fine-tuning of model behavior.
• extensions/llamacpp-extension/src/index.ts: Enhanced the llamacpp_extension class to pass the new settings as command-line arguments when initializing the model.

Web Application Integration

• web-app/src/lib/predefined.ts: Updated modelSettings with key mappings for the new configuration options, aligning the web app's predefined settings with the expanded capabilities of the models. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10]
• web-app/src/services/models.ts: Implemented a key mapping function to transform web app settings into model-compatible keys and dynamically pass these settings to the engine.load method during model initialization.

Fixes Issues

• Closes bug: llama.cpp extension only supports provider-level settings, missing per-model configuration in v0.6.6 #5818
• Closes #

Self Checklist

• Added relevant comments, esp in complex areas
• Updated docs (for bug fixes / features)
• Created issues for follow-up changes or refactoring needed

---

Important

Enhances AI model configuration by adding dynamic settings support to AIEngine and integrating expanded options into the web application.

• Behavior:
  • AIEngine.ts: Updated load() method to accept optional settings parameter for dynamic model configuration.
• Model Configuration:
  • index.ts: Added new settings to LlamacppConfig including temp, top_k, top_p, min_p, repeat_last_n, repeat_penalty, presence_penalty, and frequency_penalty.
  • index.ts: Enhanced llamacpp_extension to pass new settings as command-line arguments.
• Web Application:
  • ModelSetting.tsx: Updated modelSettings with new configuration options.
  • models.ts: Implemented key mapping function to transform web app settings into model-compatible keys and pass to engine.load().
• Testing:
  • models.test.ts: Added tests for startModel() to verify settings are correctly mapped and passed.

^{This description was created by for 7e4c2ba. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 50b3f78 in 2 minutes and 15 seconds. Click for details.

• Reviewed 191 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 2 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. core/src/browser/extensions/engines/AIEngine.ts:234

• Draft comment:
  The abstract load() signature now accepts an optional 'settings' parameter. Ensure that all subclass implementations remain backward‐compatible with calls that don’t provide settings.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure backward compatibility, which falls under the rule of not asking the author to ensure behavior is intended or tested. It doesn't provide a specific suggestion or point out a specific issue.

2. web-app/src/services/models.ts:157

• Draft comment:
  The key mapping function only maps 'ctx_len', 'temperature', and 'ngl'. If additional settings (like top_p, min_p, etc.) require renaming, consider extending this mapping.
• 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% The comment is speculative - it suggests what might be needed in the future rather than pointing out a current issue. The current mapping likely covers the known required mappings, and the function has a fallback to use the original key if no mapping exists. Without evidence that other keys need mapping, this is just speculation about potential future needs. The comment could be valuable if there are actually other known settings that need mapping. I might be missing context about the full set of possible settings. However, the code handles unmapped keys gracefully by returning the original key. If new mappings are needed, they can be added when that need arises. Making speculative suggestions about potential future needs isn't actionable now. The comment should be deleted as it's speculative and doesn't point out a current issue that needs fixing. The code handles unmapped keys appropriately.

Workflow ID: wflow_5hQ15Tz3fPJSFnLp

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 9f3f8e0 in 3 minutes and 16 seconds. Click for details.

• Reviewed 199 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 5 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/src/index.ts:845

• Draft comment:
  Removed command‐line argument blocks for settings (e.g. temp, top_k, top_p, etc.). Ensure these parameters are now handled via overrideSettings and that legacy configurations are not inadvertently broken.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. web-app/src/containers/ModelSetting.tsx:90

• Draft comment:
  Great improvement: stopModel is now only debounced when updating 'ctx_len' or 'ngl', which helps avoid unnecessary model reloads.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply praises the change without offering any specific guidance or questions.

3. web-app/src/hooks/useChat.ts:268

• Draft comment:
  Filtering out the keys 'ctx_len' and 'ngl' from model settings before merging them with assistant parameters is intentional. Please verify that this exclusion aligns with the intended per‐model configuration behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to verify their intention, which violates the rules. It does not provide a specific code suggestion or ask for a specific test to be written. Therefore, it should be removed.

4. web-app/src/services/__tests__/models.test.ts:121

• Draft comment:
  The test for updateModel casts the model to 'any'. Consider refining the model type in tests to better reflect the expected structure.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While using specific types is generally better than 'any', this is a test file where we often need flexibility with mock objects. The mock object only needs the specific properties being tested. Using Model type might force us to implement unnecessary properties. The test is already working and clear as is. The suggestion doesn't clearly improve code quality enough to warrant a change. The comment promotes TypeScript best practices by suggesting stronger typing. Type safety could catch potential issues during refactoring. In test files, using 'any' for mocks is a common and acceptable practice when we only care about specific properties. The benefit of stronger typing doesn't outweigh the added complexity in this case. Delete the comment. While the suggestion follows TypeScript best practices, using 'any' in test mocks is acceptable and the change wouldn't significantly improve code quality.

5. web-app/src/services/models.ts:158

• Draft comment:
  Mapping per‐model settings via mapSettingKey (e.g. 'ctx_len' to 'ctx_size', 'ngl' to 'n_gpu_layers') is a good approach. Note that, unlike in useChat.ts where these keys are filtered out, they are passed to engine.load. Confirm that this difference is intentional and document the rationale if needed.
• 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% The comment is making a cross-file comparison with useChat.ts which we can't verify. It's asking for confirmation of intention and documentation, which violates our rules. The comment isn't pointing out a clear issue that needs fixing, just asking for verification. The key mapping could potentially cause issues if the engine.load function expects different keys than what we're providing. Maybe there's a real compatibility concern here. Without access to useChat.ts or the engine.load implementation, we can't verify if there's actually an issue. The comment is speculative and asks for confirmation rather than pointing out a concrete problem. The comment should be deleted as it violates our rules by asking for confirmation of intention and making cross-file comparisons we can't verify.

Workflow ID: wflow_VLG5VoeEWdWpQcwH

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed bada7e3 in 2 minutes and 19 seconds. Click for details.

• Reviewed 15 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 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/src/index.ts:37

• Draft comment:
  Removed inline comments from 'n_gpu_layers' and 'ctx_size'. Consider adding proper doc comments if these notes are still useful.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. extensions/llamacpp-extension/src/index.ts:765

• Draft comment:
  The load() method signature is updated to accept an optional overrideSettings parameter. Merging the base config with overrides using the spread operator is a clear approach.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. extensions/llamacpp-extension/src/index.ts:829

• Draft comment:
  The 'ctx_size' option is pushed twice (once as '-c' and later as '--ctx-size'). Confirm if both flags are required or if this is redundant.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

4. extensions/llamacpp-extension/src/index.ts:850

• Draft comment:
  The '--no-mmap' flag is pushed unconditionally. It might be better to conditionally add it based on the config.no_mmap value.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

Workflow ID: wflow_xIS2YW6BdiNzJT8Z

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed b7353b2 in 1 minute and 16 seconds. Click for details.

• Reviewed 93 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 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. web-app/src/lib/predefined.ts:3

• Draft comment:
  Extraneous inline comments for alternate key names were removed (e.g., for 'ctx_len' and 'ngl'). Ensure that all references across the codebase now use the updated key names.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking the PR author to ensure that all references across the codebase use the updated key names. This falls under the rule of not asking the author to ensure something is done, which is not allowed.

2. web-app/src/lib/predefined.ts:33

• Draft comment:
  The 'temperature' field default value is an empty string while its placeholder is '0.6'. Verify that the form or downstream logic correctly handles type conversion for numeric inputs.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. web-app/src/lib/predefined.ts:8

• Draft comment:
  Note the inconsistency in default values: 'ctx_len' has a numeric default (8192), whereas many other numeric settings default to an empty string. Confirm that this discrepancy is intentional and is handled appropriately in the UI/model logic.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

Workflow ID: wflow_chxkY6JowggfNgXG

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 7e4c2ba in 1 minute and 20 seconds. Click for details.

• Reviewed 60 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 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. web-app/src/services/__tests__/models.test.ts:212

• Draft comment:
  Refactor: The mockSettings object is redefined in several tests. Consider extracting it into a shared constant to reduce duplication.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

2. web-app/src/services/__tests__/models.test.ts:231

• Draft comment:
  Ensure correct mapping: The test expects 'ctx_len' to be transformed to 'ctx_size' and 'ngl' to 'n_gpu_layers'. Verify that this mapping is consistent with the intended configuration logic.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50% None

3. web-app/src/services/__tests__/models.test.ts:267

• Draft comment:
  Good test for avoiding duplicate loads: The 'should not load model again' test checks that engine.load is not called when the model is already loaded.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_TfI04zVSe5HPKTCu

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[github-actions]

Barecheck - Code coverage report

Total: 35.06%

Your code coverage diff: 0.05% ▴

Uncovered files and lines

File	Lines
web-app/src/containers/ModelSetting.tsx	26-31, 34-36, 38-42, 45-57, 60, 62, 64, 67, 70-72, 74-82, 84-88, 91-95, 97-104, 106-142, 144-147, 149
web-app/src/hooks/useChat.ts	94-106, 120-126, 136-158, 161, 163, 165, 168, 171-176, 178-179, 184-186, 189-193, 196, 199-201, 203-211, 222, 252-253, 255, 269-280, 297-308, 310-317, 319-335, 338-353, 355-366, 368-373, 375-384, 386-390, 392-394, 397-403, 405-411, 428
web-app/src/services/models.ts	173

[qnixsynapse]

LGTM