Skip to content
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

Model Registry: Register New Version form #3078

Merged

Conversation

mturley
Copy link
Contributor

@mturley mturley commented Aug 8, 2024

Closes RHOAIENG-7571

Description

Implements the "Register New Version" button in these places:

  • RegisteredModelsTableToolbar (no specific registered model)
  • ModelVersionListView empty state (under a specific registered model)
  • ModelVersionListView toolbar (under a specific registered model)

Depending on where the action was selected, the user will be redirected to one of these routes which both render the RegisterVersion page:

  • /modelRegistry/{mrName}/registerVersion - Presents a typeahead select for choosing a model
  • /modelRegistry/{mrName}/registeredModels/{registeredModelId}/registerVersion - Pre-selects the model and disables the model field

The RegisterVersion form is mostly a subset of the RegisterModel form - Version Details and Model Location fields in both are factored out into a new component RegistrationCommonFormSections. The only difference is that instead of entering a model name and description we let the user select a model (or preselect it for them), and then we show some help text and prefill some fields based on the selected model.

When a model is selected, the hook usePrefillRegisterVersionFields re-renders with a new registeredModel value which causes it to fetch the versions of that model, then fetch the artifacts for that version, finally deriving a latestVersion and latestArtifact. The useEffect in that hook is then triggered to update form values based on latestArtifact (source model format/version and model location endpoint/bucket/region), and latestVersion is returned and passed into RegistrationCommonFormSections where it is used to show the "Current version is ..." help text.

If the model selection changes and the selected model has no version/artifact, nothing will be prefilled. If it has a version and artifact but the URI on that artifact is malformed (cannot be parsed into endpoint/bucket/region), only the source model format/version is prefilled. Any fields that can't be prefilled are reset to empty, so switching between models should not leave behind values prefilled from another model.

Registering a new version from the registered models table (no preselected model):

Screenshot 2024-08-15 at 11 25 31 AM

Screenshot 2024-08-15 at 11 25 50 AM

Selecting a model prefills fields:

Screenshot 2024-08-15 at 11 26 07 AM

Registering a new version from a registered model's page (when there are no versions):

Screenshot 2024-08-15 at 11 27 47 AM

Registering a new version from a registered model's page (when there are already versions):

Screenshot 2024-08-15 at 11 26 51 AM

Model is preselected and fields are already filled:

Screenshot 2024-08-15 at 11 27 10 AM

cc @yih-wang

How Has This Been Tested?

Tested on modelregistry-ui cluster. Tried registering versions by reaching the form from all 3 buttons, covered all cases described above re: prefilling.

Test Impact

Unit tests added for new getLastCreatedItem util, and tests moved for utils that moved (filterArchiveVersions, filterLiveVersions).

Cypress tests added to cover all cases of prefilling/clearing behavior, form validation, and creating expected resources on submit in each case.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

  • Included any necessary screenshots or gifs if it was a UI change.
  • Included tags to the UX team if it was a UI/UX change.

After the PR is posted & before it merges:

  • The developer has tested their solution on a cluster by using the image produced by the PR to main

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Aug 8, 2024
@openshift-ci openshift-ci bot requested review from DaoDaoNoCode and pnaik1 August 8, 2024 19:20
@mturley mturley changed the title [WIP] Register Version form [WIP] Model Registry: Register Version form Aug 8, 2024
@mturley mturley changed the title [WIP] Model Registry: Register Version form [WIP] Model Registry: Register New Version form Aug 8, 2024
Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

When I'm trying to Register new version via dropdown menu in the Registered models table.. after selecting model name.. the model location fields are pre-filled but in case of Register new version button in the Versions table ... although the model name is prefilled, the model location fields aren't pre-filled, is this expected?
Screenshot 2024-08-12 at 6 14 29 PM

@mturley
Copy link
Contributor Author

mturley commented Aug 13, 2024

nope, that is not expected... thanks, good catch.

@mturley mturley force-pushed the RHOAIENG-7571-register-version branch 2 times, most recently from 53bfe29 to 401731f Compare August 14, 2024 20:55
@mturley mturley changed the title [WIP] Model Registry: Register New Version form Model Registry: Register New Version form Aug 15, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Aug 15, 2024
@mturley mturley force-pushed the RHOAIENG-7571-register-version branch from bb50322 to 1328d73 Compare August 15, 2024 15:36
@yih-wang
Copy link

yih-wang commented Aug 16, 2024

Thanks for all the work @mturley! The only comment I have is regarding the visual style of the read-only fields (model registry and model name). I know you are following the mockup to use the read-only status for a text field. But recently I just noticed the other forms are using plain text to show the read-only values. See the image below. I think we should follow that consistency. I'm fine to either address it in this issue or leave it to post-mvp. Sorry for the duplicated work!
image

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Aug 16, 2024
Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

The registration and auto-fill work fine now. But after a successful model version registration the redirect URL should be the Model version table(according to mocks). This was there earlier AFAIR. It's going to the version Details page for me. Let me know if I'm not referring to the latest mocks.
Screenshot 2024-08-16 at 6 17 39 PM

@mturley mturley force-pushed the RHOAIENG-7571-register-version branch from 1328d73 to 7586ac5 Compare August 16, 2024 14:48
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Aug 16, 2024
@mturley
Copy link
Contributor Author

mturley commented Aug 16, 2024

@manaswinidas good catch, I've rebased and fixed that. I had to squash the commits to avoid a bunch of pain in rebasing on top of #3084, sorry if that makes it harder to re-review -- mostly I moved that PR's changes to RegisterModel into RegistrationCommonFormSections.

@yih-wang no worries! yeah, do you mind including that somewhere we're tracking post-MVP refactors? I'm not sure the best place for it for now - we need to meet and follow up to make sure there are ENG issues opened for all those under https://issues.redhat.com/browse/RHOAIENG-10405 at some point soon.

@mturley mturley requested a review from manaswinidas August 16, 2024 14:55
@mturley mturley force-pushed the RHOAIENG-7571-register-version branch from 0c229ae to 55d7029 Compare August 16, 2024 14:58
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley force-pushed the RHOAIENG-7571-register-version branch from 55d7029 to cc023cd Compare August 16, 2024 15:07
Signed-off-by: Mike Turley <[email protected]>
@mturley mturley force-pushed the RHOAIENG-7571-register-version branch from cc023cd to 2dcc616 Compare August 16, 2024 15:21
},
)[0];

export const filterArchiveVersions = (modelVersions: ModelVersion[]): ModelVersion[] =>
Copy link
Contributor

Choose a reason for hiding this comment

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

could combine these to 2 by passing ModelState or even to 1 if you didn't mind doing models: ModelVersion[] | RegisteredModel[], doesn't matter though

Copy link
Contributor

Choose a reason for hiding this comment

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

i see these were just moved from another place anyway 😆

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 😆 that's fair.. probably better to factor them out using generics or use ModelRegistryBase, but that's tech debt for another time I think (or fine as-is if we never get to it)

Copy link
Contributor

openshift-ci bot commented Aug 16, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: gitdallas

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

The pull request process is described here

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

@openshift-merge-bot openshift-merge-bot bot merged commit c69451a into opendatahub-io:main Aug 16, 2024
6 checks passed
@mturley mturley deleted the RHOAIENG-7571-register-version branch August 16, 2024 17:10
@yih-wang
Copy link

@yih-wang no worries! yeah, do you mind including that somewhere we're tracking post-MVP refactors? I'm not sure the best place for it for now - we need to meet and follow up to make sure there are ENG issues opened for all those under https://issues.redhat.com/browse/RHOAIENG-10405 at some point soon.

Add a ux issue RHOAIUX-317 to track the following work

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.

5 participants