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.

Sign up for GitHub

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

Jump to bottom

Add model serving platform settings #1982

Merged
merged 1 commit into from
Oct 25, 2023
Merged

Add model serving platform settings #1982

merged 1 commit into from
Oct 25, 2023

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Oct 18, 2023 •
edited
Loading

Closes #1913

Description

Add a section in the cluster settings to control the feature flags disableKServe and disableModelMesh in the dashboard config. When both checkboxes are unchecked, show a warning alert. When the multi-model option was initially selected but unchecked, show an info alert.

Screenshot 2023-10-18 at 5 29 25 PM Screenshot 2023-10-18 at 5 29 42 PM Screenshot 2023-10-19 at 11 15 36 AM

How Has This Been Tested?

  1. Go to the cluster settings page
  2. Try to play around with the serving platform section, change the settings, and submit
  3. Go to check the dashboard config YAML on the cluster to see if the feature flags are updated
  4. Try to uncheck both checkboxes to see if the alert is shown
  5. Try to check both options and save the changes
  6. Try to uncheck the multi-model option and see if the info alert is shown

Test Impact

Add some integration tests on the cluster settings page to see if the alert is properly prompted and if the disablement/enablement state of the save button is performing correctly.

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 tests & storybook 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 (find relevant UX in the SMEs section).

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

@DaoDaoNoCode
Copy link
Member Author

@vconzola Can you please check the screenshots in the description, thanks!

dpanshug
dpanshug previously requested changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@dpanshug dpanshug left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The info alert when MM is not selected is not added. https://www.sketch.com/s/45df001b-2441-4124-a436-5ae1abb409c9/a/Eej0Yvg

Update: It's already mentioned in PR description, my bad.

@dpanshug dpanshug dismissed their stale review October 19, 2023 09:46

Added by mistake

@lucferbux lucferbux linked an issue Oct 19, 2023 that may be closed by this pull request
[Story]: Add admin section to control model serving platforms #1913
Closed
@lucferbux
Copy link
Contributor

Note: The design in https://www.sketch.com/s/45df001b-2441-4124-a436-5ae1abb409c9/a/Eej0Yvg is not implemented yet, we need to figure out the logic.

Why can't we implement this @DaoDaoNoCode this is just basically a warning from when you uncheck an option that is already selected, just to inform the user what's the flow there

@lucferbux
Copy link
Contributor

Btw, @vconzola considering we can disable both models, we'll need another screen for the project section since none of the options are available

lucferbux
lucferbux requested changes Oct 19, 2023
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

Code looks great, check the comment i left, I don't see why you cannot add the other logic

backend/src/routes/api/cluster-settings/clusterSettingsUtils.ts Show resolved Hide resolved
frontend/src/pages/clusterSettings/ModelServingPlatformSettings.tsx Outdated Show resolved Hide resolved
lucferbux
lucferbux reviewed Oct 20, 2023
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

Ok, functionality wise it's wroking as expected @christianvogt can you take a closer look at the code, if everything is fine should be ready to go and we can approve it

@lucferbux
Copy link
Contributor

@DaoDaoNoCode if you want we can do a pair review this evening and get this asap

@DaoDaoNoCode
Copy link
Member Author

@lucferbux Sure, I was busy with the other ticket yesterday and got no time to look at this PR, I will definitely finish this today.

@lucferbux
Copy link
Contributor

@lucferbux Sure, I was busy with the other ticket yesterday and got no time to look at this PR, I will definitely finish this today.

Yeah, don't worry, just pinging you in case it wasn't in your radar, you are doing an amazing work so far!

lucferbux
lucferbux approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

@DaoDaoNoCode
Copy link
Member Author

/hold
Fixing testing

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Oct 25, 2023
@openshift-ci openshift-ci bot removed the lgtm label Oct 25, 2023
@DaoDaoNoCode
Copy link
Member Author

/unhold
Fixed now

@openshift-ci openshift-ci bot removed the do-not-merge/hold This PR is hold for some reason label Oct 25, 2023
@DaoDaoNoCode
Copy link
Member Author

/test

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

@DaoDaoNoCode: No presubmit jobs available for opendatahub-io/odh-dashboard@f/model-serving

In response to this:

/test

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lucferbux
lucferbux approved these changes Oct 25, 2023
View reviewed changes
Copy link
Contributor

@lucferbux lucferbux left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 25, 2023
@lucferbux
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 25, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lucferbux

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-ci openshift-ci bot merged commit c5c3596 into opendatahub-io:f/model-serving Oct 25, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucferbux lucferbux lucferbux approved these changes

@alexcreasy alexcreasy Awaiting requested review from alexcreasy

@ppadti ppadti Awaiting requested review from ppadti

@christianvogt christianvogt Awaiting requested review from christianvogt

@dpanshug dpanshug Awaiting requested review from dpanshug

Assignees

@lucferbux lucferbux

@dpanshug dpanshug

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Story]: Add admin section to control model serving platforms
4 participants
@DaoDaoNoCode @lucferbux @christianvogt @dpanshug