Skip to content

Conversation

@dwesolow
Copy link
Contributor

@dwesolow dwesolow commented Feb 5, 2026

Summary

This PR includes:

  1. Failed badge for failed models.
  2. Only Delete option is available for failed models.
  3. Display alert dialog before cancelling the training job.
  4. Display dataset information when grouped by architecture.
  5. Display architecture information when grouped by dataset.
image image image

Close #5410

How to test

Checklist

  • The PR title and description are clear and descriptive
  • I have manually tested the changes
  • All changes are covered by automated tests
  • All related issues are linked to this PR (if applicable)
  • Documentation has been updated (if applicable)

@dwesolow dwesolow self-assigned this Feb 5, 2026
@dwesolow dwesolow requested a review from a team as a code owner February 5, 2026 14:58
Copilot AI review requested due to automatic review settings February 5, 2026 14:58
@github-actions github-actions bot added the Geti Tune UI Issues related to Geti Tune UI label Feb 5, 2026
Copy link
Contributor

Copilot AI left a 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.

@github-actions
Copy link

github-actions bot commented Feb 5, 2026

📊 Test coverage report

Metric Coverage
Lines 35.1%
Functions 74.5%
Branches 87.9%
Statements 35.1%

@github-actions
Copy link

github-actions bot commented Feb 5, 2026

Docker Image Sizes

CPU

Image Size
geti-tune-cpu:pr-5441 2.96G
geti-tune-cpu:sha-01fe8ac 2.96G

GPU

Image Size
geti-tune-gpu:pr-5441 10.95G
geti-tune-gpu:sha-01fe8ac 10.95G

XPU

Image Size
geti-tune-xpu:pr-5441 9.76G
geti-tune-xpu:sha-01fe8ac 9.76G

import { ReactComponent as ThumbsUp } from '../../../../../assets/icons/thumbs-up.svg';
import { ModelBadge } from './model-badge.component';

import styles from './model-row.module.scss';
Copy link
Contributor

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

Copy link
Contributor Author

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'}>
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated


export const ModelRowContainer = ({ model }: ModelRowContainerProps) => {
const { activeModelArchitectureId, onExpandModel } = useModelListing();
const { activeModelArchitectureId, onExpandModel, groupBy, datasetRevisions } = useModelListing();
Copy link
Contributor

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

Copy link
Contributor Author

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?

{/* 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' ? (
Copy link
Contributor

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?

Copy link
Contributor Author

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?)

Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor

@ActiveChooN ActiveChooN left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The speed badge is the same color as the background when hovered over. It should probably be brighter.

Image

return (
<Badge variant={'neutral'} UNSAFE_className={styles.modelBadge} data-testid={id}>
<Text>
<Flex alignItems={'center'} gap={'size-50'}>
Copy link
Contributor

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

Copy link
Contributor Author

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 />
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Depends what you want to achieve: I wanted badge to be next to the model name (if it wraps to the new line, badge should wrap as well). If you want to have it next to the name, it could be moved.

image

vs

image

@dwesolow
Copy link
Contributor Author

dwesolow commented Feb 6, 2026

The speed badge is the same color as the background when hovered over. It should probably be brighter.

Image

I know, it's been like this in develop already, I only extracted to separate component. I have fixed that already in my next branch with follow-up model improvements.

@dwesolow dwesolow added this pull request to the merge queue Feb 6, 2026
Merged via the queue into develop with commit 8428cdb Feb 6, 2026
31 checks passed
@dwesolow dwesolow deleted the dwesolow/5410-failed-models branch February 6, 2026 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Geti Tune UI Issues related to Geti Tune UI

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Display failed models on model screen

4 participants