Skip to content

Commit c710487

Browse files
committed
Address review comments and add TPS column corresponding percentile
Signed-off-by: manaswinidas <[email protected]>
1 parent 2a01bef commit c710487

File tree

3 files changed

+46
-44
lines changed

3 files changed

+46
-44
lines changed

clients/ui/frontend/src/__tests__/cypress/cypress/tests/mocked/modelCatalog/modelCatalogTabs.cy.ts

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ describe('Model Catalog Details Tabs', () => {
404404
});
405405

406406
describe('With Latency Filter Applied', () => {
407-
it('should show only the selected latency column when TTFT P90 filter is applied', () => {
407+
it('should show only the selected latency column and matching TPS column when TTFT P90 filter is applied', () => {
408408
modelCatalog.findModelCatalogDetailLink().first().click();
409409
modelCatalog.clickPerformanceInsightsTab();
410410

@@ -414,28 +414,22 @@ describe('Model Catalog Details Tabs', () => {
414414
// Apply the default TTFT P90 filter
415415
modelCatalog.clickApplyFilter();
416416

417-
// Only TTFT P90 column should be visible
417+
// TTFT P90 column should be visible
418418
modelCatalog
419419
.findHardwareConfigurationTableHeaders()
420420
.should('contain.text', `TTFT${NBSP}Latency P90`);
421421

422-
// Other TTFT latency columns should be hidden
422+
// TPS P90 column should be visible (matching percentile)
423423
modelCatalog
424424
.findHardwareConfigurationTableHeaders()
425-
.should('not.contain.text', 'Latency Mean');
426-
modelCatalog
427-
.findHardwareConfigurationTableHeaders()
428-
.should('not.contain.text', 'Latency P95');
429-
modelCatalog
430-
.findHardwareConfigurationTableHeaders()
431-
.should('not.contain.text', 'Latency P99');
425+
.should('contain.text', `TPS${NBSP}Latency P90`);
432426

433427
// E2E and ITL columns should be hidden
434428
modelCatalog.findHardwareConfigurationTableHeaders().should('not.contain.text', 'E2E');
435429
modelCatalog.findHardwareConfigurationTableHeaders().should('not.contain.text', 'ITL');
436430
});
437431

438-
it('should show only E2E mean column when E2E mean filter is applied', () => {
432+
it('should show only E2E mean column and TPS mean column when E2E mean filter is applied', () => {
439433
modelCatalog.findModelCatalogDetailLink().first().click();
440434
modelCatalog.clickPerformanceInsightsTab();
441435

@@ -451,25 +445,19 @@ describe('Model Catalog Details Tabs', () => {
451445
// Apply filter
452446
modelCatalog.clickApplyFilter();
453447

454-
// Only E2E mean column should be visible
448+
// E2E mean column should be visible
455449
modelCatalog
456450
.findHardwareConfigurationTableHeaders()
457451
.should('contain.text', `E2E${NBSP}Latency Mean`);
458452

453+
// TPS Mean column should be visible (matching percentile)
454+
modelCatalog
455+
.findHardwareConfigurationTableHeaders()
456+
.should('contain.text', `TPS${NBSP}Latency Mean`);
457+
459458
// TTFT and ITL columns should be hidden
460459
modelCatalog.findHardwareConfigurationTableHeaders().should('not.contain.text', 'TTFT');
461460
modelCatalog.findHardwareConfigurationTableHeaders().should('not.contain.text', 'ITL');
462-
463-
// Other E2E percentile columns should be hidden
464-
modelCatalog
465-
.findHardwareConfigurationTableHeaders()
466-
.should('not.contain.text', 'Latency P90');
467-
modelCatalog
468-
.findHardwareConfigurationTableHeaders()
469-
.should('not.contain.text', 'Latency P95');
470-
modelCatalog
471-
.findHardwareConfigurationTableHeaders()
472-
.should('not.contain.text', 'Latency P99');
473461
});
474462

475463
it('should restore all latency columns when filter is reset', () => {
@@ -480,10 +468,13 @@ describe('Model Catalog Details Tabs', () => {
480468
modelCatalog.openLatencyFilter();
481469
modelCatalog.clickApplyFilter();
482470

483-
// Verify only TTFT P90 latency column is shown
471+
// Verify TTFT P90 and TPS P90 latency columns are shown
484472
modelCatalog
485473
.findHardwareConfigurationTableHeaders()
486474
.should('contain.text', `TTFT${NBSP}Latency P90`);
475+
modelCatalog
476+
.findHardwareConfigurationTableHeaders()
477+
.should('contain.text', `TPS${NBSP}Latency P90`);
487478
modelCatalog.findHardwareConfigurationTableHeaders().should('not.contain.text', 'E2E');
488479

489480
// Open filter and reset
@@ -497,6 +488,7 @@ describe('Model Catalog Details Tabs', () => {
497488
modelCatalog.findHardwareConfigurationTableHeaders().should('contain.text', 'TTFT');
498489
modelCatalog.findHardwareConfigurationTableHeaders().should('contain.text', 'E2E');
499490
modelCatalog.findHardwareConfigurationTableHeaders().should('contain.text', 'ITL');
491+
modelCatalog.findHardwareConfigurationTableHeaders().should('contain.text', 'TPS');
500492
});
501493

502494
it('should keep non-latency columns visible when latency filter is applied', () => {

clients/ui/frontend/src/app/pages/modelCatalog/components/HardwareConfigurationTable.tsx

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { CatalogPerformanceMetricsArtifact } from '~/app/modelCatalogTypes';
66
import { ModelCatalogContext } from '~/app/context/modelCatalog/ModelCatalogContext';
77
import { clearAllFilters } from '~/app/pages/modelCatalog/utils/hardwareConfigurationFilterUtils';
88
import { getActiveLatencyFieldName } from '~/app/pages/modelCatalog/utils/modelCatalogUtils';
9-
import { ALL_LATENCY_FIELD_NAMES } from '~/concepts/modelCatalog/const';
9+
import { ALL_LATENCY_FIELD_NAMES, LatencyMetric } from '~/concepts/modelCatalog/const';
1010
import { hardwareConfigColumns, HardwareConfigColumn } from './HardwareConfigurationTableColumns';
1111
import HardwareConfigurationTableRow from './HardwareConfigurationTableRow';
1212
import HardwareConfigurationFilterToolbar from './HardwareConfigurationFilterToolbar';
@@ -28,13 +28,21 @@ const HardwareConfigurationTable: React.FC<HardwareConfigurationTableProps> = ({
2828
// Get the active latency filter field name (if any)
2929
const activeLatencyField = getActiveLatencyFieldName(filterData);
3030

31+
const tpsPrefix = LatencyMetric.TPS.toLowerCase();
32+
3133
// When a latency filter is selected, show only that column and hide other latency columns
34+
// Also show the TPS column with the matching percentile (e.g., TTFT P90 filter shows TPS P90)
3235
const filteredColumns = React.useMemo((): HardwareConfigColumn[] => {
3336
if (!activeLatencyField) {
3437
// No latency filter selected, show all columns
3538
return hardwareConfigColumns;
3639
}
3740

41+
// Extract the percentile suffix from the active filter (e.g., 'ttft_p90' -> 'p90', 'e2e_mean' -> 'mean')
42+
const percentileSuffix = activeLatencyField.split('_').pop();
43+
// Build the matching TPS field name (e.g., 'tps_p90', 'tps_mean')
44+
const matchingTpsField = `${tpsPrefix}_${percentileSuffix}`;
45+
3846
// Filter out latency columns that don't match the active filter
3947
return hardwareConfigColumns.filter((column) => {
4048
// Check if this column is a latency column
@@ -47,10 +55,15 @@ const HardwareConfigurationTable: React.FC<HardwareConfigurationTableProps> = ({
4755
return true;
4856
}
4957

50-
// If it's a latency column, only keep it if it matches the active filter
58+
// Show TPS column with matching percentile (they measure throughput, not latency delay)
59+
if (column.field === matchingTpsField) {
60+
return true;
61+
}
62+
63+
// If it's a latency column (not TPS), only keep it if it matches the active filter
5164
return column.field === activeLatencyField;
5265
});
53-
}, [activeLatencyField]);
66+
}, [activeLatencyField, tpsPrefix]);
5467

5568
if (isLoading) {
5669
return <Spinner size="lg" />;

clients/ui/frontend/src/app/pages/modelCatalog/components/ModelCatalogCardBody.tsx

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {
1414
} from '@patternfly/react-core';
1515
import { Link } from 'react-router-dom';
1616
import { HelpIcon, AngleLeftIcon, AngleRightIcon, ArrowRightIcon } from '@patternfly/react-icons';
17+
import { TruncatedText } from 'mod-arch-shared';
1718
import {
1819
CatalogModel,
1920
CatalogSource,
@@ -39,14 +40,6 @@ type ModelCatalogCardBodyProps = {
3940
source: CatalogSource | undefined;
4041
};
4142

42-
const descriptionStyle: React.CSSProperties = {
43-
overflow: 'hidden',
44-
textOverflow: 'ellipsis',
45-
WebkitLineClamp: 4,
46-
WebkitBoxOrient: 'vertical',
47-
display: '-webkit-box',
48-
};
49-
5043
const ModelCatalogCardBody: React.FC<ModelCatalogCardBodyProps> = ({
5144
model,
5245
isValidated,
@@ -80,7 +73,7 @@ const ModelCatalogCardBody: React.FC<ModelCatalogCardBodyProps> = ({
8073
},
8174
filterData,
8275
filterOptions,
83-
isValidated, // Only fetch if validated
76+
isValidated && performanceViewEnabled, // Only fetch if validated AND toggle is ON
8477
);
8578

8679
const performanceMetrics = filterArtifactsByType<CatalogPerformanceMetricsArtifact>(
@@ -95,13 +88,13 @@ const ModelCatalogCardBody: React.FC<ModelCatalogCardBodyProps> = ({
9588
MetricsType.accuracyMetrics,
9689
);
9790

98-
const isLoading = isValidated && !performanceArtifactsLoaded;
91+
const isLoading = isValidated && performanceViewEnabled && !performanceArtifactsLoaded;
9992

10093
if (isLoading) {
10194
return <Spinner />;
10295
}
10396

104-
if (performanceArtifactsError && isValidated) {
97+
if (performanceArtifactsError && isValidated && performanceViewEnabled) {
10598
return (
10699
<Alert variant="danger" isInline title={performanceArtifactsError.name}>
107100
{performanceArtifactsError.message}
@@ -115,9 +108,11 @@ const ModelCatalogCardBody: React.FC<ModelCatalogCardBodyProps> = ({
115108
return (
116109
<Stack hasGutter>
117110
<StackItem>
118-
<div data-testid="model-catalog-card-description" style={descriptionStyle}>
119-
{model.description}
120-
</div>
111+
<TruncatedText
112+
content={model.description || ''}
113+
maxLines={4}
114+
data-testid="model-catalog-card-description"
115+
/>
121116
</StackItem>
122117
<StackItem>
123118
<Link
@@ -265,9 +260,11 @@ const ModelCatalogCardBody: React.FC<ModelCatalogCardBodyProps> = ({
265260

266261
// Standard card body for non-validated models
267262
return (
268-
<div data-testid="model-catalog-card-description" style={descriptionStyle}>
269-
{model.description}
270-
</div>
263+
<TruncatedText
264+
content={model.description || ''}
265+
maxLines={4}
266+
data-testid="model-catalog-card-description"
267+
/>
271268
);
272269
};
273270

0 commit comments

Comments
 (0)