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 mode select for kserve raw #3574

Conversation

emilys314
Copy link
Contributor

@emilys314 emilys314 commented Dec 12, 2024

Closes https://issues.redhat.com/browse/RHOAIENG-16487
Figma https://www.figma.com/design/hKimcyeu6pgAoqKH1XrqmY/Support-KServe-RawDeployment-mode?node-id=231-2365&node-type=canvas&t=KepUcCxzaanRkMU8-0

Description

First part of adding kserve raw model deployments. This adds the deployment mode dropdown to single serving (kserve) modals. What is done is basically adding ...DeploymentMode: RawDeployment and the corresponding labels for auth / routing. The backend model controller takes care of the rest.

  • Advanced -> Serverless / old behaviour
  • Standard -> KServe raw / new

Changing the deployment mode on an existing deployment is not supported so it will be disabled on edit. However the assembleInferenceService() does properly replace the labels and annotations.

The deployment mode dropdown will also show up in NIM

image

How Has This Been Tested?

  1. Have a cluster with the latest backend controllers (I have DSC config dev flags or my cluster I can share)
   kserve:
      # defaultDeploymentMode: RawDeployment
      devFlags:
        manifests:
          - contextDir: config
            sourcePath: overlays/odh
            uri: 'https://github.com/VedantMahabaleshwarkar/kserve/tarball/devflags'
          - contextDir: config
            sourcePath: ''
            uri: 'https://github.com/VedantMahabaleshwarkar/odh-model-controller/tarball/devflags'
      managementState: Managed
      serving:
        ingressGateway:
          certificate:
            type: OpenshiftDefaultIngress
        managementState: Managed
        name: knative-serving
  1. add ?devFeatureFlags to the URL and make sure the kserve raw is set to false
  2. Go to a single model serving project, deploy a new or edit an existing model
  3. The default selection should be advanced, but change it to standard (raw)
  4. Everything should work normally. In the console you can verify the inferenceService file, and check under the serverless tab on the sidebar it doesn't exist. Try inferencing the endpoint

(Public endpoint without auth doesn't work)

Test Impact

cypress and jest tests added

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 12, 2024
Copy link
Contributor

openshift-ci bot commented Dec 12, 2024

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

@emilys314 emilys314 force-pushed the kserve-raw-deployment-mode-select branch from 9fe89e9 to 9526799 Compare December 13, 2024 16:24
Copy link

codecov bot commented Dec 13, 2024

Codecov Report

Attention: Patch coverage is 91.04478% with 6 lines in your changes missing coverage. Please review.

Project coverage is 85.17%. Comparing base (1896f34) to head (d8e208c).
Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
frontend/src/api/k8s/inferenceServices.ts 91.89% 3 Missing ⚠️
...d/src/pages/modelServing/screens/projects/utils.ts 83.33% 2 Missing ⚠️
...projects/NIMServiceModal/ManageNIMServingModal.tsx 75.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #3574      +/-   ##
==========================================
- Coverage   85.19%   85.17%   -0.02%     
==========================================
  Files        1382     1383       +1     
  Lines       31572    31624      +52     
  Branches     8824     8839      +15     
==========================================
+ Hits        26897    26937      +40     
- Misses       4675     4687      +12     
Files with missing lines Coverage Δ
frontend/src/k8sTypes.ts 100.00% <ø> (ø)
...jects/kServeModal/KServeDeploymentModeDropdown.tsx 100.00% <100.00%> (ø)
...screens/projects/kServeModal/ManageKServeModal.tsx 94.26% <100.00%> (+0.18%) ⬆️
frontend/src/pages/modelServing/screens/types.ts 100.00% <ø> (ø)
...projects/NIMServiceModal/ManageNIMServingModal.tsx 87.50% <75.00%> (-0.38%) ⬇️
...d/src/pages/modelServing/screens/projects/utils.ts 93.07% <83.33%> (-0.58%) ⬇️
frontend/src/api/k8s/inferenceServices.ts 94.82% <91.89%> (-1.84%) ⬇️

... and 5 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 1896f34...d8e208c. Read the comment docs.

@emilys314 emilys314 marked this pull request as ready for review December 13, 2024 18:01
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Dec 13, 2024
@yih-wang
Copy link

Hey @emilys314, is the (default) dynamically assigned to the option which is set as the default mode in the cluster settings?

@emilys314
Copy link
Contributor Author

Hey @emilys314, is the (default) dynamically assigned to the option which is set as the default mode in the cluster settings?

In this PR, no it's not dynamic, but a future PR will make it dynamic. Unless we want to remove it completely

@emilys314 emilys314 force-pushed the kserve-raw-deployment-mode-select branch from 9526799 to d8e208c Compare December 17, 2024 14:48
@yih-wang
Copy link

yih-wang commented Dec 18, 2024

In this PR, no it's not dynamic, but a future PR will make it dynamic. Unless we want to remove it completely

I see. I didn’t include this default flag in my original mockup, but I think it could be a good idea to have it. We might just need to clarify what ‘default’ means in this context to ensure users understand. That said, I don’t think this is a blocker for the PR. I’ll think through this further and let you know if I decide to suggest any changes.

Copy link
Contributor

openshift-ci bot commented Dec 18, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: jeff-phillips-18

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 167525b into opendatahub-io:main Dec 18, 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