-
Notifications
You must be signed in to change notification settings - Fork 179
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 3 commits
a0d6444
d707a9a
9dfa838
15e0f3f
a13e1ce
ad82af1
34a4d62
de30ec4
9f42369
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 |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ import * as React from 'react'; | |
| import { | ||
| DescriptionList, | ||
| Divider, | ||
| Content, | ||
| ContentVariants, | ||
| Title, | ||
| Bullseye, | ||
|
|
@@ -33,6 +34,8 @@ import ModelDetailsCard from '~/app/pages/modelRegistry/screens/ModelVersions/Mo | |
| import ModelVersionRegisteredFromLink from '~/app/pages/modelRegistry/screens/components/ModelVersionRegisteredFromLink'; | ||
| import useModelTransferJobForArtifact from '~/app/hooks/useModelTransferJobForArtifact'; | ||
| import { modelSourcePropertiesToTransferJobParams } from '~/concepts/modelRegistry/utils'; | ||
| import { formatModelTypeDisplay } from '~/app/pages/modelCatalog/utils/modelCatalogUtils'; | ||
| import { getModelTypeRawStringFromCustomProperties } from '~/app/pages/modelRegistry/screens/RegisterModel/registerModelTypeUtils'; | ||
| import StorageLocationSection from './StorageLocationSection'; | ||
|
|
||
| type ModelVersionDetailsViewProps = { | ||
|
|
@@ -64,6 +67,10 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({ | |
| const [transferJob, transferJobLoaded, transferJobError, refreshTransferJob] = | ||
| useModelTransferJobForArtifact(transferJobParams); | ||
|
|
||
| const versionPageModelTypeRaw = | ||
| getModelTypeRawStringFromCustomProperties(registeredModel?.customProperties) ?? | ||
| getModelTypeRawStringFromCustomProperties(mv.customProperties); | ||
|
|
||
| if (!modelArtifactsLoaded) { | ||
| return ( | ||
| <Bullseye> | ||
|
|
@@ -105,6 +112,7 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({ | |
| refresh={refresh} | ||
| isArchiveModel={isArchiveVersion} | ||
| isExpandable | ||
| modelTypeFallbackCustomProperties={mv.customProperties} | ||
| /> | ||
| </StackItem> | ||
| )} | ||
|
|
@@ -117,6 +125,11 @@ const ModelVersionDetailsView: React.FC<ModelVersionDetailsViewProps> = ({ | |
| <Sidebar hasBorder hasGutter isPanelRight> | ||
| <SidebarContent> | ||
| <DescriptionList> | ||
| <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. I don't think we need to show this model type in version details card - so I think we can remove this
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. Removed it! |
||
| <Content component="p" data-testid="model-version-model-type"> | ||
| {formatModelTypeDisplay(versionPageModelTypeRaw)} | ||
| </Content> | ||
| </DashboardDescriptionListGroup> | ||
| <EditableLabelsDescriptionListGroup | ||
| labels={getLabels(mv.customProperties)} | ||
| isArchive={isArchiveVersion} | ||
|
|
||
| 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,6 +117,10 @@ const ModelDetailsCard: React.FC<ModelDetailsCardProps> = ({ | |
| /> | ||
| ); | ||
|
|
||
| const modelTypeRaw = | ||
| getModelTypeRawStringFromCustomProperties(rm.customProperties) ?? | ||
| getModelTypeRawStringFromCustomProperties(modelTypeFallbackCustomProperties); | ||
|
|
||
| const infoSection = ( | ||
| <> | ||
| <DashboardDescriptionListGroup | ||
|
|
@@ -146,6 +155,11 @@ const ModelDetailsCard: React.FC<ModelDetailsCardProps> = ({ | |
| > | ||
| <ModelTimestamp timeSinceEpoch={rm.createTimeSinceEpoch} /> | ||
| </DashboardDescriptionListGroup> | ||
| <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.
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. |
||
| <Content component="p" data-testid="registered-model-model-type"> | ||
| {formatModelTypeDisplay(modelTypeRaw)} | ||
| </Content> | ||
|
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.
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! updated the |
||
| </DashboardDescriptionListGroup> | ||
| </> | ||
| ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| 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_PLACEHOLDER_KEY = '__model-type-unset__'; | ||
|
|
||
| const MODEL_TYPE_SELECT_OPTIONS: SimpleSelectOption[] = [ | ||
| { | ||
|
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.
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. |
||
| key: MODEL_TYPE_PLACEHOLDER_KEY, | ||
| label: 'Select model type', | ||
| isPlaceholder: true, | ||
| }, | ||
| { | ||
| 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; | ||
| }; | ||
|
|
||
| const RegisterModelTypeField: React.FC<RegisterModelTypeFieldProps> = ({ | ||
| modelCustomProperties, | ||
| onModelCustomPropertiesChange, | ||
| isRequired, | ||
| }) => { | ||
| const stored = getModelTypeStoredValueFromCustomProperties(modelCustomProperties); | ||
|
|
||
| const handleChange = (key: string, isPlaceholder: boolean) => { | ||
|
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. The handleChange callback checks both isPlaceholder and key === MODEL_TYPE_PLACEHOLDER_KEY. The isPlaceholder check alone should be sufficient — checking the key as well is redundant but harmless. |
||
| if (isPlaceholder || key === MODEL_TYPE_PLACEHOLDER_KEY) { | ||
| onModelCustomPropertiesChange( | ||
| buildCustomPropertiesWithModelType(modelCustomProperties, undefined), | ||
| ); | ||
| return; | ||
| } | ||
| 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 | ||
| 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