user defined model name overwrite issue fix (kubeflow#2342)

* user defined envs overwrite issue fix Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com> * Refactor code to GetModelName util func Signed-off-by: Dan Sun <dsun20@bloomberg.net> * Fix custom transformer model name Signed-off-by: Dan Sun <dsun20@bloomberg.net> * Fix IsMMSPredictor and add tests Signed-off-by: Dan Sun <dsun20@bloomberg.net> Signed-off-by: Suresh Nakkeran <suresh.n@ideas2it.com> Signed-off-by: Dan Sun <dsun20@bloomberg.net> Co-authored-by: Dan Sun <dsun20@bloomberg.net>
magdalenakuhn17 · Nov 29, 2022 · 48427d6 · 48427d6
1 parent 6ad9b45
commit 48427d6
Show file tree

Hide file tree

Showing 13 changed files with 282 additions and 136 deletions.
diff --git a/docs/samples/v1beta1/custom/simple.yaml b/docs/samples/v1beta1/custom/simple.yaml
@@ -7,6 +7,7 @@ spec:
     minReplicas: 1
     containers:
     - image: codait/max-object-detector
+      name: kserve-container
       ports:
         - containerPort: 5000
           protocol: TCP
diff --git a/docs/samples/v1beta1/transformer/torchserve_image_transformer/transformer.yaml b/docs/samples/v1beta1/transformer/torchserve_image_transformer/transformer.yaml
@@ -6,7 +6,7 @@ spec:
   transformer:
     containers:
     - image: kserve/torchserve-image-transformer:latest
-      name: kfserving-container
+      name: kserve-container
       env:
         - name: STORAGE_URI
           value: gs://kfserving-examples/models/torchserve/image_classifier/v1

diff --git a/pkg/apis/serving/v1beta1/inference_service_defaults.go b/pkg/apis/serving/v1beta1/inference_service_defaults.go
@@ -22,6 +22,7 @@ import (
 	"strconv"
 
 	"github.com/kserve/kserve/pkg/constants"
+	"github.com/kserve/kserve/pkg/utils"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/resource"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -257,14 +258,14 @@ func (isvc *InferenceService) SetMlServerDefaults() {
 	}
 	// set environment variables based on storage uri
 	if isvc.Spec.Predictor.Model.StorageURI == nil && isvc.Spec.Predictor.Model.Storage == nil {
-		isvc.Spec.Predictor.Model.Env = append(isvc.Spec.Predictor.Model.Env,
+		isvc.Spec.Predictor.Model.Env = utils.AppendEnvVarIfNotExists(isvc.Spec.Predictor.Model.Env,
 			v1.EnvVar{
 				Name:  constants.MLServerLoadModelsStartupEnv,
 				Value: strconv.FormatBool(false),
 			},
 		)
 	} else {
-		isvc.Spec.Predictor.Model.Env = append(isvc.Spec.Predictor.Model.Env,
+		isvc.Spec.Predictor.Model.Env = utils.AppendEnvVarIfNotExists(isvc.Spec.Predictor.Model.Env,
 			v1.EnvVar{
 				Name:  constants.MLServerModelNameEnv,
 				Value: isvc.Name,

diff --git a/pkg/apis/serving/v1beta1/openapi_generated.go b/pkg/apis/serving/v1beta1/openapi_generated.go
diff --git a/pkg/apis/serving/v1beta1/predictor.go b/pkg/apis/serving/v1beta1/predictor.go
@@ -27,7 +27,6 @@ import (
 // PredictorImplementation defines common functions for all predictors e.g Tensorflow, Triton, etc
 // +kubebuilder:object:generate=false
 type PredictorImplementation interface {
-	IsFrameworkSupported(framework string, config *InferenceServicesConfig) bool
 }
 
 // PredictorSpec defines the configuration for a predictor,

diff --git a/pkg/apis/serving/v1beta1/predictor_custom.go b/pkg/apis/serving/v1beta1/predictor_custom.go
@@ -84,7 +84,7 @@ func (c *CustomPredictor) GetStorageSpec() *StorageSpec {
 	return nil
 }
 
-// GetContainers transforms the resource into a container spec
+// GetContainer transforms the resource into a container spec
 func (c *CustomPredictor) GetContainer(metadata metav1.ObjectMeta, extensions *ComponentExtensionSpec, config *InferenceServicesConfig) *v1.Container {
 	return &c.Containers[0]
 }
@@ -97,19 +97,3 @@ func (c *CustomPredictor) GetProtocol() constants.InferenceServiceProtocol {
 	}
 	return constants.ProtocolV1
 }
-
-func (c *CustomPredictor) IsMMS(config *InferenceServicesConfig) bool {
-	// Check container env if MULTI_MODEL_SERVER env var is set to true
-	container := c.Containers[0]
-	for _, envVar := range container.Env {
-		if envVar.Name == constants.CustomSpecMultiModelServerEnvVarKey && envVar.Value == "true" {
-			return true
-		}
-	}
-	return false
-}
-
-func (c *CustomPredictor) IsFrameworkSupported(framework string, config *InferenceServicesConfig) bool {
-	//TODO: Figure out how to check if custom predictor is supports framework
-	return true
-}
diff --git a/pkg/apis/serving/v1beta1/predictor_custom_test.go b/pkg/apis/serving/v1beta1/predictor_custom_test.go
@@ -286,94 +286,6 @@ func TestCreateCustomPredictorContainer(t *testing.T) {
 	}
 }
 
-func TestCustomPredictorIsMMS(t *testing.T) {
-	g := gomega.NewGomegaWithT(t)
-	config := InferenceServicesConfig{}
-
-	defaultResource = v1.ResourceList{
-		v1.ResourceCPU:    resource.MustParse("1"),
-		v1.ResourceMemory: resource.MustParse("2Gi"),
-	}
-
-	mmsCase := false
-	scenarios := map[string]struct {
-		spec     PredictorSpec
-		expected bool
-	}{
-		"DefaultResources": {
-			spec: PredictorSpec{
-				PodSpec: PodSpec{
-					Containers: []v1.Container{
-						{
-							Env: []v1.EnvVar{
-								{
-									Name:  "STORAGE_URI",
-									Value: "hdfs://modelzoo",
-								},
-							},
-						},
-					},
-				},
-			},
-			expected: mmsCase,
-		},
-	}
-
-	for name, scenario := range scenarios {
-		t.Run(name, func(t *testing.T) {
-			customPredictor := NewCustomPredictor(&scenario.spec.PodSpec)
-			res := customPredictor.IsMMS(&config)
-			if !g.Expect(res).To(gomega.Equal(scenario.expected)) {
-				t.Errorf("got %t, want %t", res, scenario.expected)
-			}
-		})
-	}
-}
-
-func TestCustomPredictorIsFrameworkSupported(t *testing.T) {
-	g := gomega.NewGomegaWithT(t)
-	framework := "framework"
-	config := InferenceServicesConfig{}
-
-	defaultResource = v1.ResourceList{
-		v1.ResourceCPU:    resource.MustParse("1"),
-		v1.ResourceMemory: resource.MustParse("2Gi"),
-	}
-
-	scenarios := map[string]struct {
-		spec     PredictorSpec
-		expected bool
-	}{
-		"DefaultResources": {
-			spec: PredictorSpec{
-				PodSpec: PodSpec{
-					Containers: []v1.Container{
-						{
-							Env: []v1.EnvVar{
-								{
-									Name:  "STORAGE_URI",
-									Value: "hdfs://modelzoo",
-								},
-							},
-						},
-					},
-				},
-			},
-			expected: true,
-		},
-	}
-
-	for name, scenario := range scenarios {
-		t.Run(name, func(t *testing.T) {
-			customPredictor := NewCustomPredictor(&scenario.spec.PodSpec)
-			res := customPredictor.IsFrameworkSupported(framework, &config)
-			if !g.Expect(res).To(gomega.Equal(scenario.expected)) {
-				t.Errorf("got %t, want %t", res, scenario.expected)
-			}
-		})
-	}
-}
-
 func TestCustomPredictorGetProtocol(t *testing.T) {
 	g := gomega.NewGomegaWithT(t)
 	scenarios := map[string]struct {

diff --git a/pkg/controller/v1beta1/inferenceservice/controller_test.go b/pkg/controller/v1beta1/inferenceservice/controller_test.go
@@ -201,8 +201,8 @@ var _ = Describe("v1beta1 inference service controller", func() {
 								},
 								Annotations: map[string]string{
 									constants.StorageInitializerSourceUriInternalAnnotationKey: *isvc.Spec.Predictor.Model.StorageURI,
-									"autoscaling.knative.dev/max-scale":                         "3",
-									"autoscaling.knative.dev/min-scale":                         "1",
+									"autoscaling.knative.dev/max-scale":                        "3",
+									"autoscaling.knative.dev/min-scale":                        "1",
 									"autoscaling.knative.dev/class":                            "kpa.autoscaling.knative.dev",
 									"key1":                                                     "val1FromSR",
 								},
@@ -467,7 +467,7 @@ var _ = Describe("v1beta1 inference service controller", func() {
 									constants.KServiceComponentLabel: constants.Transformer.String(),
 								},
 								Annotations: map[string]string{
-									"autoscaling.knative.dev/class":    "kpa.autoscaling.knative.dev",
+									"autoscaling.knative.dev/class":     "kpa.autoscaling.knative.dev",
 									"autoscaling.knative.dev/max-scale": "3",
 									"autoscaling.knative.dev/min-scale": "1",
 								},
@@ -688,8 +688,8 @@ var _ = Describe("v1beta1 inference service controller", func() {
 								},
 								Annotations: map[string]string{
 									"autoscaling.knative.dev/class":                            "kpa.autoscaling.knative.dev",
-									"autoscaling.knative.dev/max-scale":                         "3",
-									"autoscaling.knative.dev/min-scale":                         "1",
+									"autoscaling.knative.dev/max-scale":                        "3",
+									"autoscaling.knative.dev/min-scale":                        "1",
 									"internal.serving.kserve.io/storage-initializer-sourceuri": "s3://test/mnist/explainer",
 								},
 							},
@@ -1261,8 +1261,8 @@ var _ = Describe("v1beta1 inference service controller", func() {
 								Annotations: map[string]string{
 									constants.StorageInitializerSourceUriInternalAnnotationKey: "s3://test/mnist/export",
 									"autoscaling.knative.dev/class":                            "kpa.autoscaling.knative.dev",
-									"autoscaling.knative.dev/max-scale":                         "3",
-									"autoscaling.knative.dev/min-scale":                         "1",
+									"autoscaling.knative.dev/max-scale":                        "3",
+									"autoscaling.knative.dev/min-scale":                        "1",
 									"key1":                                                     "val1FromSR",
 									"key2":                                                     "val2FromISVC",
 								},

diff --git a/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go b/pkg/controller/v1beta1/inferenceservice/reconcilers/ingress/ingress_reconciler.go
@@ -378,16 +378,18 @@ func (ir *IngressReconciler) Reconcile(isvc *v1beta1.InferenceService, disableIs
 	if url, err := apis.ParseURL(serviceUrl); err == nil {
 		isvc.Status.URL = url
 		path := ""
+		modelName := isvcutils.GetModelName(isvc)
 		if isvc.Spec.Transformer != nil {
 			// As of now transformer only supports protocol V1
-			path = constants.PredictPath(isvc.Name, constants.ProtocolV1)
+			path = constants.PredictPath(modelName, constants.ProtocolV1)
 		} else if !isvcutils.IsMMSPredictor(&isvc.Spec.Predictor) {
 
 			protocol := isvc.Spec.Predictor.GetImplementation().GetProtocol()
+
 			if protocol == constants.ProtocolV1 {
-				path = constants.PredictPath(isvc.Name, constants.ProtocolV1)
+				path = constants.PredictPath(modelName, constants.ProtocolV1)
 			} else if protocol == constants.ProtocolV2 {
-				path = constants.PredictPath(isvc.Name, constants.ProtocolV2)
+				path = constants.PredictPath(modelName, constants.ProtocolV2)
 			}
 
 		}