Skip to content

Commit

Permalink
fix: set non-empty modelPath for http storageURI (#382)
Browse files Browse the repository at this point in the history
#### 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: kserve/modelmesh-runtime-adapter#41 (comment)

#### 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 <[email protected]>
Signed-off-by: Christian Kadner <[email protected]>

Co-authored-by: Christian Kadner <[email protected]>
  • Loading branch information
tjohnson31415 and ckadner authored Jun 2, 2023
1 parent 4808804 commit 4a47d56
Show file tree
Hide file tree
Showing 6 changed files with 53 additions and 5 deletions.
2 changes: 1 addition & 1 deletion fvt/storage/storage_suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
28 changes: 28 additions & 0 deletions fvt/storage/storage_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,17 @@ 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",
"isvc-pvc3": "isvc-pvc-3.yaml",
"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"}

Expand All @@ -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 {
Expand Down
12 changes: 12 additions & 0 deletions fvt/testdata/isvcs/isvc-https.yaml
Original file line number Diff line number Diff line change
@@ -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"
6 changes: 3 additions & 3 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -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=
Expand All @@ -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=
Expand Down Expand Up @@ -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=
Expand Down
4 changes: 3 additions & 1 deletion pkg/predictor_source/inferenceservice_registry.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down

0 comments on commit 4a47d56

Please sign in to comment.