-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: migrations model setting #6165
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.
LGTM!
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 0fec439 in 1 minute and 30 seconds. Click for details.
- Reviewed
91lines 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. web-app/src/hooks/useModelProvider.ts:230
- Draft comment:
Improved: Direct in-place update with forEach is clearer & avoids extra object creation. Ensure provider.settings is always an array. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
2. web-app/src/hooks/useModelProvider.ts:248
- Draft comment:
The migration correctly renames 'chatTemplate' to 'chat_template'. Verify it won’t override any custom settings when both keys exist. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. web-app/src/hooks/useModelProvider.ts:255
- Draft comment:
Default initialization for missing chat_template is solid; ensure modelSettings.chatTemplate is reliably defined. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
4. web-app/src/hooks/useModelProvider.ts:264
- Draft comment:
Adds missing override_tensor_buffer_t default. Confirm that key naming and modelSettings.override_tensor_buffer_t are consistent throughout the app. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
5. web-app/src/hooks/useModelProvider.ts:280
- Draft comment:
Migration version reset to 1; ensure consolidating all migrations under version 0 is intentional and state from higher versions is handled appropriately. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold50%The comment is asking the author to ensure that consolidating migrations is intentional and that state from higher versions is handled. This is a request for confirmation and ensuring behavior, which violates the rules.
Workflow ID: wflow_3CExrjrYzrzDsSRL
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
Barecheck - Code coverage reportTotal: 28.69%Your code coverage diff: 0.03% ▴ |
Describe Your Changes
This pull request refactors the migration logic in the
useModelProviderhook, simplifying how provider and model settings are updated. The main changes involve streamlining the migration steps and reducing the migration version number, which may impact how state is upgraded in future releases.Migration logic simplification:
cont_batchingdescription onllamacppproviders to useforEachand direct mutation, instead of mapping and returning new objects. This reduces unnecessary object creation and improves clarity.chat_templateare initialized with default values, and that key renames (e.g.,chatTemplatetochat_template) are handled in a single loop.Migration version changes:
override_tensor_buffer_typesetting, and set the migration version back to 1. This affects how persisted state is upgraded and may require additional attention if older migrations are needed in the future. [1] [2]Fixes Issues
Self Checklist
Important
Simplifies migration logic in
useModelProvider.tsand resets migration version to 1, removing older migrations.useModelProvider.tsby usingforEachforcont_batchingdescription update, reducing object creation.chat_templatewith defaults and renaming keys in a single loop.override_tensor_buffer_typesetting.This description was created by
for 0fec439. You can customize this summary. It will automatically update as commits are pushed.