-
Notifications
You must be signed in to change notification settings - Fork 462
chore: Improve models screen [PART 1] #5441
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
…et info when grouped by architecture
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.
Pull request overview
This PR enhances the models screen by improving how failed models are displayed and handled, adding contextual information based on grouping mode, and improving the user experience when canceling training jobs.
Changes:
- Failed models now display a "Failed" badge and have restricted actions (only Delete available)
- Models display dataset information when grouped by architecture, and architecture information when grouped by dataset
- Training job cancellation now requires confirmation via an alert dialog
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
utils/utils.ts |
Adds helper function to identify failed models |
utils/date-formatting.ts |
Adds date formatting function for dataset revisions |
model-listing-provider.tsx |
Exposes dataset revisions to consuming components |
model-listing.container.tsx |
Passes grouping mode and dataset revisions to training component |
training-model-row.component.tsx |
Implements confirmation dialog for cancel action and conditional column rendering |
training-model-row.component.test.tsx |
Updates tests for new props and grouping scenarios |
current-model-training.module.scss |
Removes unused tag styles |
current-model-training.component.tsx |
Accepts and forwards grouping props |
model-row.module.scss |
Adds styles for dataset information and badges |
model-row.container.tsx |
Retrieves and passes dataset revision data |
model-row.component.tsx |
Displays failed badge and conditional columns based on grouping |
model-row.component.test.tsx |
Adds test coverage for new grouping and failed model scenarios |
model-badge.component.tsx |
New reusable badge component for model metadata |
dataset-revision-column.component.tsx |
New component displaying dataset revision details |
architecture-column.component.tsx |
New component displaying architecture information |
model-actions.component.tsx |
Disables actions for failed models |
group-models-container.component.tsx |
Disables expansion for failed models |
group-headers.module.scss |
Removes unused tag styles |
dataset-group-header.component.tsx |
Uses new ModelBadge component |
labels.component.tsx |
Replaces React fragment shorthand with explicit Fragment component |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...on/ui/src/features/models/model-listing/components/model-actions/model-actions.component.tsx
Outdated
Show resolved
Hide resolved
📊 Test coverage report
|
Docker Image SizesCPU
GPU
XPU
|
| import { ReactComponent as ThumbsUp } from '../../../../../assets/icons/thumbs-up.svg'; | ||
| import { ModelBadge } from './model-badge.component'; | ||
|
|
||
| import styles from './model-row.module.scss'; |
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.
we use classes let's keep it consistent
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.
Updated
|
|
||
| export const ArchitectureColumn = ({ architecture }: ArchitectureColumnProps) => { | ||
| return ( | ||
| <Flex alignItems={'start'} direction={'column'} gap={'size-100'}> |
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.
'start' is the default, not needed here
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.
copy/pasted but can update it
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.
Updated
application/ui/src/features/models/model-listing/components/model-row/model-badge.component.tsx
Show resolved
Hide resolved
|
|
||
| export const ModelRowContainer = ({ model }: ModelRowContainerProps) => { | ||
| const { activeModelArchitectureId, onExpandModel } = useModelListing(); | ||
| const { activeModelArchitectureId, onExpandModel, groupBy, datasetRevisions } = useModelListing(); |
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.
we can get datasetRevisions from useGetDatasetRevisions. Let's not add even more stuff to modelListing
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.
it's already fetched so why not reuse it?
application/ui/src/features/models/model-listing/components/model-row/model-row.container.tsx
Show resolved
Hide resolved
| {/* TODO: Speed is hardcoded for now, once the backend is update we need to update this */} | ||
| <Tag prefix={<ThumbsUp />} text={'Speed'} className={styles.recommendedForTag} /> | ||
| </Flex> | ||
| {groupBy === 'architecture' ? ( |
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.
perhaps we can have a EntityColumn component that gets datasetrevisions and groupBy (already in the tree) and renders here. WDYT?
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.
do u mean a wrapper that renders either architecture or dataset? (entity name is too general, could u propose one?)
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.
More context: I didn't create separate component that would use useModelListing to get groupBy and datasetRevisions because I didn't want to mock useModelListing in unit tests. I decided to pass it down so it's easier to test component.
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.
I dont mind having the conditional even 10 times, my main concern is adding yet more stuff to the already convoluted modelListing (my fault there). So maybe we can meet in the middle and at least have a hook to get these 2 things? wdyt? About the naming....i have no clue :D
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.
Sorry I didn't follow, what hook?
ActiveChooN
left a comment
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.
.../models/model-listing/components/group-models-container/group-models-container.component.tsx
Show resolved
Hide resolved
| return ( | ||
| <Badge variant={'neutral'} UNSAFE_className={styles.modelBadge} data-testid={id}> | ||
| <Text> | ||
| <Flex alignItems={'center'} gap={'size-50'}> |
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.
Why do we need these inside the badge? I thought it should support an icon and text out of the box.
https://react-spectrum.adobe.com/v3/Badge.html#content
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.
We don't use spectrum icons and this causes icon scaling issues (icon is too small)
| {isFailedModel(model) && ( | ||
| <> | ||
| {' '} | ||
| <FailedModel /> |
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.
If we already have Flex with gap, can this badge be moved outside of the <Text> tag?
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.




Summary
This PR includes:
Deleteoption is available for failed models.Close #5410
How to test
Checklist