From 4a47d56372b95468e6a5061d0c4b18a9d2362a31 Mon Sep 17 00:00:00 2001 From: Travis Johnson Date: Thu, 1 Jun 2023 18:15:42 -0600 Subject: [PATCH] fix: set non-empty modelPath for http storageURI (#382) #### Motivation When using the `storageURI` form of an HTTP (non Azure blob) address to download a model, the `modelPath` needs to be an non-empty string. Before this change, the `storageURI: http://models.r.us/my-model.json` form would be equivalent to the following `storage` spec: ``` storage: type: http path: '' # this being empty is problematic for later processing parameters: url: http://models.r.us/path/to/my-model.json ``` The http storage type is currently the only way to have a valid storage configuration with an empty `path` (mainly because it has a "url" parameter that could include the full path). That said, I'm not sure if we should make a `path` required for the HTTP storage type. In particular, if the `url` is just `http://models.r.us/`, there is no path portion. Related: https://github.com/kserve/modelmesh-runtime-adapter/issues/41#issuecomment-1564909111 #### Modifications Set `modelPath` to the URL's Path and set the `url` parameter to not have the URL Path. #### Result With these changes, the `storageURI` example above changes to have a `path` field: ``` storage: type: http path: path/to/my-model.json parameters: url: http://models.r.us/ ``` Signed-off-by: Travis Johnson Signed-off-by: Christian Kadner Co-authored-by: Christian Kadner --- fvt/storage/storage_suite_test.go | 2 +- fvt/storage/storage_test.go | 28 +++++++++++++++++++ fvt/testdata/isvcs/isvc-https.yaml | 12 ++++++++ go.mod | 6 ++-- go.sum | 6 ++++ .../inferenceservice_registry.go | 4 ++- 6 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 fvt/testdata/isvcs/isvc-https.yaml diff --git a/fvt/storage/storage_suite_test.go b/fvt/storage/storage_suite_test.go index be0b8a84..2ec60695 100644 --- a/fvt/storage/storage_suite_test.go +++ b/fvt/storage/storage_suite_test.go @@ -58,7 +58,7 @@ var _ = SynchronizedBeforeSuite(func() []byte { // ensure a stable deploy state, on each process since we updated the storage config Log.Info("Waiting for stable deploy state") - WaitForStableActiveDeployState(time.Second * 60) + WaitForStableActiveDeployState(time.Second * 120) return nil }, func(_ []byte) { diff --git a/fvt/storage/storage_test.go b/fvt/storage/storage_test.go index 888b7e7b..2b586fcd 100644 --- a/fvt/storage/storage_test.go +++ b/fvt/storage/storage_test.go @@ -24,6 +24,7 @@ import ( ) var isvcFiles = map[string]string{ + "isvc-https": "isvc-https.yaml", "isvc-pvc-storage-uri": "isvc-pvc-uri.yaml", "isvc-pvc-storage-path": "isvc-pvc-path.yaml", "isvc-pvc2": "isvc-pvc-2.yaml", @@ -31,6 +32,9 @@ var isvcFiles = map[string]string{ "isvc-pvc4": "isvc-pvc-4.yaml", } +// ISVC using a model from GitHub via HTTPS URI +var isvcViaHttps = "isvc-https" + // ISVCs using PVCs from the FVT `storage-config` Secret (config/dependencies/fvt.yaml) var isvcWithPvcInStorageConfig = []string{"isvc-pvc-storage-uri", "isvc-pvc-storage-path", "isvc-pvc2"} @@ -43,6 +47,30 @@ var isvcWithNonExistentPvc = "isvc-pvc4" var _ = Describe("ISVCs", func() { + Describe("with HTTPS storage URI", Ordered, func() { + var isvcName string + var fileName string + + BeforeAll(func() { + isvcName = isvcViaHttps + fileName = isvcFiles[isvcName] + }) + + It("should successfully load a model", func() { + isvcObject := NewIsvcForFVT(fileName) + isvcName = isvcObject.GetName() + CreateIsvcAndWaitAndExpectReady(isvcObject, PredictorTimeout) + }) + + It("should successfully run inference", func() { + ExpectSuccessfulInference_sklearnMnistSvm(isvcName) + }) + + AfterAll(func() { + FVTClientInstance.DeleteIsvc(isvcName) + }) + }) + Describe("with PVC in storage-config", Ordered, func() { for _, name := range isvcWithPvcInStorageConfig { diff --git a/fvt/testdata/isvcs/isvc-https.yaml b/fvt/testdata/isvcs/isvc-https.yaml new file mode 100644 index 00000000..2de7c7d1 --- /dev/null +++ b/fvt/testdata/isvcs/isvc-https.yaml @@ -0,0 +1,12 @@ +apiVersion: serving.kserve.io/v1beta1 +kind: InferenceService +metadata: + name: isvc-https + annotations: + serving.kserve.io/deploymentMode: ModelMesh +spec: + predictor: + model: + modelFormat: + name: sklearn + storageUri: "https://github.com/kserve/modelmesh-minio-examples/blob/main/sklearn/mnist-svm.joblib?raw=true" diff --git a/go.mod b/go.mod index 0650e67e..f6f64a37 100644 --- a/go.mod +++ b/go.mod @@ -11,8 +11,8 @@ require ( github.com/manifestival/controller-runtime-client v0.4.0 github.com/manifestival/manifestival v0.7.1 github.com/moverest/mnist v0.0.0-20160628192128-ec5d9d203b59 - github.com/onsi/ginkgo/v2 v2.9.2 - github.com/onsi/gomega v1.27.6 + github.com/onsi/ginkgo/v2 v2.9.7 + github.com/onsi/gomega v1.27.7 github.com/operator-framework/operator-lib v0.10.0 github.com/pkg/errors v0.9.1 github.com/prometheus-operator/prometheus-operator/pkg/apis/monitoring v0.55.0 @@ -105,7 +105,7 @@ require ( golang.org/x/term v0.8.0 // indirect golang.org/x/text v0.9.0 // indirect golang.org/x/time v0.3.0 // indirect - golang.org/x/tools v0.8.0 // indirect + golang.org/x/tools v0.9.1 // indirect golang.org/x/xerrors v0.0.0-20220907171357-04be3eba64a2 // indirect gomodules.xyz/jsonpatch/v2 v2.2.0 // indirect google.golang.org/api v0.122.0 // indirect diff --git a/go.sum b/go.sum index 40961e71..d608f5b6 100644 --- a/go.sum +++ b/go.sum @@ -480,6 +480,8 @@ github.com/onsi/ginkgo/v2 v2.0.0/go.mod h1:vw5CSIxN1JObi/U8gcbwft7ZxR2dgaR70JSE3 github.com/onsi/ginkgo/v2 v2.1.3/go.mod h1:vw5CSIxN1JObi/U8gcbwft7ZxR2dgaR70JSE3/PpL4c= github.com/onsi/ginkgo/v2 v2.9.2 h1:BA2GMJOtfGAfagzYtrAlufIP0lq6QERkFmHLMLPwFSU= github.com/onsi/ginkgo/v2 v2.9.2/go.mod h1:WHcJJG2dIlcCqVfBAwUCrJxSPFb6v4azBwgxeMeDuts= +github.com/onsi/ginkgo/v2 v2.9.7 h1:06xGQy5www2oN160RtEZoTvnP2sPhEfePYmCDc2szss= +github.com/onsi/ginkgo/v2 v2.9.7/go.mod h1:cxrmXWykAwTwhQsJOPfdIDiJ+l2RYq7U8hFU+M/1uw0= github.com/onsi/gomega v0.0.0-20170829124025-dcabb60a477c/go.mod h1:C1qb7wdrVGGVU+Z6iS04AVkA3Q65CEZX59MT0QO5uiA= github.com/onsi/gomega v0.0.0-20190113212917-5533ce8a0da3/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= github.com/onsi/gomega v1.7.0/go.mod h1:ex+gbHU/CVuBBDIJjb2X0qEXbFg53c61hWP/1CpauHY= @@ -490,6 +492,8 @@ github.com/onsi/gomega v1.17.0/go.mod h1:HnhC7FXeEQY45zxNK3PPoIUhzk/80Xly9PcubAl github.com/onsi/gomega v1.18.1/go.mod h1:0q+aL8jAiMXy9hbwj2mr5GziHiwhAIQpFmmtT5hitRs= github.com/onsi/gomega v1.27.6 h1:ENqfyGeS5AX/rlXDd/ETokDz93u0YufY1Pgxuy/PvWE= github.com/onsi/gomega v1.27.6/go.mod h1:PIQNjfQwkP3aQAH7lf7j87O/5FiNr+ZR8+ipb+qQlhg= +github.com/onsi/gomega v1.27.7 h1:fVih9JD6ogIiHUN6ePK7HJidyEDpWGVB5mzM7cWNXoU= +github.com/onsi/gomega v1.27.7/go.mod h1:1p8OOlwo2iUUDsHnOrjE5UKYJ+e3W8eQ3qSlRahPmr4= github.com/opencontainers/go-digest v1.0.0 h1:apOUWs51W5PlhuyGyz9FCeeBIOUDA/6nW8Oi/yOhh5U= github.com/opencontainers/go-digest v1.0.0/go.mod h1:0JzlMkj0TRzQZfJkVvzbP0HBR3IKzErnv2BNG4W4MAM= github.com/operator-framework/operator-lib v0.10.0 h1:tTjrt8Udi0msABkMpgxKHp7sXKnC73jFPO5Col0tWso= @@ -903,6 +907,8 @@ golang.org/x/tools v0.1.5/go.mod h1:o0xws9oXOQQZyjljx8fwUC0k7L1pTE6eaCbjGeHmOkk= golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= golang.org/x/tools v0.8.0 h1:vSDcovVPld282ceKgDimkRSC8kpaH1dgyc9UMzlt84Y= golang.org/x/tools v0.8.0/go.mod h1:JxBZ99ISMI5ViVkT1tr6tdNmXeTrcpVSD3vZ1RsRdN4= +golang.org/x/tools v0.9.1 h1:8WMNJAz3zrtPmnYC7ISf5dEn3MT0gY7jBJfw27yrrLo= +golang.org/x/tools v0.9.1/go.mod h1:owI94Op576fPu3cIGQeHs3joujW/2Oc6MtlxbF5dfNc= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191011141410-1b5146add898/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= diff --git a/pkg/predictor_source/inferenceservice_registry.go b/pkg/predictor_source/inferenceservice_registry.go index da617dfe..726b6784 100644 --- a/pkg/predictor_source/inferenceservice_registry.go +++ b/pkg/predictor_source/inferenceservice_registry.go @@ -188,8 +188,10 @@ func processInferenceServiceStorage(inferenceService *v1beta1.InferenceService, uriParameters["account_name"] = hostParts[0] modelPath = strings.Join(pathParts[1:], "/") } else { + modelPath = strings.TrimPrefix(u.Path, "/") + u.Path = "" uriParameters["type"] = "http" - uriParameters["url"] = *storageUri + uriParameters["url"] = u.String() } default: err = fmt.Errorf("the InferenceService %v has an unsupported storageUri scheme %v", nname, u.Scheme)