From 3c2094d9c62411e623b11fa38ff0efb6220b7d1c Mon Sep 17 00:00:00 2001 From: Mike Turley Date: Wed, 1 May 2024 16:20:47 -0400 Subject: [PATCH] Model Details: add editable properties section, set up API patches for description, labels and properties Signed-off-by: Mike Turley --- .../src/__mocks__/mockRegisteredModelsList.ts | 44 ++--- .../DashboardDescriptionListGroup.tsx | 2 +- .../EditableLabelsDescriptionListGroup.tsx | 20 +- .../EditableTextDescriptionListGroup.tsx | 2 +- frontend/src/concepts/modelRegistry/types.ts | 53 +++++- .../screens/ModelDetailsView.tsx | 169 ++++++++--------- .../ModelPropertiesDescriptionListGroup.tsx | 108 +++++++++++ .../screens/ModelPropertiesTableRow.tsx | 172 ++++++++++++++++++ .../screens/__tests__/utils.spec.ts | 65 ++++++- .../src/pages/modelRegistry/screens/utils.ts | 101 +++++++++- frontend/src/pages/projects/types.ts | 6 +- frontend/src/types.ts | 5 + 12 files changed, 614 insertions(+), 133 deletions(-) create mode 100644 frontend/src/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx create mode 100644 frontend/src/pages/modelRegistry/screens/ModelPropertiesTableRow.tsx 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.externalID} + + + + + + + + + + + + + + + ); +}; 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..2181ae6235 --- /dev/null +++ b/frontend/src/pages/modelRegistry/screens/ModelPropertiesDescriptionListGroup.tsx @@ -0,0 +1,108 @@ +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 DashboardDescriptionListGroup from '~/components/DashboardDescriptionListGroup'; +import { ModelRegistryCustomProperties } from '~/concepts/modelRegistry/types'; +import ModelPropertiesTableRow from './ModelPropertiesTableRow'; +import { getProperties, getUpdatedProperties } 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); + + const filteredProperties = getProperties(customProperties); + const allExistingKeys = Object.keys(customProperties); + + const requiredAsterisk = ( + + ); + + return ( + } + iconPosition="start" + isDisabled={isAdding || isSavingEdits} + onClick={() => setIsAdding(true)} + > + Add property + + } + isEmpty={!isAdding && Object.keys(filteredProperties).length === 0} + contentWhenEmpty="No properties" + > + + + + + + + + + {Object.keys(filteredProperties).map((key) => ( + setIsEditingKey(key, isEditing)} + isSavingEdits={isSavingEdits} + setIsSavingEdits={setIsSavingEdits} + saveEditedProperty={(oldKey, newPair) => + saveEditedCustomProperties( + getUpdatedProperties({ customProperties, op: 'update', oldKey, newPair }), + ) + } + deleteProperty={(oldKey) => + saveEditedCustomProperties( + getUpdatedProperties({ customProperties, op: 'delete', oldKey }), + ) + } + /> + ))} + {isAdding && ( + + saveEditedCustomProperties( + getUpdatedProperties({ customProperties, op: 'create', newPair }), + ) + } + /> + )} + +
Key {isEditingSomeRow && requiredAsterisk}Value {isEditingSomeRow && requiredAsterisk} +
+ + ); +}; + +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..5247254be0 --- /dev/null +++ b/frontend/src/pages/modelRegistry/screens/ModelPropertiesTableRow.tsx @@ -0,0 +1,172 @@ +import * as React from 'react'; +import { ActionsColumn, Td, Tr } from '@patternfly/react-table'; +import { + ActionList, + ActionListItem, + Button, + 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 keyIsTaken = unsavedKey !== key && allExistingKeys.includes(unsavedKey); + + 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={keyIsTaken ? 'error' : 'default'} + /> + {keyIsTaken && ( + + + + Key must not match an existing property key or label + + + + )} + + ) : ( + key + )} + + + {isEditing ? ( + setUnsavedValue(str)} + /> + ) : ( + 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..4a89497d45 100644 --- a/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts +++ b/frontend/src/pages/modelRegistry/screens/__tests__/utils.spec.ts @@ -1,30 +1,75 @@ /* eslint-disable camelcase */ -import { RegisteredModel } from '~/concepts/modelRegistry/types'; -import { getLabels } from '~/pages/modelRegistry/screens/utils'; +import { + ModelRegistryCustomProperties, + ModelRegistryStringCustomProperties, + ModelRegistryMetadataType, +} from '~/concepts/modelRegistry/types'; +import { getLabels, getProperties } 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('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({}); + }); +}); diff --git a/frontend/src/pages/modelRegistry/screens/utils.ts b/frontend/src/pages/modelRegistry/screens/utils.ts index 5e9dd9f93f..9855c141c3 100644 --- a/frontend/src/pages/modelRegistry/screens/utils.ts +++ b/frontend/src/pages/modelRegistry/screens/utils.ts @@ -1,11 +1,102 @@ import { SearchType } from '~/concepts/dashboard/DashboardSearchField'; -import { ModelRegistryBase, ModelVersion } from '~/concepts/modelRegistry/types'; +import { + ModelRegistryBase, + ModelRegistryCustomProperties, + ModelRegistryMetadataType, + ModelRegistryStringCustomProperties, + ModelVersion, + RegisteredModel, +} 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. +// TODO unit tests +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 +// TODO unit test +export const getUpdatedProperties = ( + 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 should not be necessary if the backend properly merges objects on PATCH requests +// TODO unit test +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; +};