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

Cleanup of caikit OOTB #2147

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Conversation

lucferbux
Copy link
Contributor

Description

Closes #2143

How Has This Been Tested?

  1. Deploy the new template with oc apply -k manifests/modelserving
  2. Create a new Caikit deployment with kserve
  3. Check that you can infer the results

Test Impact

No testing involved

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 requested review from andrewballantyne and Xaenalt and removed request for christianvogt and pnaik1 November 15, 2023 12:40
@lucferbux
Copy link
Contributor Author

@andrewballantyne @Xaenalt This is the cleanup PR for caikit. The renaming of the file is not needed, since it's an internal file and there's no implication whatsoever to the final product. I can do it if you want but I don't see the need.

@bdattoma
Copy link

bdattoma commented Nov 15, 2023

maybe we could add the version metadata as well, otherwise user will see this in the UI:
image

@Xaenalt does the runtime have a specific version?

I would say we could review the need of (name - version) in the UI since the metadata is not mandatory.

@lucferbux
Copy link
Contributor Author

/lgtm

Copy link
Contributor

openshift-ci bot commented Nov 16, 2023

@lucferbux: you cannot LGTM your own PR.

In response to this:

/lgtm

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
Copy link
Contributor Author

/approve

Copy link
Contributor

openshift-ci bot commented Nov 16, 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

@lucferbux
Copy link
Contributor Author

maybe we could add the version metadata as well, otherwise user will see this in the UI: image

@Xaenalt does the runtime have a specific version?

I would say we could review the need of (name - version) in the UI since the metadata is not mandatory.

We can add that as a follow up!

@lucferbux lucferbux added the lgtm label Nov 16, 2023
@openshift-merge-bot openshift-merge-bot bot merged commit 3b03421 into opendatahub-io:main Nov 16, 2023
6 checks passed
@bdattoma
Copy link

@lucferbux follow up issue: #2189

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.

[Bug]: Cleanup caikit ootb runtime
4 participants