diff --git a/frontend/src/__mocks__/mockRegisteredModelsList.ts b/frontend/src/__mocks__/mockRegisteredModelsList.ts index 966055176a..ad367e199f 100644 --- a/frontend/src/__mocks__/mockRegisteredModelsList.ts +++ b/frontend/src/__mocks__/mockRegisteredModelsList.ts @@ -1,5 +1,5 @@ /* eslint-disable camelcase */ -import { RegisteredModelList } from '~/concepts/modelRegistry/types'; +import { ModelRegistryMetadataType, RegisteredModelList } from '~/concepts/modelRegistry/types'; import { mockRegisteredModel } from './mockRegisteredModel'; export const mockRegisteredModelList = ({ @@ -14,27 +14,27 @@ export const mockRegisteredModelList = ({ 'A machine learning model trained to detect fraudulent transactions in financial data', customProperties: { Financial: { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty', }, 'Financial data': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Fraud detection': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Test label': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Machine learning': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Next data to be overflow': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, }, @@ -43,23 +43,23 @@ export const mockRegisteredModelList = ({ name: 'Credit Scoring', customProperties: { 'Credit Score Predictor': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Creditworthiness scoring system': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Default Risk Analyzer': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Portfolio Management': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Risk Assessment': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, }, @@ -70,44 +70,44 @@ export const mockRegisteredModelList = ({ 'A machine learning model trained to detect fraudulent transactions in financial data', customProperties: { 'Testing label': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, Financial: { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty', }, 'Financial data': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Fraud detection': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Long label data to be truncated abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc abc': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Machine learning': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Next data to be overflow': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Label x': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Label y': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, 'Label z': { - metadataType: 'MetadataStringValue', + metadataType: ModelRegistryMetadataType.STRING, string_value: '', }, }, diff --git a/frontend/src/components/DashboardDescriptionListGroup.tsx b/frontend/src/components/DashboardDescriptionListGroup.tsx index 8f40df0d70..9a461f8c6b 100644 --- a/frontend/src/components/DashboardDescriptionListGroup.tsx +++ b/frontend/src/components/DashboardDescriptionListGroup.tsx @@ -69,7 +69,7 @@ const DashboardDescriptionListGroup: React.FC - } - isEmpty // TODO - contentWhenEmpty="No properties" - > - TODO properties here - - - - - - - - {rm.externalID} - - - - - - - - - - - - - - -); +const ModelDetailsView: React.FC = ({ registeredModel: rm }) => { + const { apiState, refreshAPIState } = React.useContext(ModelRegistryContext); + return ( + + + + + apiState.api + .patchRegisteredModel({}, getPatchBody(rm, { description: value }), rm.id) + .then(refreshAPIState) + } + /> + + apiState.api + .patchRegisteredModel( + {}, + getPatchBody(rm, { + customProperties: mergeUpdatedLabels(rm.customProperties, editedLabels), + }), + rm.id, + ) + .then(refreshAPIState) + } + /> + + apiState.api + .patchRegisteredModel( + {}, + getPatchBody(rm, { customProperties: editedProperties }), + rm.id, + ) + .then(refreshAPIState) + } + /> + + + + + + + {rm.id} + + + + + + + + + + + + + + + ); +}; export default ModelDetailsView; diff --git a/frontend/src/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx b/frontend/src/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx new file mode 100644 index 0000000000..c493a3babe --- /dev/null +++ b/frontend/src/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx @@ -0,0 +1,129 @@ +import * as React from 'react'; +import { Button } from '@patternfly/react-core'; +import { Table, Tbody, Th, Thead, Tr } from '@patternfly/react-table'; +import { PlusCircleIcon } from '@patternfly/react-icons'; +import text from '@patternfly/react-styles/css/utilities/Text/text'; +import spacing from '@patternfly/react-styles/css/utilities/Spacing/spacing'; +import DashboardDescriptionListGroup from '~/components/DashboardDescriptionListGroup'; +import { ModelRegistryCustomProperties } from '~/concepts/modelRegistry/types'; +import ModelPropertiesTableRow from './ModelPropertiesTableRow'; +import { getProperties, mergeUpdatedProperty } from './utils'; + +type ModelPropertiesDescriptionListGroupProps = { + customProperties: ModelRegistryCustomProperties; + saveEditedCustomProperties: (properties: ModelRegistryCustomProperties) => Promise; +}; + +const ModelPropertiesDescriptionListGroup: React.FC = ({ + customProperties = {}, + saveEditedCustomProperties, +}) => { + const [editingPropertyKeys, setEditingPropertyKeys] = React.useState([]); + const setIsEditingKey = (key: string, isEditing: boolean) => + setEditingPropertyKeys([ + ...editingPropertyKeys.filter((k) => k !== key), + ...(isEditing ? [key] : []), + ]); + const [isAdding, setIsAdding] = React.useState(false); + const isEditingSomeRow = isAdding || editingPropertyKeys.length > 0; + + const [isSavingEdits, setIsSavingEdits] = React.useState(false); + + // We only show string properties with a defined value (no labels or other property types) + const filteredProperties = getProperties(customProperties); + + const [isShowingMoreProperties, setIsShowingMoreProperties] = React.useState(false); + const keys = Object.keys(filteredProperties); + const needExpandControl = keys.length > 5; + const shownKeys = isShowingMoreProperties ? keys : keys.slice(0, 5); + const numHiddenKeys = keys.length - shownKeys.length; + + // Includes keys reserved by non-string properties and labels + const allExistingKeys = Object.keys(customProperties); + + const requiredAsterisk = ( + + ); + + return ( + } + iconPosition="start" + isDisabled={isAdding || isSavingEdits} + onClick={() => setIsAdding(true)} + > + Add property + + } + isEmpty={!isAdding && keys.length === 0} + contentWhenEmpty="No properties" + > + + + + + + + + + {shownKeys.map((key) => ( + setIsEditingKey(key, isEditing)} + isSavingEdits={isSavingEdits} + setIsSavingEdits={setIsSavingEdits} + saveEditedProperty={(oldKey, newPair) => + saveEditedCustomProperties( + mergeUpdatedProperty({ customProperties, op: 'update', oldKey, newPair }), + ) + } + deleteProperty={(oldKey) => + saveEditedCustomProperties( + mergeUpdatedProperty({ customProperties, op: 'delete', oldKey }), + ) + } + /> + ))} + {isAdding && ( + + saveEditedCustomProperties( + mergeUpdatedProperty({ customProperties, op: 'create', newPair }), + ) + } + /> + )} + +
Key {isEditingSomeRow && requiredAsterisk}Value {isEditingSomeRow && requiredAsterisk} +
+ {needExpandControl && ( + + )} +
+ ); +}; + +export default ModelPropertiesDescriptionListGroup; diff --git a/frontend/src/pages/modelRegistry/screens/ModelPropertiesTableRow.tsx b/frontend/src/pages/modelRegistry/screens/ModelPropertiesTableRow.tsx new file mode 100644 index 0000000000..c3d75a2cd0 --- /dev/null +++ b/frontend/src/pages/modelRegistry/screens/ModelPropertiesTableRow.tsx @@ -0,0 +1,187 @@ +import * as React from 'react'; +import { ActionsColumn, Td, Tr } from '@patternfly/react-table'; +import { + ActionList, + ActionListItem, + Button, + ExpandableSection, + FormHelperText, + HelperText, + HelperTextItem, + TextInput, +} from '@patternfly/react-core'; +import { CheckIcon, TimesIcon } from '@patternfly/react-icons'; +import { KeyValuePair } from '~/types'; +import { EitherNotBoth } from '~/typeHelpers'; + +type ModelPropertiesTableRowProps = { + allExistingKeys: string[]; + setIsEditing: (isEditing: boolean) => void; + isSavingEdits: boolean; + setIsSavingEdits: (isSaving: boolean) => void; + saveEditedProperty: (oldKey: string, newPair: KeyValuePair) => Promise; +} & EitherNotBoth< + { isAddRow: true }, + { + isEditing: boolean; + keyValuePair: KeyValuePair; + deleteProperty: (key: string) => Promise; + } +>; + +const ModelPropertiesTableRow: React.FC = ({ + isAddRow, + isEditing = isAddRow, + keyValuePair = { key: '', value: '' }, + deleteProperty = () => Promise.resolve(), + allExistingKeys, + setIsEditing, + isSavingEdits, + setIsSavingEdits, + saveEditedProperty, +}) => { + const { key, value } = keyValuePair; + const [unsavedKey, setUnsavedKey] = React.useState(key); + const [unsavedValue, setUnsavedValue] = React.useState(value); + + const [isValueExpanded, setIsValueExpanded] = React.useState(false); + + let keyValidationError: string | null = null; + if (unsavedKey !== key && allExistingKeys.includes(unsavedKey)) { + keyValidationError = 'Key must not match an existing property key or label'; + } else if (unsavedKey.length > 63) { + keyValidationError = "Key text can't exceed 63 characters"; + } + + const clearUnsavedInputs = () => { + setUnsavedKey(key); + setUnsavedValue(value); + }; + + const onEditClick = () => { + clearUnsavedInputs(); + setIsEditing(true); + }; + + const onDeleteClick = async () => { + setIsSavingEdits(true); + try { + await deleteProperty(key); + } finally { + setIsSavingEdits(false); + } + }; + + const onSaveEditsClick = async () => { + setIsSavingEdits(true); + try { + await saveEditedProperty(key, { key: unsavedKey, value: unsavedValue }); + } finally { + setIsSavingEdits(false); + } + setIsEditing(false); + }; + + const onDiscardEditsClick = () => { + clearUnsavedInputs(); + setIsEditing(false); + }; + + return ( + + + {isEditing ? ( + <> + setUnsavedKey(str)} + validated={keyValidationError ? 'error' : 'default'} + /> + {keyValidationError && ( + + + {keyValidationError} + + + )} + + ) : ( + key + )} + + + {isEditing ? ( + setUnsavedValue(str)} + /> + ) : ( + setIsValueExpanded(isExpanded)} + isExpanded={isValueExpanded} + > + {value} + + )} + + + {isEditing ? ( + + + + + + + + + ) : ( + + )} + + + ); +}; + +export default ModelPropertiesTableRow; diff --git a/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts b/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts index 229f0bb1c8..e0fec49058 100644 --- a/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts +++ b/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts @@ -1,30 +1,301 @@ /* eslint-disable camelcase */ -import { RegisteredModel } from '~/concepts/modelRegistry/types'; -import { getLabels } from '~/pages/modelRegistry/screens/utils'; +import { mockModelVersion } from '~/__mocks__/mockModelVersion'; +import { mockRegisteredModel } from '~/__mocks__/mockRegisteredModel'; +import { + ModelRegistryCustomProperties, + ModelRegistryStringCustomProperties, + ModelRegistryMetadataType, + RegisteredModel, + ModelVersion, + RegisteredModelState, + ModelVersionState, +} from '~/concepts/modelRegistry/types'; +import { + getLabels, + getProperties, + mergeUpdatedProperty, + mergeUpdatedLabels, + getPatchBody, +} from '~/pages/modelRegistry/screens/utils'; describe('getLabels', () => { it('should return an empty array when customProperties is empty', () => { - const customProperties: RegisteredModel['customProperties'] = {}; + const customProperties: ModelRegistryCustomProperties = {}; const result = getLabels(customProperties); expect(result).toEqual([]); }); it('should return an array of keys with empty string values in customProperties', () => { - const customProperties: RegisteredModel['customProperties'] = { - label1: { string_value: '' }, - label2: { string_value: 'non-empty' }, - label3: { string_value: '' }, + const customProperties: ModelRegistryCustomProperties = { + label1: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' }, + label2: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' }, + label3: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' }, }; const result = getLabels(customProperties); expect(result).toEqual(['label1', 'label3']); }); it('should return an empty array when all values in customProperties are non-empty strings', () => { - const customProperties: RegisteredModel['customProperties'] = { - label1: { string_value: 'non-empty' }, - label2: { string_value: 'another-non-empty' }, + const customProperties: ModelRegistryCustomProperties = { + label1: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' }, + label2: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'another-non-empty' }, }; const result = getLabels(customProperties); expect(result).toEqual([]); }); }); + +describe('mergeUpdatedLabels', () => { + it('should return an empty object when customProperties and updatedLabels are empty', () => { + const customProperties: ModelRegistryCustomProperties = {}; + const result = mergeUpdatedLabels(customProperties, []); + expect(result).toEqual({}); + }); + + it('should return an unmodified object if updatedLabels match existing labels', () => { + const customProperties: ModelRegistryCustomProperties = { + someUnrelatedProp: { string_value: 'foo', metadataType: ModelRegistryMetadataType.STRING }, + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedLabels(customProperties, ['label1']); + expect(result).toEqual(customProperties); + }); + + it('should return an object with labels added', () => { + const customProperties: ModelRegistryCustomProperties = {}; + const result = mergeUpdatedLabels(customProperties, ['label1', 'label2']); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label2: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should return an object with labels removed', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label2: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label3: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label4: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedLabels(customProperties, ['label2', 'label4']); + expect(result).toEqual({ + label2: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label4: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should return an object with labels both added and removed', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label2: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label3: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedLabels(customProperties, ['label1', 'label3', 'label4']); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label3: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label4: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should not affect non-label properties on the object', () => { + const customProperties: ModelRegistryCustomProperties = { + someUnrelatedStrProp: { string_value: 'foo', metadataType: ModelRegistryMetadataType.STRING }, + someUnrelatedIntProp: { int_value: '3', metadataType: ModelRegistryMetadataType.INT }, + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label2: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedLabels(customProperties, ['label2', 'label3']); + expect(result).toEqual({ + someUnrelatedStrProp: { string_value: 'foo', metadataType: ModelRegistryMetadataType.STRING }, + someUnrelatedIntProp: { int_value: '3', metadataType: ModelRegistryMetadataType.INT }, + label2: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + label3: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); +}); + +describe('getProperties', () => { + it('should return an empty object when customProperties is empty', () => { + const customProperties: ModelRegistryCustomProperties = {}; + const result = getProperties(customProperties); + expect(result).toEqual({}); + }); + + it('should return a filtered object including only string properties with a non-empty value', () => { + const customProperties: ModelRegistryCustomProperties = { + property1: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' }, + property2: { + metadataType: ModelRegistryMetadataType.STRING, + string_value: 'another-non-empty', + }, + label1: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' }, + label2: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' }, + int1: { metadataType: ModelRegistryMetadataType.INT, int_value: '1' }, + int2: { metadataType: ModelRegistryMetadataType.INT, int_value: '2' }, + }; + const result = getProperties(customProperties); + expect(result).toEqual({ + property1: { metadataType: ModelRegistryMetadataType.STRING, string_value: 'non-empty' }, + property2: { + metadataType: ModelRegistryMetadataType.STRING, + string_value: 'another-non-empty', + }, + } satisfies ModelRegistryStringCustomProperties); + }); + + it('should return an empty object when all values in customProperties are empty strings or non-string values', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' }, + label2: { metadataType: ModelRegistryMetadataType.STRING, string_value: '' }, + int1: { metadataType: ModelRegistryMetadataType.INT, int_value: '1' }, + int2: { metadataType: ModelRegistryMetadataType.INT, int_value: '2' }, + }; + const result = getProperties(customProperties); + expect(result).toEqual({}); + }); +}); + +describe('mergeUpdatedProperty', () => { + it('should handle the create operation', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedProperty({ + customProperties, + op: 'create', + newPair: { key: 'prop2', value: 'val2' }, + }); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + prop2: { string_value: 'val2', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should handle the update operation without a key change', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedProperty({ + customProperties, + op: 'update', + oldKey: 'prop1', + newPair: { key: 'prop1', value: 'updatedVal1' }, + }); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'updatedVal1', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should handle the update operation with a key change', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedProperty({ + customProperties, + op: 'update', + oldKey: 'prop1', + newPair: { key: 'prop2', value: 'val2' }, + }); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop2: { string_value: 'val2', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should perform a create if using the update operation with an invalid oldKey', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedProperty({ + customProperties, + op: 'update', + oldKey: 'prop2', + newPair: { key: 'prop3', value: 'val3' }, + }); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + prop3: { string_value: 'val3', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should handle the delete operation', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + prop2: { string_value: 'val2', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedProperty({ + customProperties, + op: 'delete', + oldKey: 'prop2', + }); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); + + it('should do nothing if using the delete operation with an invalid oldKey', () => { + const customProperties: ModelRegistryCustomProperties = { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + }; + const result = mergeUpdatedProperty({ + customProperties, + op: 'delete', + oldKey: 'prop2', + }); + expect(result).toEqual({ + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + prop1: { string_value: 'val1', metadataType: ModelRegistryMetadataType.STRING }, + } satisfies ModelRegistryCustomProperties); + }); +}); + +describe('getPatchBody', () => { + it('returns a given RegisteredModel with id/name/timestamps removed, customProperties updated and other values unchanged', () => { + const registeredModel = mockRegisteredModel({ + id: '1', + name: 'test-model', + description: 'Description here', + customProperties: {}, + state: RegisteredModelState.LIVE, + }); + const result = getPatchBody(registeredModel, { + customProperties: { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + }, + }); + expect(result).toEqual({ + description: 'Description here', + customProperties: { + label1: { string_value: '', metadataType: ModelRegistryMetadataType.STRING }, + }, + state: RegisteredModelState.LIVE, + externalID: '1234132asdfasdf', + } satisfies Partial); + }); + + it('returns a given ModelVersion with id/name/timestamps removed, description updated and other values unchanged', () => { + const modelVersion = mockModelVersion({ + author: 'Test author', + registeredModelID: '1', + }); + const result = getPatchBody(modelVersion, { description: 'New description' }); + expect(result).toEqual({ + author: 'Test author', + registeredModelID: '1', + description: 'New description', + customProperties: {}, + state: ModelVersionState.ARCHIVED, + } satisfies Partial); + }); +}); diff --git a/frontend/src/pages/modelRegistry/screens/utils.ts b/frontend/src/pages/modelRegistry/screens/utils.ts index 5e9dd9f93f..21e47ca3b5 100644 --- a/frontend/src/pages/modelRegistry/screens/utils.ts +++ b/frontend/src/pages/modelRegistry/screens/utils.ts @@ -1,11 +1,99 @@ import { SearchType } from '~/concepts/dashboard/DashboardSearchField'; -import { ModelRegistryBase, ModelVersion } from '~/concepts/modelRegistry/types'; +import { + ModelRegistryBase, + ModelRegistryCustomProperties, + ModelRegistryMetadataType, + ModelRegistryStringCustomProperties, + ModelVersion, +} from '~/concepts/modelRegistry/types'; +import { KeyValuePair } from '~/types'; -//Retrieves the labels from customProperties that have non-empty string_value. -export const getLabels = ( +// Retrieves the labels from customProperties that have non-empty string_value. +export const getLabels = (customProperties: T): string[] => + Object.keys(customProperties).filter((key) => { + const prop = customProperties[key]; + return prop.metadataType === ModelRegistryMetadataType.STRING && prop.string_value === ''; + }); + +// Returns the customProperties object with an updated set of labels (non-empty string_value) without affecting other properties. +export const mergeUpdatedLabels = ( + customProperties: ModelRegistryCustomProperties, + updatedLabels: string[], +): ModelRegistryCustomProperties => { + const existingLabels = getLabels(customProperties); + const addedLabels = updatedLabels.filter((label) => !existingLabels.includes(label)); + const removedLabels = existingLabels.filter((label) => !updatedLabels.includes(label)); + const customPropertiesCopy = { ...customProperties }; + removedLabels.forEach((label) => { + delete customPropertiesCopy[label]; + }); + addedLabels.forEach((label) => { + customPropertiesCopy[label] = { + // eslint-disable-next-line camelcase + string_value: '', + metadataType: ModelRegistryMetadataType.STRING, + }; + }); + return customPropertiesCopy; +}; + +// Retrives the customProperties that are not labels (they have a defined string_value). +export const getProperties = ( customProperties: T, -): string[] => - Object.keys(customProperties).filter((key) => customProperties[key].string_value === ''); +): ModelRegistryStringCustomProperties => { + const initial: ModelRegistryStringCustomProperties = {}; + return Object.keys(customProperties).reduce((acc, key) => { + const prop = customProperties[key]; + if (prop.metadataType === ModelRegistryMetadataType.STRING && prop.string_value !== '') { + return { ...acc, [key]: prop }; + } + return acc; + }, initial); +}; + +// Returns the customProperties object with a single string property added, updated or deleted +export const mergeUpdatedProperty = ( + args: { customProperties: ModelRegistryCustomProperties } & ( + | { op: 'create'; newPair: KeyValuePair } + | { op: 'update'; oldKey: string; newPair: KeyValuePair } + | { op: 'delete'; oldKey: string } + ), +): ModelRegistryCustomProperties => { + const { op } = args; + const customPropertiesCopy = { ...args.customProperties }; + if (op === 'delete' || (op === 'update' && args.oldKey !== args.newPair.key)) { + delete customPropertiesCopy[args.oldKey]; + } + if (op === 'create' || op === 'update') { + const { key, value } = args.newPair; + customPropertiesCopy[key] = { + // eslint-disable-next-line camelcase + string_value: value, + metadataType: ModelRegistryMetadataType.STRING, + }; + } + return customPropertiesCopy; +}; + +// Returns a patch payload for a Model Registry object, retaining all mutable fields but excluding internal fields to prevent errors +// TODO this will not be necessary if the backend eventually merges objects on PATCH requests. +// see https://issues.redhat.com/browse/RHOAIENG-6652 +export const getPatchBody = ( + existingObj: T, + updates: Partial, +): Partial => { + const objCopy = { ...existingObj }; + const excludedKeys: (keyof T)[] = [ + 'id', + 'name', + 'createTimeSinceEpoch', + 'lastUpdateTimeSinceEpoch', + ]; + excludedKeys.forEach((key) => { + delete objCopy[key]; + }); + return { ...objCopy, ...updates }; +}; export const filteredmodelVersions = ( unfilteredmodelVersions: ModelVersion[], diff --git a/frontend/src/pages/projects/types.ts b/frontend/src/pages/projects/types.ts index 5962e71f42..ff238eaa44 100644 --- a/frontend/src/pages/projects/types.ts +++ b/frontend/src/pages/projects/types.ts @@ -2,6 +2,7 @@ import { K8sResourceCommon } from '@openshift/dynamic-plugin-sdk-utils'; import { ContainerResources, ImageStreamAndVersion, + KeyValuePair, NotebookSize, Toleration, TolerationSettings, @@ -121,10 +122,7 @@ export type DataConnection = export type AWSDataEntry = { key: AwsKeys; value: string }[]; -export type EnvVariableDataEntry = { - key: string; - value: string; -}; +export type EnvVariableDataEntry = KeyValuePair; export type EnvVariableData = { category: SecretCategory | ConfigMapCategory | null; diff --git a/frontend/src/types.ts b/frontend/src/types.ts index 68e9efd459..3417f76903 100644 --- a/frontend/src/types.ts +++ b/frontend/src/types.ts @@ -727,3 +727,8 @@ export enum ServingRuntimeAPIProtocol { REST = 'REST', GRPC = 'gRPC', } + +export type KeyValuePair = { + key: string; + value: string; +};