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

Home Page MR card in the "Manage models" section #3537

Merged

Conversation

YuliaKrimerman
Copy link
Contributor

@YuliaKrimerman YuliaKrimerman commented Dec 3, 2024

RHOAIENG-16021

Description

In the home page under "Work with AI/ML models"(based on comments below) ( previously "Train, serve, monitor, and manage AI/ML models") "Manage Models"( Previously Deploy and monitor models) will be displayed based on the 4 cases below:
Case 1 : customer has model registry and model serving both enabled - display all 3 cards .
Case 2 : customer only has model registry enabled - display only Model registry card.
Case 3 : customer only has model serving enabled - display Model servers and Deploying models cards only.
Case 4 : Customer not enabled both model registry or model serving - don't display Manage Models all together.

NOTE: The icons in the screenshot are not up to date, and don't reflect the "Work with AI/ML models" name change as well

How Has This Been Tested?

The best way to tests is to add ?devFeatureFlags and disable/enable model registry/model serving in according to the 4 Cases above and verify that we display the correct cards as shown in the screenshots below
Screenshot 2024-12-03 at 12 07 02 PM
Screenshot 2024-12-02 at 11 27 10 AM
Screenshot 2024-12-02 at 11 27 28 AM
Screenshot 2024-12-02 at 11 27 41 AM

Test Impact

Fixed test to match the new scenario

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 Dec 3, 2024
Copy link

codecov bot commented Dec 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.16%. Comparing base (70e0d4e) to head (94d0d5a).
Report is 19 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3537      +/-   ##
==========================================
- Coverage   85.56%   85.16%   -0.40%     
==========================================
  Files        1372     1380       +8     
  Lines       31211    31541     +330     
  Branches     8741     8818      +77     
==========================================
+ Hits        26705    26862     +157     
- Misses       4506     4679     +173     
Files with missing lines Coverage Δ
...src/pages/home/aiFlows/DeployAndMonitorGallery.tsx 100.00% <100.00%> (ø)
frontend/src/pages/home/aiFlows/useAIFlows.tsx 96.29% <100.00%> (+0.14%) ⬆️

... and 51 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70e0d4e...94d0d5a. Read the comment docs.

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.

LGTM aside from rebasing to fix conflicts with #3533 and the PF6 changes

@YuliaKrimerman YuliaKrimerman changed the title WIP Home Page MR card in the "Manage models" section Home Page MR card in the "Manage models" section Dec 10, 2024
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Dec 10, 2024
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.

Sorry @YuliaKrimerman , I think I missed this one detail (see inline comment) when I reviewed before your rebase.

Also, looking back at the mockups, there's an additional change we should add here - the "Train, serve, monitor, and manage AI/ML models" text that is rendered in useAIFlows.tsx should now be conditional and become "Train and manage AI/ML models" if both model serving and model registry are disabled. @yih-wang is that a correct interpretation of how that text should behave?

<Content component="small">
Deploy models to test them and integrate them into applications. Deploying a model makes
it accessible via an API, enabling you to return predictions based on data inputs.
infoItems.push(
Copy link
Contributor

@mturley mturley Dec 10, 2024

Choose a reason for hiding this comment

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

This "deploying models" info item should appear if model serving is enabled even if modelmesh is not enabled. (i.e. if both modelmesh and kserve are enabled or if only kserve is enabled). That used to be true simply because this component was only rendered if model serving was enabled and this was outside the if (modelMeshEnabled) that you moved it into.

I think you need to add const kServeEnabled = servingPlatformStatuses.kServe.enabled; above next to line 13 where modelMeshEnabled is defined, and then this "deploying models" info item should be pushed if (modelMeshEnabled || kServeEnabled).

@yih-wang
Copy link

Also, looking back at the mockups, there's an additional change we should add here - the "Train, serve, monitor, and manage AI/ML models" text that is rendered in useAIFlows.tsx should now be conditional and become "Train and manage AI/ML models" if both model serving and model registry are disabled.

@mturley yes that's what I meant in the mockup... But looking at it again, I realize it's not accurate to call the section "Train and manage AI/ML models" when both model serving and model registry are disabled, because users cannot manage models without these two sections...

@kaedward can you help take a look at this microcopy? In the last case 4 shown here where the whole Manage models section is hidden, what should the title say? We have 'Train, serve, monitor, and manage AI/ML models' for the common case but sounds like the last three words are all related to the 'manage model' section. Should we just say 'Train AI/ML models' when the Manage models section is not there? Sounds too pale for me...

@kaedward
Copy link

@kaedward can you help take a look at this microcopy? In the last case 4 shown here where the whole Manage models section is hidden, what should the title say? We have 'Train, serve, monitor, and manage AI/ML models' for the common case but sounds like the last three words are all related to the 'manage model' section. Should we just say 'Train AI/ML models' when the Manage models section is not there? Sounds too pale for me...

@yih-wang, does "Work with AI/ML models" sound better? However, if all they can do is train them, I don't think it's a bad thing to just say "Train AI/ML models". We're just being honest ;-)

@YuliaKrimerman
Copy link
Contributor Author

@yih-wang @kaedward Also what about the case when reg is enabled but serv is not we'd still have the word "serve" there?

@kaedward
Copy link

@yih-wang @YuliaKrimerman I honestly wonder if just using a more generic title, like "Work with AI/ML models" would be more efficient, instead of having a different version of the title for each combo of permissions.

@YuliaKrimerman
Copy link
Contributor Author

@mturley Addressed the comments, ready for re-review

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.

LGTM! Thanks @YuliaKrimerman

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

openshift-ci bot commented Dec 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

@mturley mturley dismissed manaswinidas’s stale review December 12, 2024 20:45

Comments were addressed

@YuliaKrimerman
Copy link
Contributor Author

/retest

@openshift-merge-bot openshift-merge-bot bot merged commit f155ba2 into opendatahub-io:main Dec 12, 2024
4 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.

5 participants