From 71b26315bb68b16f0e254ebb9d1c8a85f8c25c1d Mon Sep 17 00:00:00 2001 From: Jeff Puzzo Date: Tue, 11 Jun 2024 15:25:01 -0400 Subject: [PATCH] [RHOAIENG-7481] Add metrics columns to pipeline run table and modal param selector --- frontend/package-lock.json | 112 ++++++++++-- frontend/package.json | 1 + .../cypress/pages/pipelines/compareRuns.ts | 2 +- frontend/src/components/table/CheckboxTd.tsx | 9 +- frontend/src/components/table/TableBase.tsx | 35 ++-- frontend/src/components/table/types.ts | 9 +- .../__tests__/useGetArtifactsByRuns.spec.ts | 155 +++++++++++++++++ .../apiHooks/mlmd/useGetArtifactsByRuns.ts | 43 +++++ .../artifacts/ArtifactVisualization.tsx | 22 +-- .../artifacts/__tests__/utils.spec.ts | 31 ++++ .../pipelineRun/artifacts/types.ts | 4 + .../pipelineRun/artifacts/utils.ts | 22 +++ .../__tests__/useMetricColumnNames.spec.ts | 37 ++++ .../pipelines/content/tables/columns.ts | 48 +++++ .../pipelineRun/CustomMetricsColumnsModal.tsx | 164 ++++++++++++++++++ .../pipelineRun/MetricColumnSearchInput.tsx | 27 +++ .../tables/pipelineRun/PipelineRunTable.tsx | 141 +++++++++++++-- .../pipelineRun/PipelineRunTableRow.tsx | 15 +- .../pipelineRun/UnavailableMetricValue.tsx | 24 +++ .../content/tables/pipelineRun/types.ts | 4 + .../pipelineRun/useMetricColumnNames.ts | 38 ++++ .../content/tables/pipelineRun/utils.ts | 3 + 22 files changed, 882 insertions(+), 64 deletions(-) create mode 100644 frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetArtifactsByRuns.spec.ts create mode 100644 frontend/src/concepts/pipelines/apiHooks/mlmd/useGetArtifactsByRuns.ts create mode 100644 frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/__tests__/utils.spec.ts create mode 100644 frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/types.ts create mode 100644 frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/utils.ts create mode 100644 frontend/src/concepts/pipelines/content/tables/__tests__/useMetricColumnNames.spec.ts create mode 100644 frontend/src/concepts/pipelines/content/tables/pipelineRun/CustomMetricsColumnsModal.tsx create mode 100644 frontend/src/concepts/pipelines/content/tables/pipelineRun/MetricColumnSearchInput.tsx create mode 100644 frontend/src/concepts/pipelines/content/tables/pipelineRun/UnavailableMetricValue.tsx create mode 100644 frontend/src/concepts/pipelines/content/tables/pipelineRun/types.ts create mode 100644 frontend/src/concepts/pipelines/content/tables/pipelineRun/useMetricColumnNames.ts create mode 100644 frontend/src/concepts/pipelines/content/tables/pipelineRun/utils.ts diff --git a/frontend/package-lock.json b/frontend/package-lock.json index f9a8d143d9..b7cc54ac88 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -17,6 +17,7 @@ "@patternfly/react-charts": "^7.1.1", "@patternfly/react-code-editor": "^5.2.1", "@patternfly/react-core": "^5.2.1", + "@patternfly/react-drag-drop": "^5.3.3", "@patternfly/react-icons": "^5.2.1", "@patternfly/react-log-viewer": "^5.2.0", "@patternfly/react-styles": "^5.2.1", @@ -2240,6 +2241,68 @@ "node": ">=10.0.0" } }, + "node_modules/@dnd-kit/accessibility": { + "version": "3.1.0", + "resolved": "https://registry.npmjs.org/@dnd-kit/accessibility/-/accessibility-3.1.0.tgz", + "integrity": "sha512-ea7IkhKvlJUv9iSHJOnxinBcoOI3ppGnnL+VDJ75O45Nss6HtZd8IdN8touXPDtASfeI2T2LImb8VOZcL47wjQ==", + "dependencies": { + "tslib": "^2.0.0" + }, + "peerDependencies": { + "react": ">=16.8.0" + } + }, + "node_modules/@dnd-kit/core": { + "version": "6.1.0", + "resolved": "https://registry.npmjs.org/@dnd-kit/core/-/core-6.1.0.tgz", + "integrity": "sha512-J3cQBClB4TVxwGo3KEjssGEXNJqGVWx17aRTZ1ob0FliR5IjYgTxl5YJbKTzA6IzrtelotH19v6y7uoIRUZPSg==", + "dependencies": { + "@dnd-kit/accessibility": "^3.1.0", + "@dnd-kit/utilities": "^3.2.2", + "tslib": "^2.0.0" + }, + "peerDependencies": { + "react": ">=16.8.0", + "react-dom": ">=16.8.0" + } + }, + "node_modules/@dnd-kit/modifiers": { + "version": "6.0.1", + "resolved": "https://registry.npmjs.org/@dnd-kit/modifiers/-/modifiers-6.0.1.tgz", + "integrity": "sha512-rbxcsg3HhzlcMHVHWDuh9LCjpOVAgqbV78wLGI8tziXY3+qcMQ61qVXIvNKQFuhj75dSfD+o+PYZQ/NUk2A23A==", + "dependencies": { + "@dnd-kit/utilities": "^3.2.1", + "tslib": "^2.0.0" + }, + "peerDependencies": { + "@dnd-kit/core": "^6.0.6", + "react": ">=16.8.0" + } + }, + "node_modules/@dnd-kit/sortable": { + "version": "7.0.2", + "resolved": "https://registry.npmjs.org/@dnd-kit/sortable/-/sortable-7.0.2.tgz", + "integrity": "sha512-wDkBHHf9iCi1veM834Gbk1429bd4lHX4RpAwT0y2cHLf246GAvU2sVw/oxWNpPKQNQRQaeGXhAVgrOl1IT+iyA==", + "dependencies": { + "@dnd-kit/utilities": "^3.2.0", + "tslib": "^2.0.0" + }, + "peerDependencies": { + "@dnd-kit/core": "^6.0.7", + "react": ">=16.8.0" + } + }, + "node_modules/@dnd-kit/utilities": { + "version": "3.2.2", + "resolved": "https://registry.npmjs.org/@dnd-kit/utilities/-/utilities-3.2.2.tgz", + "integrity": "sha512-+MKAJEOfaBe5SmV6t34p80MMKhjvUz0vRrvVJbPT0WElzaOJ/1xs+D+KDv+tD/NE5ujfrChEcshd4fLn0wpiqg==", + "dependencies": { + "tslib": "^2.0.0" + }, + "peerDependencies": { + "react": ">=16.8.0" + } + }, "node_modules/@eslint-community/eslint-utils": { "version": "4.4.0", "resolved": "https://registry.npmjs.org/@eslint-community/eslint-utils/-/eslint-utils-4.4.0.tgz", @@ -3854,13 +3917,13 @@ } }, "node_modules/@patternfly/react-core": { - "version": "5.2.1", - "resolved": "https://registry.npmjs.org/@patternfly/react-core/-/react-core-5.2.1.tgz", - "integrity": "sha512-SWQHALhcjxjmwcIJ6V3tG6V7a2M0WkkUbc6F8mSPk6l9q6j3f+WvZ9HqgzVA+h+Q12UbtIrlQvgUx7pAxZekkg==", + "version": "5.3.3", + "resolved": "https://registry.npmjs.org/@patternfly/react-core/-/react-core-5.3.3.tgz", + "integrity": "sha512-qq3j0M+Vi+Xmd+a/MhRhGgjdRh9Hnm79iA+L935HwMIVDcIWRYp6Isib/Ha4+Jk+f3Qdl0RT3dBDvr/4m6OpVQ==", "dependencies": { - "@patternfly/react-icons": "^5.2.1", - "@patternfly/react-styles": "^5.2.1", - "@patternfly/react-tokens": "^5.2.1", + "@patternfly/react-icons": "^5.3.2", + "@patternfly/react-styles": "^5.3.1", + "@patternfly/react-tokens": "^5.3.1", "focus-trap": "7.5.2", "react-dropzone": "^14.2.3", "tslib": "^2.5.0" @@ -3870,10 +3933,29 @@ "react-dom": "^17 || ^18" } }, + "node_modules/@patternfly/react-drag-drop": { + "version": "5.3.3", + "resolved": "https://registry.npmjs.org/@patternfly/react-drag-drop/-/react-drag-drop-5.3.3.tgz", + "integrity": "sha512-zqv6nR2Q7pElJ6v2auFVZQxFgEVfKZh4Tq3/RxEt2lCIoGofhW1W0unzdT6WTxHX5l1KrsxF7SNLdSbFt437ZQ==", + "dependencies": { + "@dnd-kit/core": "^6.0.8", + "@dnd-kit/modifiers": "^6.0.1", + "@dnd-kit/sortable": "^7.0.2", + "@patternfly/react-core": "^5.3.3", + "@patternfly/react-icons": "^5.3.2", + "@patternfly/react-styles": "^5.3.1", + "memoize-one": "^5.1.0", + "resize-observer-polyfill": "^1.5.1" + }, + "peerDependencies": { + "react": "^17 || ^18", + "react-dom": "^17 || ^18" + } + }, "node_modules/@patternfly/react-icons": { - "version": "5.2.1", - "resolved": "https://registry.npmjs.org/@patternfly/react-icons/-/react-icons-5.2.1.tgz", - "integrity": "sha512-aeJ0X+U2NDe8UmI5eQiT0iuR/wmUq97UkDtx3HoZcpRb9T6eUBfysllxjRqHS8rOOspdU8OWq+CUhQ/E2ZDibg==", + "version": "5.3.2", + "resolved": "https://registry.npmjs.org/@patternfly/react-icons/-/react-icons-5.3.2.tgz", + "integrity": "sha512-GEygYbl0H4zD8nZuTQy2dayKIrV2bMMeWKSOEZ16Y3EYNgYVUOUnN+J0naAEuEGH39Xb1DE9n+XUbE1PC4CxPA==", "peerDependencies": { "react": "^17 || ^18", "react-dom": "^17 || ^18" @@ -3895,9 +3977,9 @@ } }, "node_modules/@patternfly/react-styles": { - "version": "5.2.1", - "resolved": "https://registry.npmjs.org/@patternfly/react-styles/-/react-styles-5.2.1.tgz", - "integrity": "sha512-GT96hzI1QenBhq6Pfc51kxnj9aVLjL1zSLukKZXcYVe0HPOy0BFm90bT1Fo4e/z7V9cDYw4SqSX1XLc3O4jsTw==" + "version": "5.3.1", + "resolved": "https://registry.npmjs.org/@patternfly/react-styles/-/react-styles-5.3.1.tgz", + "integrity": "sha512-H6uBoFH3bJjD6PP75qZ4k+2TtF59vxf9sIVerPpwrGJcRgBZbvbMZCniSC3+S2LQ8DgXLnDvieq78jJzHz0hiA==" }, "node_modules/@patternfly/react-table": { "version": "5.2.1", @@ -3917,9 +3999,9 @@ } }, "node_modules/@patternfly/react-tokens": { - "version": "5.2.1", - "resolved": "https://registry.npmjs.org/@patternfly/react-tokens/-/react-tokens-5.2.1.tgz", - "integrity": "sha512-8GYz/jnJTGAWUJt5eRAW5dtyiHPKETeFJBPGHaUQnvi/t1ZAkoy8i4Kd/RlHsDC7ktiu813SKCmlzwBwldAHKg==" + "version": "5.3.1", + "resolved": "https://registry.npmjs.org/@patternfly/react-tokens/-/react-tokens-5.3.1.tgz", + "integrity": "sha512-VYK0uVP2/2RJ7ZshJCCLeq0Boih5I1bv+9Z/Bg6h12dCkLs85XsxAX9Ve+BGIo5DF54/mzcRHE1RKYap4ISXuw==" }, "node_modules/@patternfly/react-topology": { "version": "5.4.0-prerelease.6", diff --git a/frontend/package.json b/frontend/package.json index f1bbf2f623..6248757144 100644 --- a/frontend/package.json +++ b/frontend/package.json @@ -60,6 +60,7 @@ "@patternfly/react-charts": "^7.1.1", "@patternfly/react-code-editor": "^5.2.1", "@patternfly/react-core": "^5.2.1", + "@patternfly/react-drag-drop": "^5.3.3", "@patternfly/react-icons": "^5.2.1", "@patternfly/react-log-viewer": "^5.2.0", "@patternfly/react-styles": "^5.2.1", diff --git a/frontend/src/__tests__/cypress/cypress/pages/pipelines/compareRuns.ts b/frontend/src/__tests__/cypress/cypress/pages/pipelines/compareRuns.ts index e1c23b9529..c4fb2dcf0a 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/pipelines/compareRuns.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/pipelines/compareRuns.ts @@ -44,7 +44,7 @@ class CompareRunParamsTable { } findEmptyState() { - return this.find().parent().findByTestId('compare-runs-params-empty-state'); + return this.find().parent().parent().findByTestId('compare-runs-params-empty-state'); } findColumnByName(name: string) { diff --git a/frontend/src/components/table/CheckboxTd.tsx b/frontend/src/components/table/CheckboxTd.tsx index 3d689d1381..b8d12b77f9 100644 --- a/frontend/src/components/table/CheckboxTd.tsx +++ b/frontend/src/components/table/CheckboxTd.tsx @@ -8,7 +8,7 @@ type CheckboxTrProps = { onToggle: () => void; isDisabled?: boolean; tooltip?: string; -}; +} & React.ComponentProps; const CheckboxTd: React.FC = ({ id, @@ -16,6 +16,7 @@ const CheckboxTd: React.FC = ({ onToggle, isDisabled, tooltip, + ...props }) => { let content = ( = ({ content = {content}; } - return {content}; + return ( + + {content} + + ); }; export default CheckboxTd; diff --git a/frontend/src/components/table/TableBase.tsx b/frontend/src/components/table/TableBase.tsx index 9c4c6ff695..2c2cd08e50 100644 --- a/frontend/src/components/table/TableBase.tsx +++ b/frontend/src/components/table/TableBase.tsx @@ -19,6 +19,7 @@ import { Tbody, Td, TbodyProps, + InnerScrollContainer, } from '@patternfly/react-table'; import { EitherNotBoth } from '~/typeHelpers'; import { GetColumnSort, SortableData } from './types'; @@ -47,6 +48,7 @@ type Props = { }; getColumnSort?: GetColumnSort; disableItemCount?: boolean; + hasStickyColumns?: boolean; } & EitherNotBoth< { disableRowRenderSupport?: boolean }, { tbodyProps?: TbodyProps & { ref?: React.Ref } } @@ -109,6 +111,7 @@ const TableBase = ({ loading, toggleTemplate, disableItemCount = false, + hasStickyColumns, ...props }: Props): React.ReactElement => { const selectAllRef = React.useRef(null); @@ -159,6 +162,8 @@ const TableBase = ({ ref={selectAllRef} colSpan={col.colSpan} rowSpan={col.rowSpan} + isStickyColumn={col.isStickyColumn} + stickyMinWidth={col.stickyMinWidth} select={{ isSelected: selectAll.selected, onSelect: (e, value) => selectAll.onSelect(value), @@ -182,6 +187,8 @@ const TableBase = ({ info={col.info} isSubheader={isSubheader} isStickyColumn={col.isStickyColumn} + stickyMinWidth={col.stickyMinWidth} + stickyLeftOffset={col.stickyLeftOffset} hasRightBorder={col.hasRightBorder} modifier={col.modifier} className={col.className} @@ -246,6 +253,20 @@ const TableBase = ({ }) : data.map((row, rowIndex) => rowRenderer(row, rowIndex)); + const table = ( + + {caption && } + + {columns.map((col, i) => renderColumnHeader(col, i))} + {subColumns?.length ? ( + {subColumns.map((col, i) => renderColumnHeader(col, columns.length + i, true))} + ) : null} + + {disableRowRenderSupport ? renderRows() : {renderRows()}} + {footerRow && footerRow(page)} +
{caption}
+ ); + return ( <> {(toolbarContent || showPagination) && ( @@ -268,17 +289,9 @@ const TableBase = ({ )} - - {caption && } - - {columns.map((col, i) => renderColumnHeader(col, i))} - {subColumns?.length ? ( - {subColumns.map((col, i) => renderColumnHeader(col, columns.length + i, true))} - ) : null} - - {disableRowRenderSupport ? renderRows() : {renderRows()}} - {footerRow && footerRow(page)} -
{caption}
+ + {hasStickyColumns ? {table} : table} + {!loading && emptyTableView && data.length === 0 && (
{emptyTableView} diff --git a/frontend/src/components/table/types.ts b/frontend/src/components/table/types.ts index b07488647a..8e6bc15586 100644 --- a/frontend/src/components/table/types.ts +++ b/frontend/src/components/table/types.ts @@ -4,7 +4,14 @@ export type GetColumnSort = (columnIndex: number) => ThProps['sort']; export type SortableData = Pick< ThProps, - 'hasRightBorder' | 'isStickyColumn' | 'modifier' | 'width' | 'info' | 'className' + | 'hasRightBorder' + | 'isStickyColumn' + | 'stickyMinWidth' + | 'stickyLeftOffset' + | 'modifier' + | 'width' + | 'info' + | 'className' > & { label: string; field: string; diff --git a/frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetArtifactsByRuns.spec.ts b/frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetArtifactsByRuns.spec.ts new file mode 100644 index 0000000000..d49a309d98 --- /dev/null +++ b/frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetArtifactsByRuns.spec.ts @@ -0,0 +1,155 @@ +import { testHook, standardUseFetchState } from '~/__tests__/unit/testUtils/hooks'; +import { + MetadataStoreServicePromiseClient, + Artifact, + Execution, + Event, + Context, +} from '~/third_party/mlmd'; +import { usePipelinesAPI } from '~/concepts/pipelines/context'; +import { getMlmdContext } from '~/concepts/pipelines/apiHooks/mlmd/useMlmdContext'; +import { PipelineRunKFv2 } from '~/concepts/pipelines/kfTypes'; +import { + GetArtifactsByContextResponse, + GetExecutionsByContextResponse, + GetEventsByExecutionIDsResponse, +} from '~/third_party/mlmd/generated/ml_metadata/proto/metadata_store_service_pb'; +import { useGetArtifactsByRuns } from '~/concepts/pipelines/apiHooks/mlmd/useGetArtifactsByRuns'; + +// Mock the usePipelinesAPI and getMlmdContext hooks +jest.mock('~/concepts/pipelines/context', () => ({ + usePipelinesAPI: jest.fn(), +})); + +jest.mock('~/concepts/pipelines/apiHooks/mlmd/useMlmdContext', () => ({ + getMlmdContext: jest.fn(), +})); + +// Mock the MetadataStoreServicePromiseClient +jest.mock('~/third_party/mlmd', () => { + const originalModule = jest.requireActual('~/third_party/mlmd'); + return { + ...originalModule, + MetadataStoreServicePromiseClient: jest.fn().mockImplementation(() => ({ + getArtifactsByContext: jest.fn(), + getExecutionsByContext: jest.fn(), + getEventsByExecutionIDs: jest.fn(), + })), + GetArtifactsByContextRequest: originalModule.GetArtifactsByContextRequest, + GetExecutionsByContextRequest: originalModule.GetExecutionsByContextRequest, + GetEventsByExecutionIDsRequest: originalModule.GetEventsByExecutionIDsRequest, + }; +}); + +describe('useGetArtifactsByRuns', () => { + const mockClient = new MetadataStoreServicePromiseClient(''); + const mockUsePipelinesAPI = jest.mocked( + usePipelinesAPI as () => Partial>, + ); + const mockGetMlmdContext = jest.mocked(getMlmdContext); + const mockGetArtifactsByContext = jest.mocked(mockClient.getArtifactsByContext); + const mockGetExecutionsByContext = jest.mocked(mockClient.getExecutionsByContext); + const mockGetEventsByExecutionIDs = jest.mocked(mockClient.getEventsByExecutionIDs); + + const mockContext = new Context(); + mockContext.setId(1); + + const mockArtifact = new Artifact(); + mockArtifact.setId(1); + mockArtifact.setName('artifact1'); + + const mockExecution = new Execution(); + mockExecution.setId(1); + + const mockEvent = new Event(); + mockEvent.getArtifactId = jest.fn().mockReturnValue(1); + mockEvent.getExecutionId = jest.fn().mockReturnValue(1); + + // eslint-disable-next-line camelcase + const mockRun = { run_id: 'test-run-id' } as PipelineRunKFv2; + + beforeEach(() => { + jest.clearAllMocks(); + mockUsePipelinesAPI.mockReturnValue({ + metadataStoreServiceClient: mockClient, + }); + }); + + it('throws error when no MLMD context is found', async () => { + mockGetMlmdContext.mockResolvedValue(undefined); + const renderResult = testHook(useGetArtifactsByRuns)([mockRun]); + + // wait for update + await renderResult.waitForNextUpdate(); + + expect(renderResult.result.current).toEqual( + standardUseFetchState([], false, new Error('No context for run: test-run-id')), + ); + }); + + it('should fetch and return MLMD packages for pipeline runs', async () => { + mockGetMlmdContext.mockResolvedValue(mockContext); + mockGetArtifactsByContext.mockResolvedValue({ + getArtifactsList: () => [mockArtifact], + } as GetArtifactsByContextResponse); + mockGetExecutionsByContext.mockResolvedValue({ + getExecutionsList: () => [mockExecution], + } as GetExecutionsByContextResponse); + mockGetEventsByExecutionIDs.mockResolvedValue({ + getEventsList: () => [mockEvent], + } as GetEventsByExecutionIDsResponse); + + const renderResult = testHook(useGetArtifactsByRuns)([mockRun]); + + expect(renderResult.result.current).toStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + + expect(renderResult.result.current).toStrictEqual( + standardUseFetchState( + [ + { + [mockRun.run_id]: [mockArtifact], + }, + ], + true, + ), + ); + expect(renderResult).hookToHaveUpdateCount(2); + }); + + it('should handle errors from getMlmdContext', async () => { + const error = new Error('Cannot fetch context'); + mockGetMlmdContext.mockRejectedValue(error); + + const renderResult = testHook(useGetArtifactsByRuns)([mockRun]); + + expect(renderResult.result.current).toStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + + expect(renderResult.result.current).toStrictEqual(standardUseFetchState([], false, error)); + expect(renderResult).hookToHaveUpdateCount(2); + }); + + it('should handle errors from getArtifactsByContext', async () => { + const error = new Error('Cannot fetch artifacts'); + mockGetMlmdContext.mockResolvedValue(mockContext); + mockGetArtifactsByContext.mockRejectedValue(error); + + const renderResult = testHook(useGetArtifactsByRuns)([mockRun]); + + expect(renderResult.result.current).toStrictEqual(standardUseFetchState([])); + expect(renderResult).hookToHaveUpdateCount(1); + + // wait for update + await renderResult.waitForNextUpdate(); + + expect(renderResult.result.current).toStrictEqual(standardUseFetchState([], false, error)); + expect(renderResult).hookToHaveUpdateCount(2); + }); +}); diff --git a/frontend/src/concepts/pipelines/apiHooks/mlmd/useGetArtifactsByRuns.ts b/frontend/src/concepts/pipelines/apiHooks/mlmd/useGetArtifactsByRuns.ts new file mode 100644 index 0000000000..6ec4d43ca2 --- /dev/null +++ b/frontend/src/concepts/pipelines/apiHooks/mlmd/useGetArtifactsByRuns.ts @@ -0,0 +1,43 @@ +import React from 'react'; + +import { Artifact } from '~/third_party/mlmd'; +import { GetArtifactsByContextRequest } from '~/third_party/mlmd/generated/ml_metadata/proto/metadata_store_service_pb'; +import useFetchState, { FetchState, FetchStateCallbackPromise } from '~/utilities/useFetchState'; +import { usePipelinesAPI } from '~/concepts/pipelines/context'; +import { PipelineRunKFv2 } from '~/concepts/pipelines/kfTypes'; +import { MlmdContextTypes } from './types'; +import { getMlmdContext } from './useMlmdContext'; + +export const useGetArtifactsByRuns = ( + runs: PipelineRunKFv2[], +): FetchState[]> => { + const { metadataStoreServiceClient } = usePipelinesAPI(); + + const call = React.useCallback[]>>( + () => + Promise.all( + runs.map((run) => + getMlmdContext(metadataStoreServiceClient, run.run_id, MlmdContextTypes.RUN).then( + async (context) => { + if (!context) { + throw new Error(`No context for run: ${run.run_id}`); + } + + const request = new GetArtifactsByContextRequest(); + request.setContextId(context.getId()); + + const response = await metadataStoreServiceClient.getArtifactsByContext(request); + const artifacts = response.getArtifactsList(); + + return { + [run.run_id]: artifacts, + }; + }, + ), + ), + ), + [metadataStoreServiceClient, runs], + ); + + return useFetchState(call, []); +}; diff --git a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/ArtifactVisualization.tsx b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/ArtifactVisualization.tsx index a6b8386810..c9903fad86 100644 --- a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/ArtifactVisualization.tsx +++ b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/ArtifactVisualization.tsx @@ -36,6 +36,7 @@ import { extractS3UriComponents } from '~/concepts/pipelines/content/artifacts/u import MarkdownView from '~/components/MarkdownView'; import { useIsAreaAvailable, SupportedArea } from '~/concepts/areas'; import { bytesAsRoundedGiB } from '~/utilities/number'; +import { getArtifactProperties } from './utils'; interface ArtifactVisualizationProps { artifact: Artifact; @@ -116,24 +117,7 @@ export const ArtifactVisualization: React.FC = ({ ar } if (artifactType === ArtifactType.METRICS) { - const scalarMetrics = artifact - .toObject() - .customPropertiesMap.reduce( - ( - acc: { name: string; value: string }[], - [customPropKey, { stringValue, intValue, doubleValue, boolValue }], - ) => { - if (customPropKey !== 'display_name') { - acc.push({ - name: customPropKey, - value: stringValue || (intValue || doubleValue || boolValue).toString(), - }); - } - - return acc; - }, - [], - ); + const artifactProperties = getArtifactProperties(artifact); return ( @@ -141,7 +125,7 @@ export const ArtifactVisualization: React.FC = ({ ar { + const mockArtifact = new Artifact(); + mockArtifact.setId(1); + mockArtifact.setName('artifact-1'); + mockArtifact.getCustomPropertiesMap().set('display_name', new Value()); + + it('returns empty array when no custom props exist other than display_name', () => { + const result = getArtifactProperties(mockArtifact); + expect(result).toEqual([]); + }); + + it('returns artifact properties when custom props exist other than display_name', () => { + mockArtifact + .getCustomPropertiesMap() + .set('metric-string', new Value().setStringValue('some string')); + mockArtifact.getCustomPropertiesMap().set('metric-int', new Value().setIntValue(10)); + mockArtifact.getCustomPropertiesMap().set('metric-double', new Value().setDoubleValue(1.1)); + mockArtifact.getCustomPropertiesMap().set('metric-bool', new Value().setBoolValue(true)); + + const result = getArtifactProperties(mockArtifact); + expect(result).toEqual([ + { name: 'metric-bool', value: 'true' }, + { name: 'metric-double', value: '1.1' }, + { name: 'metric-int', value: '10' }, + { name: 'metric-string', value: 'some string' }, + ]); + }); +}); diff --git a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/types.ts b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/types.ts new file mode 100644 index 0000000000..e44ad51402 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/types.ts @@ -0,0 +1,4 @@ +export interface ArtifactProperty { + name: string; + value: string; +} diff --git a/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/utils.ts b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/utils.ts new file mode 100644 index 0000000000..2c55df61ca --- /dev/null +++ b/frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/utils.ts @@ -0,0 +1,22 @@ +import { Artifact } from '~/third_party/mlmd'; +import { ArtifactProperty } from './types'; + +export const getArtifactProperties = (artifact: Artifact): ArtifactProperty[] => + artifact + .toObject() + .customPropertiesMap.reduce( + ( + acc: { name: string; value: string }[], + [customPropKey, { stringValue, intValue, doubleValue, boolValue }], + ) => { + if (customPropKey !== 'display_name') { + acc.push({ + name: customPropKey, + value: stringValue || (intValue || doubleValue || boolValue).toString(), + }); + } + + return acc; + }, + [], + ); diff --git a/frontend/src/concepts/pipelines/content/tables/__tests__/useMetricColumnNames.spec.ts b/frontend/src/concepts/pipelines/content/tables/__tests__/useMetricColumnNames.spec.ts new file mode 100644 index 0000000000..5dd75f4bf1 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/tables/__tests__/useMetricColumnNames.spec.ts @@ -0,0 +1,37 @@ +import { testHook } from '~/__tests__/unit/testUtils/hooks'; +import { useMetricColumnNames } from '~/concepts/pipelines/content/tables/pipelineRun/useMetricColumnNames'; + +describe('useMetricColumnNames', () => { + const experimentId = 'test-experiment-id'; + const localStorageGetItemSpy = jest.spyOn(Storage.prototype, 'getItem'); + + beforeEach(() => { + localStorageGetItemSpy.mockReset(); + }); + + it('returns first metric as default value when only 1 metric exists', () => { + const renderResult = testHook(useMetricColumnNames)(experimentId, new Set(['metric-1'])); + expect(renderResult.result.current).toEqual(['metric-1']); + }); + + it('returns first 2 metrics as default values', () => { + const renderResult = testHook(useMetricColumnNames)( + experimentId, + new Set(['metric-1', 'metric-2', 'metric-3']), + ); + + expect(renderResult.result.current).toEqual(['metric-1', 'metric-2']); + }); + + it('returns no metrics if localStorage columns are set as empty for the experiment', () => { + localStorageGetItemSpy.mockReturnValue('[]'); + const renderResult = testHook(useMetricColumnNames)(experimentId, new Set(['metric-1'])); + + expect(renderResult.result.current).toEqual([]); + }); + + it('returns no metrics if no defaults are available to choose from', () => { + const renderResult = testHook(useMetricColumnNames)(experimentId, new Set([])); + expect(renderResult.result.current).toEqual([]); + }); +}); diff --git a/frontend/src/concepts/pipelines/content/tables/columns.ts b/frontend/src/concepts/pipelines/content/tables/columns.ts index b232fc6e4a..04213869fd 100644 --- a/frontend/src/concepts/pipelines/content/tables/columns.ts +++ b/frontend/src/concepts/pipelines/content/tables/columns.ts @@ -124,6 +124,54 @@ export const pipelineRunColumns: SortableData[] = [ kebabTableColumn(), ]; +export function getExperimentRunColumns( + metricsColumnNames: string[], +): SortableData[] { + return [ + { ...checkboxTableColumn(), isStickyColumn: true, stickyMinWidth: '45px' }, + { + label: 'Run', + field: 'name', + sortable: true, + isStickyColumn: true, + hasRightBorder: true, + stickyMinWidth: '200px', + stickyLeftOffset: '45px', + width: 20, + }, + { + label: 'Pipeline version', + field: 'pipeline_version', + sortable: false, + width: 15, + }, + { + label: 'Started', + field: 'created_at', + sortable: true, + width: 15, + }, + { + label: 'Duration', + field: 'duration', + sortable: false, + width: 15, + }, + { + label: 'Status', + field: 'status', + sortable: true, + width: 10, + }, + ...metricsColumnNames.map((metricName: string) => ({ + label: metricName, + field: metricName, + sortable: false, + })), + kebabTableColumn(), + ]; +} + export const pipelineRunJobColumns: SortableData[] = [ checkboxTableColumn(), { diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/CustomMetricsColumnsModal.tsx b/frontend/src/concepts/pipelines/content/tables/pipelineRun/CustomMetricsColumnsModal.tsx new file mode 100644 index 0000000000..d42bbef6f6 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/CustomMetricsColumnsModal.tsx @@ -0,0 +1,164 @@ +import React from 'react'; +import { + Button, + Checkbox, + Flex, + FlexItem, + Label, + Modal, + ModalBoxBody, + ModalVariant, + Stack, + StackItem, + Tooltip, +} from '@patternfly/react-core'; +import { DragDropSort, DraggableObject } from '@patternfly/react-drag-drop'; +import { getMetricsColumnsLocalStorageKey } from './utils'; +import { MetricColumnSearchInput } from './MetricColumnSearchInput'; + +interface CustomMetricsColumnsModalProps { + columns: DraggableObject[]; + experimentId: string | undefined; + onClose: () => void; +} + +export const CustomMetricsColumnsModal: React.FC = ({ + onClose, + experimentId, + columns: defaultColumns, +}) => { + const [columns, setColumns] = React.useState(defaultColumns); + const [filteredColumns, setFilteredColumns] = React.useState(defaultColumns); + const metricsColumnsLocalStorageKey = getMetricsColumnsLocalStorageKey(experimentId ?? ''); + const selectedColumnNames = Object.values(columns).reduce((acc: string[], column) => { + if (column.props.checked) { + acc.push(column.id); + } + return acc; + }, []); + + const onUpdate = React.useCallback(() => { + localStorage.removeItem(metricsColumnsLocalStorageKey); + localStorage.setItem(metricsColumnsLocalStorageKey, JSON.stringify(selectedColumnNames)); + + onClose(); + }, [metricsColumnsLocalStorageKey, onClose, selectedColumnNames]); + + return ( + + + Select up to 10 metrics that will display as columns in the table. Drag and drop column + names to reorder them. + + + + { + let newColumns = defaultColumns; + + if (searchText) { + newColumns = defaultColumns.filter((column) => + column.id.toLowerCase().includes(searchText.toLowerCase()), + ); + } + + setFilteredColumns(newColumns); + }} + /> + + + + + + + } + isOpen + onClose={onClose} + actions={[ + , + , + ]} + > + + { + const { + id, + content, + props: { checked }, + } = columns.find((column) => column.id === filteredColumnId) || { + id: filteredColumnId, + }; + const isDisabled = selectedColumnNames.length === 10 && !checked; + + const columnCheckbox = ( + + setColumns((prevColumns) => + prevColumns.map((prevColumn) => { + if (prevColumn.id === id) { + return { id, content, props: { checked: isChecked } }; + } + + return prevColumn; + }), + ) + } + /> + ); + + return { + id, + content: ( +
+ + + {isDisabled ? ( + + {columnCheckbox} + + ) : ( + columnCheckbox + )} + + {id} + +
+ ), + props: { checked }, + }; + })} + variant="defaultWithHandle" + onDrop={(_, newColumns) => { + setFilteredColumns(newColumns); + setColumns(newColumns); + }} + /> + + + ); +}; diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/MetricColumnSearchInput.tsx b/frontend/src/concepts/pipelines/content/tables/pipelineRun/MetricColumnSearchInput.tsx new file mode 100644 index 0000000000..45164927a7 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/MetricColumnSearchInput.tsx @@ -0,0 +1,27 @@ +import React from 'react'; +import { SearchInput } from '@patternfly/react-core'; + +interface MetricColumnSearchInputProps { + onSearch: (value: string) => void; +} + +export const MetricColumnSearchInput: React.FC = ({ onSearch }) => { + const [value, setValue] = React.useState(); + + const onChange = React.useCallback( + (_: React.FormEvent | null, newValue: string) => { + setValue(newValue); + onSearch(newValue); + }, + [onSearch], + ); + + return ( + onChange(null, '')} + /> + ); +}; diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTable.tsx b/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTable.tsx index 57bf1e6f7f..b0b7d5bb9c 100644 --- a/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTable.tsx +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTable.tsx @@ -1,11 +1,16 @@ import React from 'react'; import { useNavigate, useParams } from 'react-router-dom'; -import { Button, Tooltip } from '@patternfly/react-core'; -import { TableVariant } from '@patternfly/react-table'; +import { Button, Skeleton, Tooltip } from '@patternfly/react-core'; +import { TableVariant, Td } from '@patternfly/react-table'; +import { ColumnsIcon } from '@patternfly/react-icons'; + import { TableBase, getTableColumnSort, useCheckboxTable } from '~/components/table'; -import { PipelineRunKFv2 } from '~/concepts/pipelines/kfTypes'; -import { pipelineRunColumns } from '~/concepts/pipelines/content/tables/columns'; +import { ArtifactType, PipelineRunKFv2 } from '~/concepts/pipelines/kfTypes'; +import { + getExperimentRunColumns, + pipelineRunColumns, +} from '~/concepts/pipelines/content/tables/columns'; import PipelineRunTableRow from '~/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow'; import DashboardEmptyTableView from '~/concepts/dashboard/DashboardEmptyTableView'; import PipelineRunTableToolbar from '~/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableToolbar'; @@ -21,6 +26,13 @@ import { useSetVersionFilter } from '~/concepts/pipelines/content/tables/useSetV import { createRunRoute, experimentsCompareRunsRoute } from '~/routes'; import { SupportedArea, useIsAreaAvailable } from '~/concepts/areas'; import { useContextExperimentArchived } from '~/pages/pipelines/global/experiments/ExperimentRunsContext'; +import { getArtifactProperties } from '~/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/utils'; +import { useGetArtifactsByRuns } from '~/concepts/pipelines/apiHooks/mlmd/useGetArtifactsByRuns'; +import { ArtifactProperty } from '~/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/types'; +import { CustomMetricsColumnsModal } from './CustomMetricsColumnsModal'; +import { RunWithMetrics } from './types'; +import { UnavailableMetricValue } from './UnavailableMetricValue'; +import { useMetricColumnNames } from './useMetricColumnNames'; type PipelineRunTableProps = { runs: PipelineRunKFv2[]; @@ -38,13 +50,23 @@ type PipelineRunTableProps = { runType: PipelineRunType.ACTIVE | PipelineRunType.ARCHIVED; }; -const PipelineRunTable: React.FC = ({ +interface PipelineRunTableInternalProps extends Omit { + runs: RunWithMetrics[]; + artifactsLoaded: boolean; + artifactsError: Error | undefined; + metricsNames: Set; +} + +const PipelineRunTableInternal: React.FC = ({ runs, loading, totalSize, page, pageSize, runType, + metricsNames, + artifactsLoaded, + artifactsError, setPage, setPageSize, setFilter, @@ -65,6 +87,7 @@ const PipelineRunTable: React.FC = ({ const [isDeleteModalOpen, setIsDeleteModalOpen] = React.useState(false); const [isArchiveModalOpen, setIsArchiveModalOpen] = React.useState(false); const [isRestoreModalOpen, setIsRestoreModalOpen] = React.useState(false); + const [isCustomColModalOpen, setIsCustomColModalOpen] = React.useState(false); const selectedRuns = selectedIds.reduce((acc: PipelineRunKFv2[], selectedId) => { const selectedRun = runs.find((run) => run.run_id === selectedId); @@ -74,9 +97,14 @@ const PipelineRunTable: React.FC = ({ return acc; }, []); - const restoreButtonTooltipRef = React.useRef(null); const isExperimentArchived = useContextExperimentArchived(); + const isExperimentsEnabled = isExperimentsAvailable && experimentId; + const metricsColumnNames = useMetricColumnNames(experimentId ?? '', metricsNames); + + const columns = isExperimentsEnabled + ? getExperimentRunColumns(metricsColumnNames) + : pipelineRunColumns; const primaryToolbarAction = React.useMemo(() => { if (runType === PipelineRunType.ARCHIVED) { @@ -125,7 +153,7 @@ const PipelineRunTable: React.FC = ({ ]); const compareRunsAction = - isExperimentsAvailable && experimentId && !isExperimentArchived ? ( + isExperimentsEnabled && !isExperimentArchived ? ( + ))} /> )} variant={TableVariant.compact} - getColumnSort={getTableColumnSort({ columns: pipelineRunColumns, ...tableProps })} + getColumnSort={getTableColumnSort({ + columns, + ...tableProps, + })} data-testid={`${runType}-runs-table`} id={`${runType}-runs-table`} /> @@ -253,8 +318,56 @@ const PipelineRunTable: React.FC = ({ }} /> )} + {isCustomColModalOpen && ( + ({ + id: metricName, + content: metricName, + props: { checked: metricsColumnNames.includes(metricName) }, + }))} + onClose={() => setIsCustomColModalOpen(false)} + /> + )} ); }; +const PipelineRunTable: React.FC = ({ runs, page, ...props }) => { + const [runArtifacts, runArtifactsLoaded, runArtifactsError] = useGetArtifactsByRuns(runs); + const metricsNames = new Set(); + + const runsWithMetrics = runs.map((run) => ({ + ...run, + metrics: runArtifacts.reduce((acc: ArtifactProperty[], runArtifactseMap) => { + const artifacts = Object.entries(runArtifactseMap).find( + ([runId]) => run.run_id === runId, + )?.[1]; + + artifacts?.forEach((artifact) => { + if (artifact.getType() === ArtifactType.METRICS) { + const artifactProperties = getArtifactProperties(artifact); + + artifactProperties.map((artifactProp) => metricsNames.add(artifactProp.name)); + acc.push(...artifactProperties); + } + }); + + return acc; + }, []), + })); + + return ( + + ); +}; + export default PipelineRunTable; diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx b/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx index 0569ffaa88..1b12258e4f 100644 --- a/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/PipelineRunTableRow.tsx @@ -27,6 +27,7 @@ type PipelineRunTableRowProps = { checkboxProps: Omit, 'id'>; onDelete?: () => void; run: PipelineRunKFv2; + customCells?: React.ReactNode; hasExperiments?: boolean; hasRowActions?: boolean; }; @@ -35,6 +36,7 @@ const PipelineRunTableRow: React.FC = ({ hasRowActions = true, hasExperiments = true, checkboxProps, + customCells, onDelete, run, }) => { @@ -123,7 +125,17 @@ const PipelineRunTableRow: React.FC = ({ return ( - {hasExperiments && } @@ -144,6 +156,7 @@ const PipelineRunTableRow: React.FC = ({ + {customCells} {hasRowActions && (
+ {!artifactsLoaded && !artifactsError ? ( + + ) : ( + run.metrics.find((metric) => metric.name === metricName)?.value ?? ( + + ) + )} +
+ diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/UnavailableMetricValue.tsx b/frontend/src/concepts/pipelines/content/tables/pipelineRun/UnavailableMetricValue.tsx new file mode 100644 index 0000000000..6425515ba2 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/UnavailableMetricValue.tsx @@ -0,0 +1,24 @@ +import React from 'react'; + +import { Popover, Alert, Icon } from '@patternfly/react-core'; +import { ExclamationTriangleIcon } from '@patternfly/react-icons'; + +export const UnavailableMetricValue: React.FC = () => ( + + } + bodyContent={ + <> + This run does not support this metric type, and may have been generated from a different + pipeline version. To move this run, clone it to an appropriate experiment, then archive and + delete it from this experiment. + + } + > + + + + +); diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/types.ts b/frontend/src/concepts/pipelines/content/tables/pipelineRun/types.ts new file mode 100644 index 0000000000..250109409b --- /dev/null +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/types.ts @@ -0,0 +1,4 @@ +import { PipelineRunKFv2 } from '~/concepts/pipelines/kfTypes'; +import { ArtifactProperty } from '~/concepts/pipelines/content/pipelinesDetails/pipelineRun/artifacts/types'; + +export type RunWithMetrics = PipelineRunKFv2 & { metrics: ArtifactProperty[] }; diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/useMetricColumnNames.ts b/frontend/src/concepts/pipelines/content/tables/pipelineRun/useMetricColumnNames.ts new file mode 100644 index 0000000000..14c196e7d4 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/useMetricColumnNames.ts @@ -0,0 +1,38 @@ +import React from 'react'; +import { getMetricsColumnsLocalStorageKey } from './utils'; + +export const useMetricColumnNames = (experimentId: string, metricsNames: Set): string[] => { + const metricsColumnsLocalStorageKey = getMetricsColumnsLocalStorageKey(experimentId); + const metricsColumnsLocalStorageItem = localStorage.getItem(metricsColumnsLocalStorageKey) ?? ''; + const storedMetricsColumnNames: string[] | undefined = metricsColumnsLocalStorageItem + ? JSON.parse(metricsColumnsLocalStorageItem) + : undefined; + const [firstDefaultMetricColumn, secondDefaultMetricColumn] = [...metricsNames]; + + // Defaults include at least 1 and no more than 2 metrics. + const defaultMetricsColumnNames = React.useMemo( + () => [ + ...(firstDefaultMetricColumn ? [firstDefaultMetricColumn] : []), + ...(secondDefaultMetricColumn ? [secondDefaultMetricColumn] : []), + ], + [firstDefaultMetricColumn, secondDefaultMetricColumn], + ); + const metricsColumnNames = storedMetricsColumnNames ?? defaultMetricsColumnNames; + + // Set default metric columns in localStorage when no prior stored + // columns exist and at least 1 metric exists to use as a default. + React.useEffect(() => { + if ( + metricsColumnsLocalStorageKey && + !storedMetricsColumnNames && + defaultMetricsColumnNames.length + ) { + localStorage.setItem( + metricsColumnsLocalStorageKey, + JSON.stringify(defaultMetricsColumnNames), + ); + } + }, [defaultMetricsColumnNames, metricsColumnsLocalStorageKey, storedMetricsColumnNames]); + + return metricsColumnNames; +}; diff --git a/frontend/src/concepts/pipelines/content/tables/pipelineRun/utils.ts b/frontend/src/concepts/pipelines/content/tables/pipelineRun/utils.ts new file mode 100644 index 0000000000..f18b016c88 --- /dev/null +++ b/frontend/src/concepts/pipelines/content/tables/pipelineRun/utils.ts @@ -0,0 +1,3 @@ +export function getMetricsColumnsLocalStorageKey(experimentId: string): string { + return `metrics-columns-${experimentId}`; +}