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

handle kserve in global model serving page #2001

Conversation

christianvogt
Copy link
Contributor

@christianvogt christianvogt commented Oct 22, 2023

Addresses most of #1948 but not everything since the edit modal for kserve isn't yet available. For now the edit action for a kserve inference service is disabled.
Also does not expose GRPC url in table (out of scope).
Does not address deletion of ServingRuntime as this will be addressed by ownerReferences when we create the kserve ServingRuntime and InferenceService - related to #1998

Description

Updates the model serving table to display column for Serving runtime and removes the Model server column.
Changed title of the global model serving page to Deployed models

image

fyi @vconzola

How Has This Been Tested?

  • Create a model server: ServingRuntime
  • Deploy a model: InferenceService
  • Edit the InferenceService and change annotation serving.kserve.io/deploymentMode to a value other than ModelMesh
  • Go to the Model Serving page using the left navigation
  • Observe the changes to the table
  • Click the kebab menu for your InferenceService and observe that Edit is disabled
  • Use the Delete action
  • Observe the InferenceService is deleted (Note that until the kserve creation flows are complete, the ServingRuntime will not be deleted as we will rely on ownerReferences)

Test Impact

Added some unit tests.

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

@lucferbux lucferbux linked an issue Oct 25, 2023 that may be closed by this pull request
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, so far the changes look good, we need to review the logic to delete the ServingRuntime as we'll need to delete other resources too and there's a new version of the mockups, if you can take a look would be awesome!

inferenceService.metadata.namespace,
),
servingRuntime
? deleteServingRuntime(
Copy link
Contributor

Choose a reason for hiding this comment

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

Be aware that there are several resources to delete with the serving runtime too, such as the rolebinding, secrets and more, I think we could cover this in #1998 but this logic should be similar to https://github.com/opendatahub-io/odh-dashboard/blob/main/frontend/src/pages/modelServing/screens/projects/DeleteServingRuntimeModal.tsx#L50

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The complex deletion of the ServingRuntime should be handled in #1998.

That being said, I am deleting two related resources here and I wonder if we should rely on creating an ownerReference between InferenceService and ServingRuntime for kserve. If so then I don't need to delete both resources here.

@openshift-merge-robot openshift-merge-robot added the needs-rebase PR needs to be rebased label Oct 25, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase PR needs to be rebased label Oct 25, 2023
@christianvogt
Copy link
Contributor Author

@lucferbux I update the PR to include the latest UX change for the project label.

I've also removed the deletion change I initially made in lieu of #1998. As per our slack conversation, we will utilize ownerReferences to perform the clean up of related resources.

<>
{project ? (
<>
{getProjectDisplayName(project)}{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

We could give a little bit of padding, but that can be a follow up enhancement and we can discuss it later with UX

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

I've left a little nit that we can change later on, but we can unblock this

@openshift-ci openshift-ci bot added the lgtm label Oct 25, 2023
@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 7ec208c into opendatahub-io:f/model-serving Oct 25, 2023
3 checks passed
@vconzola
Copy link

LGTM

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.

[Story]: Adapt Global View Table to Kserve
4 participants