-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: migrate cortex models to llamacpp extension #5838
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
feat: migrate cortex models to llamacpp extension #5838
Conversation
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.
Caution
Changes requested ❌
Reviewed everything up to 162205a in 3 minutes and 12 seconds. Click for details.
- Reviewed
1000lines of code in8files - Skipped
0files when reviewing. - Skipped posting
3draft 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:560
- Draft comment:
Use the full child path (childPath) instead of just 'child' when pushing into the DFS stack. This ensures proper directory traversal for subsequent fs operations. - 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. extensions/llamacpp-extension/src/index.ts:546
- Draft comment:
Review the modelId extraction using slice() – it can yield an empty string if the currentDir exactly matches the models base path. Consider a more robust method to generate non-empty model IDs. - 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.
3. extensions/llamacpp-extension/src/util.ts:65
- Draft comment:
Be cautious when logging proxy configuration details. Although only a boolean flag for authentication is logged, ensure that sensitive details (like credentials) are not inadvertently exposed in production logs. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is advising caution about logging sensitive information, which is a valid concern in software development. However, it is phrased in a way that asks the author to ensure something, which violates the rule against asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.
Workflow ID: wflow_IIoMKOjRuxdX597J
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 35.15%Your code coverage diff: -0.01% ▾ ✅ 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.
Important
Looks good to me! 👍
Reviewed 876d3b6 in 53 seconds. Click for details.
- Reviewed
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1draft 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:30
- Draft comment:
Remove duplicate import of 'basename' from '@tauri-apps/api/path'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_3LzEb0Yt3t0xi9A1
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
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.
LGTM so far now
Describe Your Changes
This PR migrates cortex models to the new llama.cpp extension and adds comprehensive test coverage for the migration functionality.
Changes
Test Plan
Important
Migrates cortex models to llamacpp extension, updates conversational extension, and enhances test coverage.
migrateLegacyModels()inllamacpp-extension/src/index.tsto migrate cortex models to llamacpp format.list()inllamacpp-extension/src/index.tsto callmigrateLegacyModels().CortexConversationalExtensiontoJanConversationalExtensioninconversational-extension/src/index.ts.migrateLegacyModels.test.tsfor testing migration logic.backend.test.ts,index.test.ts, andutil.test.tsfor enhanced coverage.setup.tsfor consistent test environment.This description was created by
for 876d3b6. You can customize this summary. It will automatically update as commits are pushed.