Merge pull request #2770 from mturley/RHOAIENG-2235-2-properties-editor

Model Details and Model Version Details: add editable properties section, set up API patches for description, labels and properties

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: '',
         },
       },

frontend/src/components/DashboardDescriptionListGroup.tsx

@@ -8,7 +8,6 @@ import {
   DescriptionListTerm,
   Split,
   SplitItem,
-  Text,
 } from '@patternfly/react-core';
 import text from '@patternfly/react-styles/css/utilities/Text/text';
 import { CheckIcon, PencilAltIcon, TimesIcon } from '@patternfly/react-icons';
@@ -70,7 +69,7 @@ const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps
                     </ActionListItem>
                     <ActionListItem>
                       <Button
-                        data-testid={`discard-edit-button-${title} `}
+                        data-testid={`discard-edit-button-${title}`}
                         aria-label={`Discard edits to ${title} `}
                         variant="plain"
                         onClick={onDiscardEditsClick}
@@ -100,13 +99,7 @@ const DashboardDescriptionListGroup: React.FC<DashboardDescriptionListGroupProps
         <DescriptionListTerm>{title}</DescriptionListTerm>
       )}
       <DescriptionListDescription className={isEmpty && !isEditing ? text.disabledColor_100 : ''}>
-        {isEditing ? (
-          contentWhenEditing
-        ) : isEmpty ? (
-          <Text style={{ color: '--pf-v5-global--Color--200' }}>{contentWhenEmpty}</Text>
-        ) : (
-          children
-        )}
+        {isEditing ? contentWhenEditing : isEmpty ? contentWhenEmpty : children}
       </DescriptionListDescription>
     </DescriptionListGroup>
   );

frontend/src/components/EditableLabelsDescriptionListGroup.tsx

@@ -20,14 +20,16 @@ type EditableTextDescriptionListGroupProps = Partial<
   Pick<DashboardDescriptionListGroupProps, 'title' | 'contentWhenEmpty'>
 > & {
   labels: string[];
-  saveEditedLabels: (labels: string[]) => Promise<void>;
+  saveEditedLabels: (labels: string[]) => Promise<unknown>;
+  allExistingKeys?: string[];
 };
 
 const EditableLabelsDescriptionListGroup: React.FC<EditableTextDescriptionListGroupProps> = ({
   title = 'Labels',
   contentWhenEmpty = 'No labels',
   labels,
   saveEditedLabels,
+  allExistingKeys = labels,
 }) => {
   const [isEditing, setIsEditing] = React.useState(false);
   const [unsavedLabels, setUnsavedLabels] = React.useState(labels);
@@ -54,10 +56,22 @@ const EditableLabelsDescriptionListGroup: React.FC<EditableTextDescriptionListGr
     setUnsavedLabels([...unsavedLabels, text]);
   };
 
+  // Don't allow a label that matches a non-label property key or another label (as they stand before saving)
+  // Note that this means if you remove a label and add it back before saving, that is valid
+  const reservedKeys = [
+    ...allExistingKeys.filter((key) => !labels.includes(key)),
+    ...unsavedLabels,
+  ];
+
   const [isAddLabelModalOpen, setIsAddLabelModalOpen] = React.useState(false);
   const [addLabelInputValue, setAddLabelInputValue] = React.useState('');
   const addLabelInputRef = React.useRef<HTMLInputElement>(null);
-  const addLabelInputTooLong = addLabelInputValue.length > 63;
+  let addLabelValidationError: string | null = null;
+  if (reservedKeys.includes(addLabelInputValue)) {
+    addLabelValidationError = 'Label must not match an existing label or property key';
+  } else if (addLabelInputValue.length > 63) {
+    addLabelValidationError = "Label text can't exceed 63 characters";
+  }
 
   const toggleAddLabelModal = () => {
     setAddLabelInputValue('');
@@ -68,7 +82,8 @@ const EditableLabelsDescriptionListGroup: React.FC<EditableTextDescriptionListGr
       addLabelInputRef.current.focus();
     }
   }, [isAddLabelModalOpen]);
-  const addLabelModalSubmitDisabled = !addLabelInputValue || addLabelInputTooLong;
+
+  const addLabelModalSubmitDisabled = !addLabelInputValue || !!addLabelValidationError;
   const submitAddLabelModal = (event?: React.FormEvent) => {
     event?.preventDefault();
     if (!addLabelModalSubmitDisabled) {
@@ -107,7 +122,11 @@ const EditableLabelsDescriptionListGroup: React.FC<EditableTextDescriptionListGr
                 editableProps={{ 'aria-label': `Editable label with text ${label}` }}
                 onClose={() => removeUnsavedLabel(label)}
                 closeBtnProps={{ isDisabled: isSavingEdits }}
-                onEditComplete={(_event, newText) => editUnsavedLabel(newText, index)}
+                onEditComplete={(_event, newText) => {
+                  if (!reservedKeys.includes(newText) && newText.length <= 63) {
+                    editUnsavedLabel(newText, index);
+                  }
+                }}
               >
                 {label}
               </Label>
@@ -172,13 +191,13 @@ const EditableLabelsDescriptionListGroup: React.FC<EditableTextDescriptionListGr
               }
               ref={addLabelInputRef}
               isRequired
-              validated={addLabelInputTooLong ? 'error' : 'default'}
+              validated={addLabelValidationError ? 'error' : 'default'}
             />
-            {addLabelInputTooLong && (
+            {addLabelValidationError && (
               <FormHelperText>
                 <HelperText>
                   <HelperTextItem icon={<ExclamationCircleIcon />} variant="error">
-                    Label text can&apos;t exceed 63 characters
+                    {addLabelValidationError}
                   </HelperTextItem>
                 </HelperText>
               </FormHelperText>

frontend/src/components/EditableTextDescriptionListGroup.tsx

@@ -9,7 +9,7 @@ type EditableTextDescriptionListGroupProps = Pick<
   'title' | 'contentWhenEmpty'
 > & {
   value: string;
-  saveEditedValue: (value: string) => Promise<void>;
+  saveEditedValue: (value: string) => Promise<unknown>;
 };
 
 const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGroupProps> = ({
@@ -37,6 +37,8 @@ const EditableTextDescriptionListGroup: React.FC<EditableTextDescriptionListGrou
           value={unsavedValue}
           onChange={(_event, v) => setUnsavedValue(v)}
           isDisabled={isSavingEdits}
+          rows={24}
+          resizeOrientation="vertical"
         />
       }
       onEditClick={() => {

frontend/src/concepts/modelRegistry/types.ts

@@ -35,14 +35,65 @@ export enum ModelArtifactState {
   REFERENCE = 'REFERENCE',
 }
 
+export enum ModelRegistryMetadataType {
+  INT = 'MetadataIntValue',
+  DOUBLE = 'MetadataDoubleValue',
+  STRING = 'MetadataStringValue',
+  STRUCT = 'MetadataStructValue',
+  PROTO = 'MetadataProtoValue',
+  BOOL = 'MetadataBoolValue',
+}
+
+export type ModelRegistryCustomPropertyInt = {
+  metadataType: ModelRegistryMetadataType.INT;
+  int_value: string; // int64-formatted string
+};
+
+export type ModelRegistryCustomPropertyDouble = {
+  metadataType: ModelRegistryMetadataType.DOUBLE;
+  double_value: number;
+};
+
+export type ModelRegistryCustomPropertyString = {
+  metadataType: ModelRegistryMetadataType.STRING;
+  string_value: string;
+};
+
+export type ModelRegistryCustomPropertyStruct = {
+  metadataType: ModelRegistryMetadataType.STRUCT;
+  struct_value: string; // Base64 encoded bytes for struct value
+};
+
+export type ModelRegistryCustomPropertyProto = {
+  metadataType: ModelRegistryMetadataType.PROTO;
+  type: string; // url describing proto value
+  proto_value: string; // Base64 encoded bytes for proto value
+};
+
+export type ModelRegistryCustomPropertyBool = {
+  metadataType: ModelRegistryMetadataType.BOOL;
+  bool_value: boolean;
+};
+
+export type ModelRegistryCustomProperty =
+  | ModelRegistryCustomPropertyInt
+  | ModelRegistryCustomPropertyDouble
+  | ModelRegistryCustomPropertyString
+  | ModelRegistryCustomPropertyStruct
+  | ModelRegistryCustomPropertyProto
+  | ModelRegistryCustomPropertyBool;
+
+export type ModelRegistryCustomProperties = Record<string, ModelRegistryCustomProperty>;
+export type ModelRegistryStringCustomProperties = Record<string, ModelRegistryCustomPropertyString>;
+
 export type ModelRegistryBase = {
   id: string;
   name: string;
   externalID?: string;
   description?: string;
   createTimeSinceEpoch?: string;
   lastUpdateTimeSinceEpoch: string;
-  customProperties: Record<string, Record<string, string>>;
+  customProperties: ModelRegistryCustomProperties;
 };
 
 export type ModelArtifact = ModelRegistryBase & {

frontend/src/pages/modelRegistry/screens/GlobalModelVersionsTabs.tsx

@@ -11,12 +11,14 @@ type GlobalModelVersionsTabProps = {
   tab: ModelVersionsTabs;
   registeredModel: RegisteredModel;
   modelVersions: ModelVersion[];
+  refresh: () => void;
 };
 
 const GlobalModelVersionsTabs: React.FC<GlobalModelVersionsTabProps> = ({
   tab,
   registeredModel: rm,
   modelVersions,
+  refresh,
 }) => {
   const navigate = useNavigate();
   return (
@@ -44,7 +46,7 @@ const GlobalModelVersionsTabs: React.FC<GlobalModelVersionsTabProps> = ({
         data-testid="model-details-tab"
       >
         <PageSection isFilled variant="light" data-testid="model-details-tab-content">
-          <ModelDetailsView registeredModel={rm} />
+          <ModelDetailsView registeredModel={rm} refresh={refresh} />
         </PageSection>
       </Tab>
     </Tabs>