feat: storage phase 1 for inference service reconciler #56

Tomcli · 2021-10-16T00:59:08Z

Motivation

rebase #32 to the new inference service reconciler for model mesh

For Storage Spec details, please refer to the design doc: https://docs.google.com/document/d/1rYNh93XNMRp8YaR56m-_zK3bqpZhhGKgpVbLPPsi-Ow/edit#

Additional storages/parameters support will come in phase 2.

Modifications

Result

Tomcli · 2021-10-16T01:23:32Z

@njhill @pvaneck Quick question, is the schemaPath only used in Pytorch framework? Right now I have schemaPath as optional parameter as part of the storage Spec, but let me know if these kinds of parameters are more framework specific so I can also change it.

In Kserve, the pytorch server assumes both model and graph .mar files are under the model-store folder instead of pointing to a single file.
https://console.cloud.google.com/storage/browser/kfserving-examples/models/torchserve/image_classifier;tab=objects?prefix=&forceOnObjectsSortingFiltering=false&pageState=(%22StorageObjectListTable%22:(%22f%22:%22%255B%255D%22))

In Kserve triton server, it assumes the path is pointing to a folder with structure similar to tensorflow's savedmodel format.
https://console.cloud.google.com/storage/browser/kfserving-examples/models/torchscript?pageState=(%22StorageObjectListTable%22:(%22f%22:%22%255B%255D%22))&prefix=&forceOnObjectsSortingFiltering=false

I think the schemaPath is useful for backward capability since pytorch didn't have a model saving standard, but I feel like this might be too much to add it as part of the storage spec.

yuzisun · 2021-10-16T02:20:42Z

apis/serving/v1beta1/inferenceservice_types.go

+	Storage *StorageSpec `json:"storage,omitempty"`
+}
+
+type StorageSpec struct {


Can we discuss this? we do not want to maintain two versions of InferenceService

I'm going to create the same spec update to the kserve repo once we agreed on how to handle some of the current edge cases on modelmesh storage. The long term goal is to move the modelmesh serving code as part of the kserve reconciler.

@pvaneck I remember in your InferenceService PR #34, you copied the spec because it was during the kserve migration. Will there be any challenges for you to import the inferenceservice types from the kserve package now?

Tomcli · 2021-10-18T18:03:31Z

@njhill @pvaneck Quick question, is the schemaPath only used in Pytorch framework? Right now I have schemaPath as optional parameter as part of the storage Spec, but let me know if these kinds of parameters are more framework specific so I can also change it.

In Kserve, the pytorch server assumes both model and graph .mar files are under the model-store folder instead of pointing to a single file. https://console.cloud.google.com/storage/browser/kfserving-examples/models/torchserve/image_classifier;tab=objects?prefix=&forceOnObjectsSortingFiltering=false&pageState=(%22StorageObjectListTable%22:(%22f%22:%22%255B%255D%22))

In Kserve triton server, it assumes the path is pointing to a folder with structure similar to tensorflow's savedmodel format. https://console.cloud.google.com/storage/browser/kfserving-examples/models/torchscript?pageState=(%22StorageObjectListTable%22:(%22f%22:%22%255B%255D%22))&prefix=&forceOnObjectsSortingFiltering=false

I think the schemaPath is useful for backward capability since pytorch didn't have a model saving standard, but I feel like this might be too much to add it as part of the storage spec.

We have a conversation on this and the schemaPath should be generic for all frameworks. The definition of the schemaPath is located here and ultimately provide a generic way for any model server to know the schema payload.

njhill

Thanks @Tomcli, I made a few comments inline

docs/inferenceservice.md

pkg/predictor_source/inferenceservice_registry.go

njhill · 2021-10-26T21:32:45Z

pkg/predictor_source/inferenceservice_registry.go

+	secretKey := inferenceService.ObjectMeta.Annotations[v1beta1.SecretKeyAnnotation]
+	if schemaPath, ok := inferenceService.ObjectMeta.Annotations[v1beta1.SchemaPathAnnotation]; ok {
+		p.Spec.SchemaPath = &schemaPath
+	}


There appears to be duplication here with below. It would be best to move the annotation checks into the ProcessInferenceServiceStorage function. And only need to check for the annotations if the corresponding explicit fields aren't set.

njhill · 2021-10-26T21:37:14Z

Re schemaPath addition, that's correct it would apply (potentially) to any kind of model, but might be used differently depending on the corresponding model server / adapter. The idea is to abstract how the input/output schema is provided so that InferenceService creator doesn't need to structure their storage in a server impl-specific way (but they still can do that if desired). This is one of the things we'd like to get thoughts on from others, hopefully on the upcoming community call.

njhill

Thanks @Tomcli, sorry for the delay in re-reviewing!

pkg/predictor_source/inferenceservice_registry.go

Tomcli · 2021-11-12T23:22:14Z

thanks @njhill for your reviews

pkg/predictor_source/inferenceservice_registry.go

njhill · 2021-11-16T22:39:45Z

config/default/kustomization.yaml

@@ -94,3 +94,4 @@ resources:
  - ../crd
  - ../rbac
  - ../manager
+namespace: modelmesh-serving


This looks unrelated to the PR?

njhill · 2021-11-16T22:43:31Z

@Tomcli are you sure we want to include the new serving.kserve.io_inferenceservices.yaml file at this point (it's huge)?

May be better to omit it until the other kserve changes are ready? The PR could still be merged, I don't think the new logic will break anything when used with the existing CRD... the storage spec will just appear empty.

njhill

Hey @Tomcli sorry I noticed a few more things

pkg/predictor_source/inferenceservice_registry.go

Tomcli · 2021-11-17T01:04:18Z

thanks @njhill for your reviews. I updated based on your comments

pkg/predictor_source/inferenceservice_registry.go

Tomcli · 2021-11-20T01:40:32Z

Thanks @njhill I addressed the pointer changes.

njhill

Thanks @Tomcli

kserve-oss-bot · 2021-11-22T21:10:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: njhill, Tomcli

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:

~~OWNERS~~ [njhill]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

njhill · 2021-11-22T21:11:57Z

/lgtm

Tomcli added 2 commits October 15, 2021 17:43

add isvc storage spec

9ebe14b

lint config

d2c5b7d

kserve-oss-bot requested review from animeshsingh and pvaneck October 16, 2021 00:59

yuzisun reviewed Oct 16, 2021

View reviewed changes

This was referenced Oct 22, 2021

[WIP] storage phase 1 for modelmesh reconciler #32

Closed

Propose new storage spec to support next-gen multi-model serving kserve/kserve#1814

Closed

njhill reviewed Oct 26, 2021

View reviewed changes

address comments

db063f8

njhill reviewed Nov 9, 2021

View reviewed changes

pkg/predictor_source/inferenceservice_registry.go Outdated Show resolved Hide resolved

pkg/predictor_source/inferenceservice_registry.go Outdated Show resolved Hide resolved

pkg/predictor_source/inferenceservice_registry.go Outdated Show resolved Hide resolved

address comments

0065b49

njhill reviewed Nov 16, 2021

View reviewed changes

pkg/predictor_source/inferenceservice_registry.go Outdated Show resolved Hide resolved

njhill reviewed Nov 16, 2021

View reviewed changes

Tomcli added 3 commits November 16, 2021 15:56

address comments

df1cf85

fix markdown lint

6c57788

fix markdown lint

4e38c7f

njhill reviewed Nov 19, 2021

View reviewed changes

pkg/predictor_source/inferenceservice_registry.go Show resolved Hide resolved

address comments

c49d4e9

njhill approved these changes Nov 22, 2021

View reviewed changes

kserve-oss-bot added the approved label Nov 22, 2021

njhill changed the title ~~storage phase 1 for inference service reconciler~~ feat: storage phase 1 for inference service reconciler Nov 22, 2021

kserve-oss-bot assigned njhill Nov 22, 2021

kserve-oss-bot added the lgtm label Nov 22, 2021

kserve-oss-bot merged commit be8f87f into kserve:main Nov 22, 2021

tjohnson31415 mentioned this pull request Dec 3, 2021

feat: add and use new StorageSpec fields in Predictors #83

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: storage phase 1 for inference service reconciler #56

feat: storage phase 1 for inference service reconciler #56

Tomcli commented Oct 16, 2021

Tomcli commented Oct 16, 2021 •

edited

Loading

yuzisun Oct 16, 2021

Tomcli Oct 18, 2021

Tomcli Oct 18, 2021

Tomcli commented Oct 18, 2021

njhill left a comment

njhill Oct 26, 2021

njhill commented Oct 26, 2021

njhill left a comment

Tomcli commented Nov 12, 2021

njhill Nov 16, 2021

njhill commented Nov 16, 2021

njhill left a comment

Tomcli commented Nov 17, 2021

Tomcli commented Nov 20, 2021

njhill left a comment

kserve-oss-bot commented Nov 22, 2021

njhill commented Nov 22, 2021

feat: storage phase 1 for inference service reconciler #56

feat: storage phase 1 for inference service reconciler #56

Conversation

Tomcli commented Oct 16, 2021

Motivation

Modifications

Result

Tomcli commented Oct 16, 2021 • edited Loading

yuzisun Oct 16, 2021

Choose a reason for hiding this comment

Tomcli Oct 18, 2021

Choose a reason for hiding this comment

Tomcli Oct 18, 2021

Choose a reason for hiding this comment

Tomcli commented Oct 18, 2021

njhill left a comment

Choose a reason for hiding this comment

njhill Oct 26, 2021

Choose a reason for hiding this comment

njhill commented Oct 26, 2021

njhill left a comment

Choose a reason for hiding this comment

Tomcli commented Nov 12, 2021

njhill Nov 16, 2021

Choose a reason for hiding this comment

njhill commented Nov 16, 2021

njhill left a comment

Choose a reason for hiding this comment

Tomcli commented Nov 17, 2021

Tomcli commented Nov 20, 2021

njhill left a comment

Choose a reason for hiding this comment

kserve-oss-bot commented Nov 22, 2021

njhill commented Nov 22, 2021

Tomcli commented Oct 16, 2021 •

edited

Loading