From d3b8faac76cbc8bc0c527387a0a10d4e9f286a50 Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Thu, 15 Aug 2024 09:24:34 -0400 Subject: [PATCH 1/6] changed the source of Owner and updated field to be searchable and sortable --- frontend/src/concepts/dashboard/DashboardSearchField.tsx | 1 + frontend/src/concepts/modelRegistry/types.ts | 4 ++-- .../screens/ModelVersions/ModelDetailsView.tsx | 7 +++---- .../pages/modelRegistry/screens/RegisterModel/utils.ts | 1 + .../screens/RegisteredModels/RegisteredModelListView.tsx | 2 +- .../screens/RegisteredModels/RegisteredModelTableRow.tsx | 8 ++++++++ .../RegisteredModels/RegisteredModelsTableColumns.ts | 6 +++--- frontend/src/pages/modelRegistry/screens/utils.ts | 5 ++--- 8 files changed, 21 insertions(+), 13 deletions(-) diff --git a/frontend/src/concepts/dashboard/DashboardSearchField.tsx b/frontend/src/concepts/dashboard/DashboardSearchField.tsx index 4413fd4960..ac070e8750 100644 --- a/frontend/src/concepts/dashboard/DashboardSearchField.tsx +++ b/frontend/src/concepts/dashboard/DashboardSearchField.tsx @@ -19,6 +19,7 @@ export enum SearchType { IDENTIFIER = 'Identifier', KEYWORD = 'Keyword', AUTHOR = 'Author', + OWNER = 'Owner', } type DashboardSearchFieldProps = { diff --git a/frontend/src/concepts/modelRegistry/types.ts b/frontend/src/concepts/modelRegistry/types.ts index d601a97409..e0dc58a031 100644 --- a/frontend/src/concepts/modelRegistry/types.ts +++ b/frontend/src/concepts/modelRegistry/types.ts @@ -85,6 +85,8 @@ export type ModelRegistryBase = { id: string; name: string; externalID?: string; + author?: string; + owner?: string; description?: string; createTimeSinceEpoch?: string; lastUpdateTimeSinceEpoch: string; @@ -94,7 +96,6 @@ export type ModelRegistryBase = { export type ModelArtifact = ModelRegistryBase & { uri?: string; state?: ModelArtifactState; - author?: string; modelFormatName?: string; storageKey?: string; storagePath?: string; @@ -105,7 +106,6 @@ export type ModelArtifact = ModelRegistryBase & { export type ModelVersion = ModelRegistryBase & { state?: ModelState; - author?: string; registeredModelId: string; }; diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx index 731054af39..09c8ffc6e2 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx @@ -1,11 +1,10 @@ import * as React from 'react'; -import { ClipboardCopy, DescriptionList, Flex, FlexItem } from '@patternfly/react-core'; +import { ClipboardCopy, DescriptionList, Flex, FlexItem, Text } from '@patternfly/react-core'; import { RegisteredModel } from '~/concepts/modelRegistry/types'; import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegistryContext'; import DashboardDescriptionListGroup from '~/components/DashboardDescriptionListGroup'; import EditableTextDescriptionListGroup from '~/components/EditableTextDescriptionListGroup'; import EditableLabelsDescriptionListGroup from '~/components/EditableLabelsDescriptionListGroup'; -import RegisteredModelAuthor from '~/pages/modelRegistry/screens/RegisteredModels/RegisteredModelAuthor'; import ModelTimestamp from '~/pages/modelRegistry/screens/components/ModelTimestamp'; import { getLabels, @@ -82,8 +81,8 @@ const ModelDetailsView: React.FC = ({ registeredModel: rm {rm.id} - - + + {rm.author || '-'} = ({ const [searchType, setSearchType] = React.useState(SearchType.KEYWORD); const [search, setSearch] = React.useState(''); - const searchTypes = React.useMemo(() => [SearchType.KEYWORD], []); // TODO Add owner once RHOAIENG-7566 is completed. + const searchTypes = React.useMemo(() => [SearchType.KEYWORD, SearchType.OWNER], []); if (unfilteredRegisteredModels.length === 0) { return ( diff --git a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx index 0eac70c7f3..21a31e1836 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx @@ -14,7 +14,10 @@ import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegi import { ArchiveRegisteredModelModal } from '~/pages/modelRegistry/screens/components/ArchiveRegisteredModelModal'; import { getPatchBodyForRegisteredModel } from '~/pages/modelRegistry/screens/utils'; import { RestoreRegisteredModelModal } from '~/pages/modelRegistry/screens/components/RestoreRegisteredModel'; +<<<<<<< HEAD import RegisteredModelAuthor from './RegisteredModelAuthor'; +======= +>>>>>>> b0f9e160 (changed the source of Owner and updated field to be searchable and sortable) type RegisteredModelTableRowProps = { registeredModel: RegisteredModel; @@ -76,8 +79,13 @@ const RegisteredModelTableRow: React.FC = ({ +<<<<<<< HEAD +======= + + {rm.owner || '-'} +>>>>>>> b0f9e160 (changed the source of Owner and updated field to be searchable and sortable) diff --git a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelsTableColumns.ts b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelsTableColumns.ts index 2d44626882..b367f9ad74 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelsTableColumns.ts +++ b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelsTableColumns.ts @@ -24,9 +24,9 @@ export const rmColumns: SortableData[] = [ }, }, { - field: 'author', - label: 'Author', - sortable: false, // TODO Add sortable once RHOAIENG-7566 is completed. + field: 'owner', + label: 'Owner', + sortable: true, info: { tooltip: 'The owner is the user who registered the model.', tooltipProps: { diff --git a/frontend/src/pages/modelRegistry/screens/utils.ts b/frontend/src/pages/modelRegistry/screens/utils.ts index 1a24d116b1..e48cd1175c 100644 --- a/frontend/src/pages/modelRegistry/screens/utils.ts +++ b/frontend/src/pages/modelRegistry/screens/utils.ts @@ -154,9 +154,8 @@ export const filterRegisteredModels = ( (rm.description && rm.description.toLowerCase().includes(search.toLowerCase())) ); - case SearchType.AUTHOR: - // TODO Implement owner search functionality once RHOAIENG-7566 is completed. - return; + case SearchType.OWNER: + return rm.owner && rm.owner.toLowerCase().includes(search.toLowerCase()); default: return true; From 06caa32562cf0ae6d408b1e4354129c996112c3e Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Thu, 15 Aug 2024 09:49:16 -0400 Subject: [PATCH 2/6] fixed lint and test --- .../cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts index d9220ce847..103eab1e95 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts @@ -178,7 +178,14 @@ describe('Model Registry core', () => { .contains( 'A machine learning model trained to detect fraudulent transactions in financial data', ); +<<<<<<< HEAD registeredModelRow.findAuthor().contains('Author 1'); +======= + registeredModelRow.findOwner().should(($owner) => { + const text = $owner.text(); + expect(text).to.be.oneOf(['Author 1', '-']); + }); +>>>>>>> 8ca179e7 (fixed lint and test) // Label popover registeredModelRow.findLabelPopoverText().contains('2 more'); From eeb5deb8ec4310051e4d8b66c818513352c76955 Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Thu, 15 Aug 2024 14:24:40 -0400 Subject: [PATCH 3/6] updated conflicts --- frontend/src/__mocks__/mockRegisteredModel.ts | 6 ++++++ .../tests/mocked/modelRegistry/modelRegistry.cy.ts | 9 +++++---- .../pages/modelRegistry/screens/__tests__/utils.spec.ts | 4 ++++ 3 files changed, 15 insertions(+), 4 deletions(-) diff --git a/frontend/src/__mocks__/mockRegisteredModel.ts b/frontend/src/__mocks__/mockRegisteredModel.ts index 0ba8a8c70c..404bc59c8f 100644 --- a/frontend/src/__mocks__/mockRegisteredModel.ts +++ b/frontend/src/__mocks__/mockRegisteredModel.ts @@ -4,6 +4,8 @@ import { createModelRegistryLabelsObject } from './utils'; type MockRegisteredModelType = { id?: string; name?: string; + author?: string; + owner?: string; state?: ModelState; description?: string; labels?: string[]; @@ -11,6 +13,8 @@ type MockRegisteredModelType = { export const mockRegisteredModel = ({ name = 'test', + author = 'Author 1', + owner = 'Author 1', state = ModelState.LIVE, description = '', labels = [], @@ -23,5 +27,7 @@ export const mockRegisteredModel = ({ lastUpdateTimeSinceEpoch: '1710404288975', name, state, + author, + owner, customProperties: createModelRegistryLabelsObject(labels), }); diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts index 103eab1e95..18f99292b2 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts @@ -66,10 +66,7 @@ const initIntercepts = ({ ], }), ], - modelVersions = [ - mockModelVersion({ author: 'Author 1' }), - mockModelVersion({ name: 'model version' }), - ], + modelVersions = [mockModelVersion({ name: 'model version' })], allowed = true, }: HandlersProps) => { cy.interceptOdh( @@ -178,9 +175,13 @@ describe('Model Registry core', () => { .contains( 'A machine learning model trained to detect fraudulent transactions in financial data', ); +<<<<<<< HEAD <<<<<<< HEAD registeredModelRow.findAuthor().contains('Author 1'); ======= +======= + registeredModelRow.findOwner().contains('Author 1'); +>>>>>>> f9da61fb (updated tests) registeredModelRow.findOwner().should(($owner) => { const text = $owner.text(); expect(text).to.be.oneOf(['Author 1', '-']); diff --git a/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts b/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts index a281ea9899..1ea633a059 100644 --- a/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts +++ b/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts @@ -270,6 +270,8 @@ describe('getPatchBody', () => { it('returns a given RegisteredModel with id/name/timestamps removed, customProperties updated and other values unchanged', () => { const registeredModel = mockRegisteredModel({ id: '1', + author: 'Author 1', + owner: 'Author 1', name: 'test-model', description: 'Description here', labels: [], @@ -286,9 +288,11 @@ describe('getPatchBody', () => { ); expect(result).toEqual({ description: 'Description here', + author: 'Author 1', customProperties: { label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, }, + owner: 'Author 1', state: ModelState.LIVE, externalID: '1234132asdfasdf', } satisfies Partial); From 24fdd2e9027e7557ba4279a36f3d30fc79db4fac Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Thu, 15 Aug 2024 14:31:19 -0400 Subject: [PATCH 4/6] take 2 --- .../cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts index 18f99292b2..2265324edd 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts @@ -181,12 +181,15 @@ describe('Model Registry core', () => { ======= ======= registeredModelRow.findOwner().contains('Author 1'); +<<<<<<< HEAD >>>>>>> f9da61fb (updated tests) registeredModelRow.findOwner().should(($owner) => { const text = $owner.text(); expect(text).to.be.oneOf(['Author 1', '-']); }); >>>>>>> 8ca179e7 (fixed lint and test) +======= +>>>>>>> 05749f70 (removed assertion) // Label popover registeredModelRow.findLabelPopoverText().contains('2 more'); From e98646db7e33ccd0141ef732512c3ecd3179844c Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Thu, 15 Aug 2024 16:29:43 -0400 Subject: [PATCH 5/6] ' ' --- frontend/src/__mocks__/mockRegisteredModel.ts | 3 --- .../cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts | 5 ++++- .../tests/mocked/modelRegistry/registeredModelArchive.cy.ts | 4 ++++ frontend/src/concepts/modelRegistry/types.ts | 5 +++-- .../modelRegistry/screens/ModelVersions/ModelDetailsView.tsx | 2 +- .../src/pages/modelRegistry/screens/RegisterModel/utils.ts | 2 +- .../src/pages/modelRegistry/screens/__tests__/utils.spec.ts | 2 -- 7 files changed, 13 insertions(+), 10 deletions(-) diff --git a/frontend/src/__mocks__/mockRegisteredModel.ts b/frontend/src/__mocks__/mockRegisteredModel.ts index 404bc59c8f..7a34f843d1 100644 --- a/frontend/src/__mocks__/mockRegisteredModel.ts +++ b/frontend/src/__mocks__/mockRegisteredModel.ts @@ -4,7 +4,6 @@ import { createModelRegistryLabelsObject } from './utils'; type MockRegisteredModelType = { id?: string; name?: string; - author?: string; owner?: string; state?: ModelState; description?: string; @@ -13,7 +12,6 @@ type MockRegisteredModelType = { export const mockRegisteredModel = ({ name = 'test', - author = 'Author 1', owner = 'Author 1', state = ModelState.LIVE, description = '', @@ -27,7 +25,6 @@ export const mockRegisteredModel = ({ lastUpdateTimeSinceEpoch: '1710404288975', name, state, - author, owner, customProperties: createModelRegistryLabelsObject(labels), }); diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts index 2265324edd..495f4a5719 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts @@ -66,7 +66,10 @@ const initIntercepts = ({ ], }), ], - modelVersions = [mockModelVersion({ name: 'model version' })], + modelVersions = [ + mockModelVersion({ author: 'Author 1' }), + mockModelVersion({ name: 'model version' }), + ], allowed = true, }: HandlersProps) => { cy.interceptOdh( diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts index 8feea1aab0..1c8d9acf98 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/registeredModelArchive.cy.ts @@ -171,6 +171,7 @@ describe('Restoring archive model', () => { description: '', externalID: '1234132asdfasdf', state: 'LIVE', + owner: 'Author 1', }); }); }); @@ -200,6 +201,7 @@ describe('Restoring archive model', () => { description: '', externalID: '1234132asdfasdf', state: 'LIVE', + owner: 'Author 1', }); }); }); @@ -233,6 +235,7 @@ describe('Archiving model', () => { description: '', externalID: '1234132asdfasdf', state: 'ARCHIVED', + owner: 'Author 1', }); }); }); @@ -266,6 +269,7 @@ describe('Archiving model', () => { description: '', externalID: '1234132asdfasdf', state: 'ARCHIVED', + owner: 'Author 1', }); }); }); diff --git a/frontend/src/concepts/modelRegistry/types.ts b/frontend/src/concepts/modelRegistry/types.ts index e0dc58a031..142809ce35 100644 --- a/frontend/src/concepts/modelRegistry/types.ts +++ b/frontend/src/concepts/modelRegistry/types.ts @@ -85,8 +85,6 @@ export type ModelRegistryBase = { id: string; name: string; externalID?: string; - author?: string; - owner?: string; description?: string; createTimeSinceEpoch?: string; lastUpdateTimeSinceEpoch: string; @@ -96,6 +94,7 @@ export type ModelRegistryBase = { export type ModelArtifact = ModelRegistryBase & { uri?: string; state?: ModelArtifactState; + author?: string; modelFormatName?: string; storageKey?: string; storagePath?: string; @@ -106,11 +105,13 @@ export type ModelArtifact = ModelRegistryBase & { export type ModelVersion = ModelRegistryBase & { state?: ModelState; + author?: string; registeredModelId: string; }; export type RegisteredModel = ModelRegistryBase & { state?: ModelState; + owner?: string; }; export type InferenceService = ModelRegistryBase & { diff --git a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx index 09c8ffc6e2..1bef66780c 100644 --- a/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx +++ b/frontend/src/pages/modelRegistry/screens/ModelVersions/ModelDetailsView.tsx @@ -82,7 +82,7 @@ const ModelDetailsView: React.FC = ({ registeredModel: rm - {rm.author || '-'} + {rm.owner || '-'} { it('returns a given RegisteredModel with id/name/timestamps removed, customProperties updated and other values unchanged', () => { const registeredModel = mockRegisteredModel({ id: '1', - author: 'Author 1', owner: 'Author 1', name: 'test-model', description: 'Description here', @@ -288,7 +287,6 @@ describe('getPatchBody', () => { ); expect(result).toEqual({ description: 'Description here', - author: 'Author 1', customProperties: { label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, }, From e1fa4d6a2c13bbe27b939523520a1d25a3f88237 Mon Sep 17 00:00:00 2001 From: Yulia Krimerman Date: Fri, 16 Aug 2024 13:16:00 -0400 Subject: [PATCH 6/6] fixed last issues --- .../cypress/cypress/pages/modelRegistry.ts | 4 ++-- .../tests/mocked/modelRegistry/modelRegistry.cy.ts | 14 -------------- .../RegisteredModels/RegisteredModelTableRow.tsx | 9 --------- 3 files changed, 2 insertions(+), 25 deletions(-) diff --git a/frontend/src/__tests__/cypress/cypress/pages/modelRegistry.ts b/frontend/src/__tests__/cypress/cypress/pages/modelRegistry.ts index 8a9507b78f..1b9c2a1d9c 100644 --- a/frontend/src/__tests__/cypress/cypress/pages/modelRegistry.ts +++ b/frontend/src/__tests__/cypress/cypress/pages/modelRegistry.ts @@ -30,8 +30,8 @@ class ModelRegistryTableRow extends TableRow { return this.find().findByTestId('description'); } - findAuthor() { - return this.find().findByTestId('registered-model-author'); + findOwner() { + return this.find().findByTestId('registered-model-owner'); } findLabelPopoverText() { diff --git a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts index 495f4a5719..6f39297340 100644 --- a/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts +++ b/frontend/src/__tests__/cypress/cypress/tests/mocked/modelRegistry/modelRegistry.cy.ts @@ -178,21 +178,7 @@ describe('Model Registry core', () => { .contains( 'A machine learning model trained to detect fraudulent transactions in financial data', ); -<<<<<<< HEAD -<<<<<<< HEAD - registeredModelRow.findAuthor().contains('Author 1'); -======= -======= registeredModelRow.findOwner().contains('Author 1'); -<<<<<<< HEAD ->>>>>>> f9da61fb (updated tests) - registeredModelRow.findOwner().should(($owner) => { - const text = $owner.text(); - expect(text).to.be.oneOf(['Author 1', '-']); - }); ->>>>>>> 8ca179e7 (fixed lint and test) -======= ->>>>>>> 05749f70 (removed assertion) // Label popover registeredModelRow.findLabelPopoverText().contains('2 more'); diff --git a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx index 21a31e1836..86eafda0bf 100644 --- a/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx +++ b/frontend/src/pages/modelRegistry/screens/RegisteredModels/RegisteredModelTableRow.tsx @@ -14,10 +14,6 @@ import { ModelRegistryContext } from '~/concepts/modelRegistry/context/ModelRegi import { ArchiveRegisteredModelModal } from '~/pages/modelRegistry/screens/components/ArchiveRegisteredModelModal'; import { getPatchBodyForRegisteredModel } from '~/pages/modelRegistry/screens/utils'; import { RestoreRegisteredModelModal } from '~/pages/modelRegistry/screens/components/RestoreRegisteredModel'; -<<<<<<< HEAD -import RegisteredModelAuthor from './RegisteredModelAuthor'; -======= ->>>>>>> b0f9e160 (changed the source of Owner and updated field to be searchable and sortable) type RegisteredModelTableRowProps = { registeredModel: RegisteredModel; @@ -79,13 +75,8 @@ const RegisteredModelTableRow: React.FC = ({ -<<<<<<< HEAD - - -======= {rm.owner || '-'} ->>>>>>> b0f9e160 (changed the source of Owner and updated field to be searchable and sortable)