Skip to content

feat(ui): register and display model_type from catalog and model registry#2545

Open
Taj010 wants to merge 12 commits intokubeflow:mainfrom
Taj010:register-model-type
Open

feat(ui): register and display model_type from catalog and model registry#2545
Taj010 wants to merge 12 commits intokubeflow:mainfrom
Taj010:register-model-type

Conversation

@Taj010
Copy link
Copy Markdown
Contributor

@Taj010 Taj010 commented Apr 3, 2026

Description

This PR adds a shared Model type field (Generative / Predictive) on register-from-catalog and register-from-registry flows.

  • Catalog registration prefills the field from catalog metadata when it is generative or predictive.
  • Register-from-registry requires a selection before submit.

Model overview and version details was also adjusted to show model type property.

image image image

How Has This Been Tested?

Manually Testing

  • Open register-from-catalog with a catalog model confirm dropdown prefills
  • Open MR Register model, confirm submit stays disabled until model type is chosen
  • Open model and version detail pages and confirm Model type displays as expected.

Automated Testing

  • Added tests to modelCatalogUtils.spec.tsand utils.spec.ts file and adjusted tests in registerAndStoreFields.cy.ts

Merge criteria:

  • All the commits have been signed-off (To pass the DCO check)
  • The commits have meaningful messages
  • Automated tests are provided as part of the PR for major new functionalities; testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work.
  • Code changes follow the kubeflow contribution guidelines.
  • For first time contributors: Please reach out to the Reviewers to ensure all tests are being run, ensuring the label ok-to-test has been added to the PR.

If you have UI changes

  • The developer has added tests or explained why testing cannot be added.
  • Included any necessary screenshots or gifs if it was a UI change.
  • Verify that UI/UX changes conform the UX guidelines for Kubeflow.

Taj010 added 2 commits April 3, 2026 14:39
Signed-off-by: Taj010 <rsheen003@gmail.com>
…o it

Signed-off-by: Taj010 <rsheen003@gmail.com>
@google-oss-prow google-oss-prow Bot requested review from chambridge and fege April 3, 2026 19:12
@github-actions github-actions Bot added Area/UI and removed size/L labels Apr 3, 2026
Signed-off-by: Taj010 <rsheen003@gmail.com>
}) => {
const stored = getModelTypeStoredValueFromCustomProperties(modelCustomProperties);

const handleChange = (key: string, isPlaceholder: boolean) => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

string_value: next,
};
}
return result;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add unit tests for this utility.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added registerModelTypeUtils.spec.ts

isSubmitDisabledForCommonFields(formData, namespaceHasAccess, isNamespaceAccessLoading) ||
!isNameValid(formData.modelName) ||
isModelNameExisting(formData.modelName, registeredModels);
options?: { requireModelType?: boolean },
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We've changed the default behavior here which is fine, the only thing I would maybe comment here that if in the future a caller will forget to opt-out it might be surprising . Alternatively we can default to false with explicit opt-in( just safer) not a must, you're choice

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted to have requireModelType now defaults to false and adjusted the utils.spec.ts too

* Returns model registry customProperties entries to prefill `model_type` when registering
* from the catalog. Only generative/predictive are copied; unknown or missing values yield {}.
*/
export const getCatalogModelTypePropertyForRegistration = (
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reimplemented via getModelTypeStoredValueFromCustomProperties and buildCustomPropertiesWithModelType

Signed-off-by: Taj010 <rsheen003@gmail.com>
@google-oss-prow google-oss-prow Bot added size/XL and removed size/L labels Apr 9, 2026
Signed-off-by: Taj010 <rsheen003@gmail.com>
Copy link
Copy Markdown
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! One thing to check: model_type is now displayed in a dedicated "Model type" field on the detail pages, but the getProperties() function in screens/utils.ts doesn't exclude model_type from the editable properties table. This means it will show up twice — once as the new read-only display field, and again as an editable row in the properties section. Should it be filtered out of the properties table (similar to how _lastModified and _registeredFrom are excluded)?

Also I actually think we may want a tech debt ticket to clean those two up, at least _registeredFrom isn't used anymore now that we have modelSource* properties, but that's out of scope.

Comment on lines +155 to +161
>
<ModelTimestamp timeSinceEpoch={rm.createTimeSinceEpoch} />
</DashboardDescriptionListGroup>
<DashboardDescriptionListGroup title="Model type">
<Content component="p" data-testid="registered-model-model-type">
{formatModelTypeDisplay(modelTypeRaw)}
</Content>
Copy link
Copy Markdown
Contributor

@mturley mturley Apr 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

model_type will also appear in the editable properties table rendered by ModelPropertiesExpandableSection below, since getProperties() in screens/utils.ts doesn't filter it out. This means the user will see it twice — here as read-only, and again as an editable property row. Consider adding model_type to the exclusion list in getProperties().

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! updated the getProperties() in utils.ts to exclude model_type and also ajsued the utils.spc.ts test file accordingly

Signed-off-by: Taj010 <rsheen003@gmail.com>
Copy link
Copy Markdown
Contributor

@ppadti ppadti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taj010 @mturley - On the Register new version form, are we not adding this field there? In this form either we will have the model name selected and disabled or drop down.

<Sidebar hasBorder hasGutter isPanelRight>
<SidebarContent>
<DescriptionList>
<DashboardDescriptionListGroup title="Model type">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed it!

>
<ModelTimestamp timeSinceEpoch={rm.createTimeSinceEpoch} />
</DashboardDescriptionListGroup>
<DashboardDescriptionListGroup title="Model type">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see this model type at the bottom - but design has it at the top, no?

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! Moved model type to the top
image

const MODEL_TYPE_PLACEHOLDER_KEY = '__model-type-unset__';

const MODEL_TYPE_SELECT_OPTIONS: SimpleSelectOption[] = [
{
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see Select model type as one of the option - But I think we don't need that as an option

Image

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! Adjusted it - removed the select model type option now
image

Signed-off-by: Taj010 <rsheen003@gmail.com>
@ppadti
Copy link
Copy Markdown
Contributor

ppadti commented Apr 17, 2026

Thanks @Taj010 for addressing all the review comments.
circling back to this in case it got missed -
@mturley we might have add this model type field to the Register a new version form as well right?

@mturley
Copy link
Copy Markdown
Contributor

mturley commented Apr 17, 2026

@ppadti I think since the model type goes into custom properties of the model and not the version, we can't include it in that form. Makes sense to me that all versions of a model are the same type

@mturley
Copy link
Copy Markdown
Contributor

mturley commented Apr 17, 2026

(sorry, on mobile, I'll read all the context in a bit)

Signed-off-by: Arsheen Taj Syed  <87820563+Taj010@users.noreply.github.com>
@google-oss-prow
Copy link
Copy Markdown
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from mturley. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Signed-off-by: Taj010 <rsheen003@gmail.com>

const infoSection = (
<>
<DashboardDescriptionListGroup title="Model type">
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

Signed-off-by: Taj010 <rsheen003@gmail.com>
@mturley
Copy link
Copy Markdown
Contributor

mturley commented Apr 29, 2026

/lgtm
I'll let others approve who have been more directly involved in reviewing here.

@google-oss-prow google-oss-prow Bot added the lgtm label Apr 29, 2026
@ppadti
Copy link
Copy Markdown
Contributor

ppadti commented Apr 30, 2026

Apologies for another question here @mturley
when we register from catalog - currently we are pre-filling the model-type from model custom properties, and letting the user to switch it to different option. Is that intended behavior?
Screenshot 2026-04-30 at 3 57 09 PM

<RegisterModelTypeField
modelCustomProperties={formData.modelCustomProperties}
onModelCustomPropertiesChange={(next) => setData('modelCustomProperties', next)}
isRequired={isModelTypeRequired}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The 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 isRequired prop?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

RegisterModelDetailsFormSection is shared: MR register passes isModelTypeRequired so the type field shows as required and matches isRegisterModelSubmitDisabled(..., { requireModelType: true }). Catalog register uses the same block but omits the prop and keeps requireModelType: false in isRegisterCatalogModelSubmitDisabled, so we only flag required in the MR path for now. If we want the catalog to match the mocks, we’d turn on the prop there and flip that submit check to true as well.

// _lastModified is a property that is required to update the timestamp on the backend and we have a workaround for it. It should be resolved by
// backend team
if (key === '_lastModified' || /^_registeredFrom/.test(key)) {
if (key === '_lastModified' || key === 'model_type' || /^_registeredFrom/.test(key)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we use the const CatalogModelCustomPropertyKey.MODEL_TYPE here instead of model_type?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! updated to use CatalogModelCustomPropertyKey.MODEL_TYPE

@google-oss-prow google-oss-prow Bot removed the lgtm label Apr 30, 2026
Signed-off-by: Taj010 <rsheen003@gmail.com>
@mturley
Copy link
Copy Markdown
Contributor

mturley commented Apr 30, 2026

@ppadti re: your earlier comment - when registering from catalog, you're right that we probably shouldn't let the user change it. we don't let them change the uri after all, this is more knowledge from the catalog that we're just passing through. cc @Taj010

Signed-off-by: Taj010 <rsheen003@gmail.com>
@Taj010
Copy link
Copy Markdown
Contributor Author

Taj010 commented Apr 30, 2026

Now from the model catalog flow - the model type is in a read-only state:

image

@Taj010 Taj010 requested review from mturley and ppadti May 1, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants