-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix: handle checking model gated #6316
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.
Caution
Changes requested ❌
Reviewed everything up to 85b614f in 2 minutes and 26 seconds. Click for details.
- Reviewed
82lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4draft 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/containers/ModelInfoHoverCard.tsx:82
- Draft comment:
Good implementation for the new GREY status branch with clear messaging. Ensure the UI color classes are consistent with other status indicators. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. web-app/src/containers/ModelInfoHoverCard.tsx:121
- Draft comment:
Text adjustment for quantization label (Default Quantization) is clearer now. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. web-app/src/services/__tests__/models.test.ts:927
- Draft comment:
Test updated to expect 'GREY' when an error occurs; this aligns with the new error status. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
4. web-app/src/services/models.ts:611
- Draft comment:
Changing the catch block to return 'GREY' in case of errors correctly reflects the new status semantics. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_JFtLuDcfQ9L6jtqV
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
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 11a7bf7 in 48 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. web-app/src/services/models.ts:600
- Draft comment:
Removed console.log used for debugging. This cleanup helps avoid polluting production logs. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
Workflow ID: wflow_kAcqcDbe4D36lJct
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.
Important
Looks good to me! 👍
Reviewed 5257a24 in 1 minute and 32 seconds. Click for details.
- Reviewed
22lines 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. web-app/src/containers/ModelSupportStatus.tsx:88
- Draft comment:
On error, consider returning 'GREY' (not null) so that compatibility errors are visibly indicated per the new logic. - 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/ModelSupportStatus.tsx:105
- Draft comment:
Add an explicit case for 'GREY' in getStatusColor to assign a proper grey color instead of falling to the default. - 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/containers/ModelSupportStatus.tsx:121
- Draft comment:
Include an explicit 'GREY' case in getStatusTooltip with a clear message (e.g., 'Compatibility unknown') instead of defaulting to 'Unknown'. - 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.
Workflow ID: wflow_mnaVZsguGM9Cx2Fr
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.
Important
Looks good to me! 👍
Reviewed 328d680 in 1 minute and 40 seconds. Click for details.
- Reviewed
13lines 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. web-app/src/routes/hub/$modelId.tsx:66
- Draft comment:
Union type updated: 'GREY' is now included in modelSupportStatus. Ensure that error handling (e.g., in checkModelSupport's catch block) is updated to assign 'GREY' for indeterminate compatibility errors to stay consistent. - 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 that error handling "should" be updated to use GREY, but provides no evidence that this is actually needed. The existing error handling using RED seems intentional. The comment is asking the author to "ensure" something rather than pointing out a clear issue. This violates the rule about not asking authors to verify things. Maybe GREY is intended to be used for error cases and RED is being used incorrectly? Maybe there's context in other files about when GREY should be used? Without clear evidence that GREY should be used for errors, this is just speculation. The existing code explicitly chooses to use RED for errors, and we should assume that's intentional unless proven otherwise. Delete this comment. It's speculative and asks the author to verify something rather than pointing out a clear issue that needs fixing.
Workflow ID: wflow_CCtZ2dxMAn3e26D0
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.

Describe Your Changes
This pull request updates how model compatibility errors are handled and displayed in the application. The main change is introducing a new
'GREY'status to indicate when the system cannot determine model compatibility, instead of treating errors as unsupported ('RED'). This status is now reflected both in the backend logic and the frontend UI, and the related tests have been updated accordingly.Error handling and status logic:
isModelSupportedfunction now returns'GREY'when an error occurs during compatibility checking, instead of'RED'. The type signature is updated to include'GREY'. [1] [2]models.test.tsare updated to expect'GREY'for error cases, ensuring consistency with the new status. [1] [2]Frontend UI updates:
ModelInfoHoverCardcomponent now displays a message and a grey indicator when the status is'GREY', informing the user that compatibility could not be determined.Minor UI text adjustment:
ModelInfoHoverCardis clarified to show'Default Quantization'for the default variant, improving user understanding.Fixes Issues
Self Checklist
Important
Introduce 'GREY' status for model compatibility errors, updating backend logic, frontend UI, and tests.
isModelSupportednow returns'GREY'for errors, not'RED', inmodels.ts.models.test.tsupdated to expect'GREY'for error cases.ModelInfoHoverCardshows a grey indicator and message for'GREY'status.'Default Quantization'inModelInfoHoverCard.This description was created by
for 328d680. You can customize this summary. It will automatically update as commits are pushed.