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

Remove explicit support for OCI model serving compatibility #3459

Merged

Conversation

emilys314
Copy link
Contributor

@emilys314 emilys314 commented Nov 11, 2024

towards [RHOAIENG-14748] enable connection types feature

Description

After review and discussion from Jeff DeMoss, we decided to remove the OCI ootb connection type, because it's not fully supported yet. Many OCI model registries may require authentication, and requires additional kserve support before being usable (https://kserve.github.io/website/latest/modelserving/storage/oci/#enabling-modelcars)

image
image

How Has This Been Tested?

  • Make sure there are no references to "OCI" in the connection types admin page
  • The ootb OCI connection type will also no longer exist (but you'd have to do this manually on your cluster if you already have it)
    • And fyi it will still show up in model serving as a "URI" compatible type

Test Impact

  • tests updated for removing OCI

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 requested review from dpanshug and lucferbux November 11, 2024 16:34
@emilys314 emilys314 force-pushed the remove-oci-connection-type branch from 44a7e9b to bcae032 Compare November 11, 2024 18:17
Copy link

codecov bot commented Nov 11, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.66%. Comparing base (e85d7a1) to head (bcae032).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #3459   +/-   ##
=======================================
  Coverage   85.66%   85.66%           
=======================================
  Files        1347     1347           
  Lines       30670    30669    -1     
  Branches     8550     8550           
=======================================
  Hits        26272    26272           
+ Misses       4398     4397    -1     
Files with missing lines Coverage Δ
frontend/src/concepts/connectionTypes/utils.ts 90.95% <ø> (-0.05%) ⬇️

... and 2 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 e85d7a1...bcae032. Read the comment docs.

Copy link
Contributor

@jenny-s51 jenny-s51 left a comment

Choose a reason for hiding this comment

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

Updates LGTM @emilys314 ! Deleted the oci-compliant-registry-v1 configMap from the shared cluster as discussed since it was showing up in the connection types list by default. No longer showing in the connection types list as expected.

"Model serving compatible type" dropdown looks good too and no longer references OCI registry.

Copy link
Contributor

@mturley mturley left a comment

Choose a reason for hiding this comment

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

LGTM. Tested locally against the ods-qe-psi-08 cluster. I temporarily scaled down the operator in order to delete the oci-compliant-registry-v1 configmap and verified that it is not an option in model serving or the connection types page.

Copy link
Contributor

openshift-ci bot commented Nov 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jenny-s51, mturley

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 fd2a63c into opendatahub-io:main Nov 11, 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.

3 participants