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

[RHOAIENG-6573] Model Detail - Registered Deployments Section #3092

Conversation

jpuzz0
Copy link
Contributor

@jpuzz0 jpuzz0 commented Aug 14, 2024

https://issues.redhat.com/browse/RHOAIENG-6573

Description

  • Added labels to creation of model deployments
  • Added registered deployments to model version details and archived model version details.
image

(cc @yih-wang)

How to test

Navigate to the version details of a given model version (archived or otherwise). Click on the registered deployments tab, verify the contents match the design in terms of the alert, table columns, and table data. If no table data exists, that means the version has no registered deployments, so you can create one using the toolbar action "Deploy".

Test Impact

Added unit tests and a cypress test verifying data can render in the registered deployments table.

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 14, 2024
@openshift-ci openshift-ci bot requested review from manaswinidas and ppadti August 14, 2024 22:25
@jpuzz0 jpuzz0 force-pushed the RHOAIENG-6573-reg-deploy-section branch 5 times, most recently from 0556274 to 1a15306 Compare August 15, 2024 15:27
@jpuzz0 jpuzz0 changed the title WIP [RHOAIENG-6573] Model Detail - Registered Deployments Section [RHOAIENG-6573] Model Detail - Registered Deployments Section 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
@jpuzz0 jpuzz0 force-pushed the RHOAIENG-6573-reg-deploy-section branch from 1a15306 to f7f8d3c Compare August 15, 2024 17:55
@mturley
Copy link
Contributor

mturley commented Aug 15, 2024

LGTM at a quick glance - will do a more complete review tomorrow morning. Thanks for the work @jpuzz0 .

@yih-wang
Copy link

Thanks for the efforts @jpuzz0! One comment and two questions -

  • The microcopy of the tab name and the alert should be aligned with what is provided in the handoff doc, reference this row
  • I see a column for API protocol in the table on the live Model Serving page, but it doesn't show up in the screenshots, has that column been removed in the latest RHOAI version?
  • I'm a little confused when I see the case that deployments are showing up for an archived version. Because the user is not allowed to archive the version/model which includes active deployments, so for an archived version, this tab should always be empty.

@jpuzz0
Copy link
Contributor Author

jpuzz0 commented Aug 16, 2024

  • this row

@yih-wang

  1. I've updated the microcopy

  2. Your mockups don't have that API protocol column, so therefore I removed it for this instance of the table:

image
  1. We currently don't have that restriction setup, and that isn't a part of this task's scope, so we can make a separate JIRA task/issue for that.

@jpuzz0 jpuzz0 force-pushed the RHOAIENG-6573-reg-deploy-section branch 4 times, most recently from 30c187e to 63d8f6a Compare August 16, 2024 16:49
@jpuzz0 jpuzz0 force-pushed the RHOAIENG-6573-reg-deploy-section branch from 63d8f6a to 4a90d81 Compare August 16, 2024 18:49
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.

download

(tested for RBAC issues with an image on modelregistry-ui cluster after fetches were implemented, tested new changes on localhost)

Copy link
Contributor

openshift-ci bot commented Aug 16, 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 1391418 into opendatahub-io:main Aug 16, 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.

3 participants