-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: download llamacpp backend fail back to cdn #6361
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.
Caution
Changes requested ❌
Reviewed everything up to bbcc8f3 in 2 minutes and 18 seconds. Click for details.
- Reviewed
134lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft 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/backend.ts:80
- Draft comment:
Using _fetchReleasesWithFallback now returns a 'source' along with releases. Consider logging or utilizing this 'source' info for debugging purposes. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
2. extensions/llamacpp-extension/src/backend.ts:147
- Draft comment:
New 'source' parameter is added. Consider ensuring that any partially downloaded files from a failed attempt are cleaned up before retrying with the fallback. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
3. extensions/llamacpp-extension/src/backend.ts:166
- Draft comment:
The URL construction for backend and CUDA assets is repeated. Consider refactoring this into a helper to reduce duplication and potential inconsistencies. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. extensions/llamacpp-extension/src/backend.ts:239
- Draft comment:
Fallback in the catch block triggers on any error. It might be safer to restrict fallback to transient network errors to avoid masking non-recoverable issues. - 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% The comment raises a valid point about error handling granularity. However, the current behavior of falling back on any GitHub error is likely intentional as a reliability feature. The CDN is meant to be a complete mirror of GitHub, so any error (network, auth, rate limit, etc.) can reasonably trigger the fallback. Making the fallback more restrictive could reduce reliability without clear benefits. The comment identifies a real architectural consideration about error handling granularity. More specific error handling could help diagnose issues better. While more granular error handling could be nice, the current broad fallback behavior is a valid and likely intentional design choice for reliability. The comment doesn't demonstrate a clear problem with the current approach. The comment should be deleted as it suggests a change that could reduce reliability without demonstrating clear benefits or problems with the current approach.
5. extensions/llamacpp-extension/src/backend.ts:306
- Draft comment:
The CDN releases URL is hardcoded. Ensure that this endpoint remains valid for all repository contexts and mirrors the GitHub releases correctly. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_odTerl3shzeZR5Wz
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 35.8%Your code coverage diff: 0.00% ▴ ✅ All code changes are covered |
651f907 to
362481f
Compare
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 651f907 in 1 minute and 36 seconds. Click for details.
- Reviewed
31lines of code in1files - 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/backend.ts:80
- Draft comment:
Changed release fetching to _fetchGithubReleases. Verify that relying solely on the GitHub API (with CDN fallback in its catch) meets all needs, and update docs if necessary. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to verify that the change meets all needs and to update documentation if necessary. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. It doesn't provide a specific code suggestion or point out a specific issue.
2. extensions/llamacpp-extension/src/backend.ts:156
- Draft comment:
Removed the explicit cast (window as any). Ensure that window.core.extensionManager is always defined to avoid runtime errors. - 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 about a type safety change, but it's making a speculative suggestion about runtime behavior. The removal of the type cast suggests the types are now properly defined somewhere. The comment is asking for verification/insurance which violates our rules. It's not providing clear evidence of an actual issue. Maybe there really could be runtime errors if the extension manager is undefined? Maybe the type cast was there for a reason? Without seeing the type definitions or having strong evidence of actual runtime issues, this is speculative. The removal of the type cast suggests the types are now properly handled. This comment should be deleted as it's asking for verification without clear evidence of an actual issue, and is speculative about potential runtime problems.
3. extensions/llamacpp-extension/src/backend.ts:295
- Draft comment:
Renamed function to _fetchGithubReleases. Confirm that the JSDoc and all references align with the new naming convention. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the PR author to confirm that the JSDoc and all references align with the new naming convention. This falls under asking the author to double-check things, which is not allowed according to the rules.
Workflow ID: wflow_7x2Ar6NuFFbPLzmx
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
This pull request adds a fallback mechanism to the backend download process in
llamacpp-extension. Now, if downloading from GitHub fails (due to rate limits or outages), the system will automatically retry using a CDN mirror. Additionally, the release fetching logic has been updated to prefer GitHub but gracefully fall back to the CDN if needed. This improves reliability for users in restrictive or unreliable network environments.Backend download improvements:
sourceparameter todownloadBackendand updated download URLs to support both GitHub and CDN sources, allowing downloads to switch between sources as needed. [1] [2] [3]Release fetching enhancements:
_fetchGithubReleaseswith_fetchReleasesWithFallback, which first tries GitHub and falls back to the CDN for release metadata, ensuring the latest releases are always accessible. [1] [2]Related issue
Important
Adds fallback to CDN for backend downloads in
llamacpp-extensionif GitHub fails, enhancing reliability.sourceparameter todownloadBackendinbackend.tsto support GitHub and CDN sources._fetchGithubReleaseswith_fetchReleasesWithFallbackinbackend.tsto try GitHub first, then CDN.This description was created by
for 651f907. You can customize this summary. It will automatically update as commits are pushed.