-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat(llamacpp): support .zip archives for manual backend install #6534
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
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 500fbd8 in 1 minute and 13 seconds. Click for details.
- Reviewed
377lines of code in6files - Skipped
1files when reviewing. - Skipped posting
6draft 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. src-tauri/Cargo.toml:61
- Draft comment:
Ensure that the dependency 'zip = "0.6"' meets the feature requirements used in the decompress command. Consider verifying compatibility with your extraction logic. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. src-tauri/src/core/filesystem/commands.rs:199
- Draft comment:
The file extension check uses a case-sensitive ends_with. Consider normalizing the string (e.g. to lowercase) or using a more robust method (like Path::extension) to handle cases like '.ZIP' or mixed case. - 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. web-app/src/routes/settings/providers/$providerName.tsx:391
- Draft comment:
The inline comment states to replace spaces with dashes and convert to lowercase, but the code only replaces spaces. If lowercase conversion is desired, chain a toLowerCase() call for consistency. - 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.
4. web-app/src/routes/settings/providers/$providerName.tsx:787
- Draft comment:
When mapping over provider models, the key is set to the index. It's more stable to use a unique identifier like model.id if available. - 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.
5. src-tauri/src/core/filesystem/commands.rs:10
- Draft comment:
Multiple command functions take a Vec as arguments. Consider defining a dedicated parameter struct for better type safety and clearer intent. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
6. src-tauri/src/core/filesystem/commands.rs:131
- Draft comment:
There's a TODO comment about implementing proper scope checks in write_yaml. Consider adding a tracking issue or reminder to implement robust security checks when the FS API is removed from the frontend. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_35jc7Bsmg2AIezSJ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
Hi @github-roushan, Thanks for making the contribution. We'll review it and let you know when it's ready |
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.
Please remove all the lock file changes.
b3bda2e to
bb80104
Compare
lock files are pinning exact versions of transitive dependencies. Without them, two people doing yarn install at different times might get different dependency trees, and the app could behave differently or even break. |
@qnixsynapse . would you please review this again. I see lock files in both cargo and extensions have been pushed before. Infact removing the lock files are breaking the build. I believe lock file is needed as i added a new create zip and its actual version needs to be reflected. Even louis has pushed a PR with lock files around 5 days ago. |
|
Because of security issues, I don't really recommend adding lock file changes. Team member can do this because they monitor what's happening. Rest LGTM. |
|
can you merge this though @qnixsynapse |
|
You need to rebase and remove Cargo.lock file. That diff is huge. |
ff93b38 to
bd12fb8
Compare
65a3bb6 to
d5eba4d
Compare
Hey @qnixsynapse . I have rebased to the latest dev commit as of this comment. Also I have tested with removing .lock files. From my google search i understand that .lock files shouldn't be removed (or the build is failing, you can try, yourself) when you have added a new crate (zip 0.6) for zip functionality. as these needs to finds the right dependencies for the version of zip crate that was installed. Please go through actual Cargo.toml to see the changes that was done. |
* ✨ feat: Re-arrange docs as needed * 🔧 chore: re-arrange the folder structure * Add server docs Add server docs * enhancement: migrate handbook and janv2 * Update docs/src/components/ui/dropdown-button.tsx Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * Update docs/src/pages/_meta.json Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> * chore: update feedback #1 * fix: layout ability model * feat: add azure as first class provider (#6555) * feat: add azure as first class provider * fix: deployment url * Update handbook: restructure content and add new sections - Add betting-on-open-source.mdx and open-superintelligence.mdx - Update handbook index with new structure - Remove outdated handbook sections (growth, happy, history, money, talent, teams, users, why) - Update handbook _meta.json to reflect new structure * chore: fix meta data json * chore: update missing install * fix: Catch local API server various errors (#6548) * fix: Catch local API server various errors * chore: Add tests to cover error catches * fix: LocalAPI server trusted host should accept asterisk (#6551) * feat: support .zip archives for manual backend install (#6534) * feat(llamacpp): support .zip archives for manual backend install * Update Lock Files * Merge pull request #6563 from menloresearch/feat/web-minor-ui-tweak-login feat: tweak login UI --------- Co-authored-by: LazyYuuki <[email protected]> Co-authored-by: nngostuds <[email protected]> Co-authored-by: Faisal Amir <[email protected]> Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com> Co-authored-by: Louis <[email protected]> Co-authored-by: eckartal <[email protected]> Co-authored-by: Nghia Doan <[email protected]> Co-authored-by: Roushan Kumar Singh <[email protected]>
Describe Your Changes
zipcrateFixes Issues
Self Checklist
Important
Add support for .zip archives in llamacpp backend installation, updating UI and backend logic, and adding necessary dependencies.
$providerName.tsx.installBackendinindex.tsto accept .zip files.decompress()incommands.rsto handle .zip extraction.zipcrate toCargo.tomlandCargo.lockfor .zip support.This description was created by
for 500fbd8. You can customize this summary. It will automatically update as commits are pushed.