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

Use kserve without serverless #3694

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

emilys314
Copy link
Contributor

@emilys314 emilys314 commented Jan 28, 2025

Closes https://issues.redhat.com/browse/RHOAIENG-16490

Depends on opendatahub-io/opendatahub-operator#1565

Description

This extends the defaultDeploymentMode by checking if serverless is available on the cluster. If it's not, we can default kserve to raw.

  • If raw is disabled, it should default to serverless still (even if both end up disabled somehow)
  • raw is used if flag is enabled but serverless is set as the default
  • The auth warning for serverless is moved to below the deployment mode selector if it's visible

When serverless is removed but raw is enabled:
image

If autherino is not installed and serverless is selected:
image

If autherino is not installed and raw selected:
image

How Has This Been Tested?

Overriding the dsc status in chrome networking to add the following:
components.kserve.serverlessMode to "serverlessMode": "Removed"

Test Impact

Added a cypress test

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.

@yih-wang

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 Jan 28, 2025
Copy link
Contributor

openshift-ci bot commented Jan 28, 2025

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

Copy link
Contributor

openshift-ci bot commented Jan 28, 2025

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please ask for approval from emilys314. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found 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

Copy link

codecov bot commented Jan 28, 2025

Codecov Report

Attention: Patch coverage is 91.11111% with 4 lines in your changes missing coverage. Please review.

Project coverage is 84.14%. Comparing base (3167270) to head (374062e).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
...g/screens/projects/NIMServiceModal/NoAuthAlert.tsx 33.33% 2 Missing ⚠️
...projects/NIMServiceModal/ManageNIMServingModal.tsx 88.88% 1 Missing ⚠️
...rving/screens/projects/kServeModal/NoAuthAlert.tsx 66.66% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3694      +/-   ##
==========================================
- Coverage   84.16%   84.14%   -0.03%     
==========================================
  Files        1453     1455       +2     
  Lines       33865    33881      +16     
  Branches     9384     9395      +11     
==========================================
+ Hits        28504    28510       +6     
- Misses       5361     5371      +10     
Files with missing lines Coverage Δ
frontend/src/k8sTypes.ts 100.00% <ø> (ø)
...tend/src/pages/clusterSettings/ClusterSettings.tsx 93.33% <100.00%> (ø)
...s/clusterSettings/ModelServingPlatformSettings.tsx 86.00% <100.00%> (+0.28%) ⬆️
...jects/kServeModal/KServeDeploymentModeDropdown.tsx 100.00% <100.00%> (ø)
...screens/projects/kServeModal/ManageKServeModal.tsx 94.26% <100.00%> (+0.07%) ⬆️
...d/src/pages/modelServing/screens/projects/utils.ts 93.33% <100.00%> (ø)
.../src/pages/modelServing/useKServeDeploymentMode.ts 100.00% <100.00%> (ø)
...projects/NIMServiceModal/ManageNIMServingModal.tsx 87.59% <88.88%> (+0.18%) ⬆️
...rving/screens/projects/kServeModal/NoAuthAlert.tsx 66.66% <66.66%> (ø)
...g/screens/projects/NIMServiceModal/NoAuthAlert.tsx 33.33% <33.33%> (ø)

... and 3 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 3167270...374062e. Read the comment docs.

@emilys314 emilys314 marked this pull request as ready for review January 29, 2025 14:15
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jan 29, 2025
@openshift-ci openshift-ci bot requested review from manaswinidas and pnaik1 January 29, 2025 14:15
import React from 'react';
import { Alert, AlertActionCloseButton, StackItem } from '@patternfly/react-core';

export const NoAuthAlert: React.FC<{ setAlertVisible: (isVisible: boolean) => void }> = ({
Copy link
Contributor

Choose a reason for hiding this comment

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

Having an onClose callback makes more sense because the boolean param is never set to anything other than false.

Suggested change
export const NoAuthAlert: React.FC<{ setAlertVisible: (isVisible: boolean) => void }> = ({
export const NoAuthAlert: React.FC<{ onClose: () => void }> = ({

@christianvogt
Copy link
Contributor

@emilys314 do you need to wait on the dependent operator PR to merge first? If so, put this PR on hold until then.

@emilys314
Copy link
Contributor Author

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold This PR is hold for some reason label Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold This PR is hold for some reason
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants