Skip to content
This repository has been archived by the owner on Jan 31, 2024. It is now read-only.

Conversation

israel-hdez
Copy link
Contributor

Description

  • References to Alibi and ART explainers are removed from the inferenceservice-config ConfigMap.
  • User cluster roles are moved into an overlay in opendatahub-io/kserve: Add ODH overlay kserve#35
  • Namespace resource is excluded

Fixes #884
Fixes #885
Related to #887

How Has This Been Tested?

  • Run kustomize build ./kserve/base > before.yaml over the current master branch.
  • Run kustomize build ./kserve/base > after.yaml over this PR.
  • Running diff -u before.yaml after.yaml generates the following diff:
--- before.yaml 2023-07-21 13:13:48.120286546 -0600
+++ after.yaml  2023-07-21 13:23:33.518921123 -0600
@@ -1,13 +1,3 @@
-apiVersion: v1
-kind: Namespace
-metadata:
-  labels:
-    app.kubernetes.io/part-of: kserve
-    control-plane: kserve-controller-manager
-    controller-tools.k8s.io: "1.0"
-    istio-injection: disabled
-  name: opendatahub
----
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
@@ -18372,17 +18362,7 @@
     {
       "defaultDeploymentMode": "Serverless"
     }
-  explainers: |-
-    {
-        "alibi": {
-            "image" : "quay.io/opendatahub/kserve-alibi-explainer",
-            "defaultImageVersion": "v0.10.2"
-        },
-        "art": {
-            "image" : "quay.io/opendatahub/kserve-art-explainer",
-            "defaultImageVersion": "latest"
-        }
-    }
+  explainers: '{}'
   ingress: |-
     {
         "ingressGateway" : "knative-serving/knative-ingress-gateway",
@@ -18437,10 +18417,6 @@
 apiVersion: v1
 data:
   kserve-agent: quay.io/opendatahub/kserve-agent:v0.10.2
-  kserve-alibi-explainer: quay.io/opendatahub/kserve-alibi-explainer
-  kserve-alibi-explainer-version: v0.10.2
-  kserve-art-explainer: quay.io/opendatahub/kserve-art-explainer
-  kserve-art-explainer-version: latest
   kserve-controller: quay.io/opendatahub/kserve-controller:v0.10.2
   kserve-router: quay.io/opendatahub/kserve-router:v0.10.2
   kserve-storage-initializer: quay.io/opendatahub/kserve-storage-initializer:v0.10.2
  • Visually inspect that the differential indicates that the Namespace resource is removed.
  • Visually inspect that the differential suggests that the inferenceservice-config has empty explainers configuration.
  • Visually inspect that the differential suggests that the kserve-parameters ConfigMap no longer includes explainer parameters.
  • Extract the inferenceservice-config ConfigMap from after.yaml, apply it to an existent KServe installation, and restart and confirm that the KServe controller runs normally.

Merge criteria:

  • The commits are squashed in a cohesive manner and have meaningful messages.
  • Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
  • The developer has manually tested the changes and verified that the changes work

* References to Alibi and ART explainers are removed from the
`inferenceservice-config` ConfigMap.
* User cluster roles are moved into an overlay in opendatahub-io/kserve

Signed-off-by: Edgar Hernández <23639005+israel-hdez@users.noreply.github.com>
"defaultImageVersion": "$(kserve-art-explainer-version)"
}
}
explainers: "{}"
Copy link
Contributor

Choose a reason for hiding this comment

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

kustomize build shows

 deploy: |-
    {       
      "defaultDeploymentMode": "Serverless"
    }       
  explainers: '{}'
  ingress: |-   
    {             
        "ingress.....

I didn't test it but I am not sure if {} is ok to deploy kserve and a model? If it works, I am ok with it for now before we integrate with TrustyAI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be safe if we always omit using explainers.

As soon as we try to use some explainer, we may have issues. But we are not supporting them at the moment.

@VaishnaviHire
Copy link
Member

/lgtm

Copy link
Contributor

@Jooho Jooho left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci
Copy link

openshift-ci bot commented Jul 26, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Jooho

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

@israel-hdez
Copy link
Contributor Author

/retest

3 similar comments
@israel-hdez
Copy link
Contributor Author

/retest

@israel-hdez
Copy link
Contributor Author

/retest

@israel-hdez
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit 28bbf3b into opendatahub-io:master Aug 2, 2023
@israel-hdez israel-hdez deleted the 884-885-remove-explainers-and-namespace branch August 2, 2023 16:45
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Status: Done
4 participants