-
Notifications
You must be signed in to change notification settings - Fork 178
feat(ui): register and display model_type from catalog and model registry #2545
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
a0d6444
d707a9a
9dfa838
15e0f3f
a13e1ce
ad82af1
34a4d62
de30ec4
9f42369
c608fc6
563b8cf
ae3a7d0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,25 +20,30 @@ import { | |
| DashboardDescriptionListGroup, | ||
| EditableLabelsDescriptionListGroup, | ||
| } from 'mod-arch-shared'; | ||
| import { RegisteredModel } from '~/app/types'; | ||
| import { ModelRegistryCustomProperties, RegisteredModel } from '~/app/types'; | ||
| import ModelTimestamp from '~/app/pages/modelRegistry/screens/components/ModelTimestamp'; | ||
| import ModelPropertiesExpandableSection from '~/app/pages/modelRegistry/screens/components/ModelPropertiesExpandableSection'; | ||
| import { ModelRegistryContext } from '~/app/context/ModelRegistryContext'; | ||
| import { getLabels, mergeUpdatedLabels } from '~/app/pages/modelRegistry/screens/utils'; | ||
| import { EMPTY_CUSTOM_PROPERTY_VALUE } from '~/concepts/modelCatalog/const'; | ||
| import { formatModelTypeDisplay } from '~/app/pages/modelCatalog/utils/modelCatalogUtils'; | ||
| import { getModelTypeRawStringFromCustomProperties } from '~/app/pages/modelRegistry/screens/RegisterModel/registerModelTypeUtils'; | ||
|
|
||
| type ModelDetailsCardProps = { | ||
| registeredModel: RegisteredModel; | ||
| refresh: () => void; | ||
| isArchiveModel?: boolean; | ||
| isExpandable?: boolean; | ||
| /** If `model_type` is absent on the registered model, read from these (e.g. version custom properties). */ | ||
| modelTypeFallbackCustomProperties?: ModelRegistryCustomProperties; | ||
| }; | ||
|
|
||
| const ModelDetailsCard: React.FC<ModelDetailsCardProps> = ({ | ||
| registeredModel: rm, | ||
| refresh, | ||
| isArchiveModel, | ||
| isExpandable, | ||
| modelTypeFallbackCustomProperties, | ||
| }) => { | ||
| const { apiState } = React.useContext(ModelRegistryContext); | ||
| const [isExpanded, setIsExpanded] = React.useState(false); | ||
|
|
@@ -112,8 +117,17 @@ const ModelDetailsCard: React.FC<ModelDetailsCardProps> = ({ | |
| /> | ||
| ); | ||
|
|
||
| const modelTypeRaw = | ||
| getModelTypeRawStringFromCustomProperties(rm.customProperties) ?? | ||
| getModelTypeRawStringFromCustomProperties(modelTypeFallbackCustomProperties); | ||
|
|
||
| const infoSection = ( | ||
| <> | ||
| <DashboardDescriptionListGroup title="Model type"> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @Taj010 one minor changes I noticed in the mockups is that we need move model ID field to the bottom of this listGroup.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! |
||
| <Content component="p" data-testid="registered-model-model-type"> | ||
| {formatModelTypeDisplay(modelTypeRaw)} | ||
| </Content> | ||
| </DashboardDescriptionListGroup> | ||
| <DashboardDescriptionListGroup | ||
| title="Owner" | ||
| popover="The owner is the user who registered the model." | ||
|
|
@@ -122,16 +136,6 @@ const ModelDetailsCard: React.FC<ModelDetailsCardProps> = ({ | |
| {rm.owner || EMPTY_CUSTOM_PROPERTY_VALUE} | ||
| </Content> | ||
| </DashboardDescriptionListGroup> | ||
| <DashboardDescriptionListGroup title="Model ID"> | ||
| <ClipboardCopy | ||
| hoverTip="Copy" | ||
| clickTip="Copied" | ||
| variant="inline-compact" | ||
| data-testid="registered-model-id-clipboard-copy" | ||
| > | ||
| {rm.id} | ||
| </ClipboardCopy> | ||
| </DashboardDescriptionListGroup> | ||
| <DashboardDescriptionListGroup | ||
| isEmpty={!rm.lastUpdateTimeSinceEpoch} | ||
| contentWhenEmpty="Unknown" | ||
|
|
@@ -146,6 +150,16 @@ const ModelDetailsCard: React.FC<ModelDetailsCardProps> = ({ | |
| > | ||
| <ModelTimestamp timeSinceEpoch={rm.createTimeSinceEpoch} /> | ||
| </DashboardDescriptionListGroup> | ||
| <DashboardDescriptionListGroup title="Model ID"> | ||
| <ClipboardCopy | ||
| hoverTip="Copy" | ||
| clickTip="Copied" | ||
| variant="inline-compact" | ||
| data-testid="registered-model-id-clipboard-copy" | ||
| > | ||
| {rm.id} | ||
| </ClipboardCopy> | ||
| </DashboardDescriptionListGroup> | ||
| </> | ||
| ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -11,19 +11,26 @@ import { UpdateObjectAtPropAndValue } from 'mod-arch-shared'; | |
| import FormFieldset from '~/app/pages/modelRegistry/screens/components/FormFieldset'; | ||
| import FormSection from '~/app/pages/modelRegistry/components/pf-overrides/FormSection'; | ||
| import { MR_CHARACTER_LIMIT } from './const'; | ||
| import RegisterModelTypeField from './RegisterModelTypeField'; | ||
| import { RegisterModelFormData } from './useRegisterModelData'; | ||
|
|
||
| type RegisterModelDetailsFormSectionProp<D extends RegisterModelFormData> = { | ||
| formData: D; | ||
| setData: UpdateObjectAtPropAndValue<D>; | ||
| hasModelNameError: boolean; | ||
| isModelNameDuplicate?: boolean; | ||
| /** When true (MR register-from-registry), model type is required; submit stays disabled until set. */ | ||
| isModelTypeRequired?: boolean; | ||
| /** When true (catalog → MR), model type is prefilled from catalog metadata and is not editable. */ | ||
| isModelTypeReadOnly?: boolean; | ||
| }; | ||
| const RegisterModelDetailsFormSection = <D extends RegisterModelFormData>({ | ||
| formData, | ||
| setData, | ||
| hasModelNameError, | ||
| isModelNameDuplicate, | ||
| isModelTypeRequired = false, | ||
| isModelTypeReadOnly = false, | ||
| }: RegisterModelDetailsFormSectionProp<D>): React.ReactNode => { | ||
| const modelNameInput = ( | ||
| <TextInput | ||
|
|
@@ -75,6 +82,12 @@ const RegisterModelDetailsFormSection = <D extends RegisterModelFormData>({ | |
| </HelperText> | ||
| </FormHelperText> | ||
| </FormGroup> | ||
| <RegisterModelTypeField | ||
| modelCustomProperties={formData.modelCustomProperties} | ||
| onModelCustomPropertiesChange={(next) => setData('modelCustomProperties', next)} | ||
| isRequired={isModelTypeRequired} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looking at the mock ups both flow from model-registry and catalog - this model-type field is required. So just curios why we need this extra
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| isReadOnly={isModelTypeReadOnly} | ||
| /> | ||
| </FormSection> | ||
| ); | ||
| }; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| import { FormGroup } from '@patternfly/react-core'; | ||
| import React from 'react'; | ||
| import { SimpleSelect } from 'mod-arch-shared'; | ||
| import { SimpleSelectOption } from 'mod-arch-shared/dist/components/SimpleSelect'; | ||
| import { ModelRegistryCustomProperties } from '~/app/types'; | ||
| import { ModelType } from '~/concepts/modelCatalog/const'; | ||
| import { formatModelTypeDisplay } from '~/app/pages/modelCatalog/utils/modelCatalogUtils'; | ||
| import FormFieldset from '~/app/pages/modelRegistry/screens/components/FormFieldset'; | ||
| import { | ||
| buildCustomPropertiesWithModelType, | ||
| getModelTypeStoredValueFromCustomProperties, | ||
| } from './registerModelTypeUtils'; | ||
|
|
||
| const MODEL_TYPE_SELECT_OPTIONS: SimpleSelectOption[] = [ | ||
| { | ||
| key: ModelType.GENERATIVE, | ||
| label: formatModelTypeDisplay(ModelType.GENERATIVE), | ||
| }, | ||
| { | ||
| key: ModelType.PREDICTIVE, | ||
| label: formatModelTypeDisplay(ModelType.PREDICTIVE), | ||
| }, | ||
| ]; | ||
|
|
||
| type RegisterModelTypeFieldProps = { | ||
| modelCustomProperties: ModelRegistryCustomProperties | undefined; | ||
| onModelCustomPropertiesChange: (next: ModelRegistryCustomProperties) => void; | ||
| isRequired?: boolean; | ||
| /** Catalog registration: type is catalog metadata (like URI), not user-editable. */ | ||
| isReadOnly?: boolean; | ||
| }; | ||
|
|
||
| const RegisterModelTypeField: React.FC<RegisterModelTypeFieldProps> = ({ | ||
| modelCustomProperties, | ||
| onModelCustomPropertiesChange, | ||
| isRequired, | ||
| isReadOnly, | ||
| }) => { | ||
| const stored = getModelTypeStoredValueFromCustomProperties(modelCustomProperties); | ||
|
|
||
| const handleChange = (key: string) => { | ||
| if (key === ModelType.GENERATIVE || key === ModelType.PREDICTIVE) { | ||
| onModelCustomPropertiesChange(buildCustomPropertiesWithModelType(modelCustomProperties, key)); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <FormGroup label="Model type" isRequired={isRequired} fieldId="register-model-type"> | ||
| <FormFieldset | ||
| field="Model type" | ||
| component={ | ||
| <SimpleSelect | ||
| options={MODEL_TYPE_SELECT_OPTIONS} | ||
| value={stored ?? undefined} | ||
| onChange={handleChange} | ||
| placeholder="Select model type" | ||
| isFullWidth | ||
| isDisabled={isReadOnly} | ||
| dataTestId="register-model-type-select" | ||
| previewDescription={false} | ||
| popperProps={{ direction: 'down' }} | ||
| toggleProps={{ id: 'register-model-type-toggle' }} | ||
| /> | ||
| } | ||
| /> | ||
| </FormGroup> | ||
| ); | ||
| }; | ||
|
|
||
| export default RegisterModelTypeField; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In here we can reuse reuse getModelTypeStoredValueFromCustomProperties from registerModelTypeUtils.ts , or even better - buildCustomPropertiesWithModelType
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reimplemented via
getModelTypeStoredValueFromCustomPropertiesandbuildCustomPropertiesWithModelType