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

Add deployment modal for model registry #3074

Merged

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Aug 7, 2024

JIRA: RHOAIENG-8396

Description

When you deploy a model based on a model registry, the following modal will show:

Screenshot 2024-08-07 at 1 27 46 PM

If anything wrong with the project, the corresponding error message will pop up:

Screenshot 2024-08-07 at 1 28 32 PM

Otherwise, the current KServe modal or InferenceService modal will pop up depending on the project's serving platform. The name will be pre-filled as {modelName} - {versionName}:

Screenshot 2024-08-07 at 1 29 29 PM

There is a helper text if the model registry version contains a prompt model format:

Screenshot 2024-08-07 at 1 32 08 PM

The data connections part will be decided by the model artifact URI.
If there are more than 1 matches, we don't pre-select the data connection:

Screenshot 2024-08-07 at 1 32 32 PM

If there is only one match, we pre-select the recommended data connection:

Screenshot 2024-08-07 at 1 34 19 PM

If there is no match, we pre-select the new data connection field:

Screenshot 2024-08-07 at 1 35 14 PM

The path will always be pre-filled based on the URI parsing result:

Screenshot 2024-08-07 at 1 36 58 PM

How Has This Been Tested?

  1. Create project A, B and C
  2. For project A, make it an unsupported serving platform (or don't select a serving platform if both platforms are supported)
  3. Go to Model registry page, choose one model registry and click on Deploy on the version action
  4. Choose project A, check the error message
  5. For project B and C, make the serving platform selected (or make your cluster only support one platform)
  6. Choose project B, check if the name field is auto-filled
  7. Check the data connection field is pre-selected as New data connection and path field is pre-filled
  8. Create one matching secret in project B and 2 matching secrets in project C ("matching" means the secret has the same bucket, endpoint and region as the model registry version artifact URI you are going to deploy with)
  9. Choose project B again, check the data connection field is pre-selected as Existing data connection and the only matching data connection is labeled and pre-selected
  10. Choose project C, check the data connection field is pre-selected as Existing data connection and both data connections in the dropdown menu are labeled as recommended, but neither is pre-selected

Test Impact

Add a unit test to test the util function that shows the error message when choosing the project.

Request review criteria:

Self checklist (all need to be checked):

  • The developer has manually tested the changes and verified that the changes work
  • Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
  • 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 requested review from dpanshug and mturley August 7, 2024 17:45
@yih-wang
Copy link

yih-wang commented Aug 8, 2024

Thanks for the work @DaoDaoNoCode! Just a few comments -

  1. Can you stretch the modal width in the first image to the same width as the other modals?
  2. Regarding the pre-filled model name, I can't see what happens if a version has been deployed multiple times. The ideal behavior is if the name has already been used within the selected project, add a postfix such as ‘-2’, ‘-3’ to differentiate it.
  3. Regarding the helper text to the prompt model format - the helper text will only show up after a model server is selected, and the model framework dropdown is available.
  4. For data connection, when there is no match and the new data connection is selected by default - a warning is missing, check details in the mockup

@DaoDaoNoCode
Copy link
Member Author

@yih-wang Thanks for the review! I will update it!

Copy link
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.

This is looking really good! I have one actual change request (the comment in useLabeledDataConnections) and then just some questions and suggestions.

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Aug 8, 2024
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Aug 8, 2024
@DaoDaoNoCode
Copy link
Member Author

  1. Can you stretch the modal width in the first image to the same width as the other modals?
  2. Regarding the pre-filled model name, I can't see what happens if a version has been deployed multiple times. The ideal behavior is if the name has already been used within the selected project, add a postfix such as ‘-2’, ‘-3’ to differentiate it.
  3. Regarding the helper text to the prompt model format - the helper text will only show up after a model server is selected, and the model framework dropdown is available.
  4. For data connection, when there is no match and the new data connection is selected by default - a warning is missing, check details in the mockup

Based on the comments above, I updated the following:

  1. For your first comment, I updated the size of the initial modal
Screenshot 2024-08-08 at 7 48 23 PM
  1. For the second comment, I found it's a little bit hard from the technical side to implement what you requested. We will need to fetch all the models in that project and it's hard to decide the postfix number (e.g. There are model-1, model-3 and model-4, what should the auto-generated number be? model-2 or model-5?). I prefer we keep it as it is now, and if the name is duplicated, the server will always prompt an error when the user submits the model. (cc @mturley Might need your input here)
  2. For the third comment, I updated the helper text to make it only show when there are items in the dropdown menu.

No items in the menu:

Screenshot 2024-08-08 at 7 56 50 PM

Items in the menu:

Screenshot 2024-08-08 at 7 57 01 PM
  1. For the fourth comment, I updated the alert:
Screenshot 2024-08-08 at 7 58 13 PM

@yih-wang Let me know what you think, thanks!

@yih-wang
Copy link

yih-wang commented Aug 9, 2024

@DaoDaoNoCode All the changes look good. Thanks for the rapid work!
Regarding the auto-generated deployment name, my primary concern is to avoid duplicate names and prevent user frustration from seeing duplicated names error continuously. If the original proposal doesn't work, we can implement a workaround by adding a timestamp as postfix to each name. So we'll have a format like [modelname-versionname-yyyymmddhhmmss], which should effectively eliminate duplicates as well.

@DaoDaoNoCode
Copy link
Member Author

Thanks @yih-wang I made the requested change.

Screenshot 2024-08-09 at 11 26 52 AM

@yih-wang
Copy link

LGTM, thanks @DaoDaoNoCode !

Copy link
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.

Pulled and tested on the modelregistry-ui cluster - everything looks and works great here!! Thanks so much for the hard work @DaoDaoNoCode !

@openshift-ci openshift-ci bot added the lgtm label Aug 12, 2024
Copy link
Contributor

openshift-ci bot commented Aug 12, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mturley

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 dc622db into opendatahub-io:main Aug 12, 2024
6 checks passed
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