InferenceService v1beta1 API #991

yuzisun · 2020-08-03T13:20:27Z

What this PR does / why we need it:
InferenceService v1beta1 API and InferenceRouter v1alpha1 preview

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
I am putting inference service and router together mainly for folks to review with a wholistic perspective.

v1beta1 inference service api should be good for review.
v1alpha1 inference router api is for preview, mainly focused on the pipeline spec.

Release note:

kubeflow-bot · 2020-08-03T13:20:38Z

This change is

google-cla · 2020-08-03T13:20:43Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-03T13:45:06Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

ellistarn · 2020-08-03T18:31:42Z

Any way to separate some of these types to review them separately in a smaller PR?

yuzisun · 2020-08-03T19:34:38Z

Any way to separate some of these types to review them separately in a smaller PR?

yes will separate router out to separate PR once getting initial feedback, I want to have a single place for people to review both as an overall plan.

Makefile

docs/samples/v1beta1/custom/simple.yaml

docs/samples/v1beta1/tensorflow/liveness_probe.yaml

ellistarn · 2020-08-04T21:09:44Z

pkg/apis/serving/v1alpha2/explainer.go

@@ -20,8 +20,7 @@ import (
 	"k8s.io/klog"
 )

-// +k8s:openapi-gen=false
-// +k8s:deepcopy-gen=false
+// +kubebuilder:object:generate=false


We should consider moving these interfaces into pkg/controller or somewhere else. The interfaces can be implemented by our types, but defined in a separate package to keep the generation simple.

pkg/apis/serving/v1beta1/configmap.go

ellistarn · 2020-08-04T21:13:16Z

pkg/apis/serving/v1beta1/configmap.go

+func getExplainersConfigs(configMap *v1.ConfigMap) (*ExplainersConfig, error) {
+	explainerConfig := &ExplainersConfig{}
+	if data, ok := configMap.Data[ExplainerConfigKeyName]; ok {
+		err := json.Unmarshal([]byte(data), &explainerConfig)


yaml.DisallowUnknownFields

is this an unmarshal option?

Yep. It tends to save users from headaches.

does not look like that json unmarshal has this option

https://golang.org/pkg/encoding/json/#Decoder.DisallowUnknownFields

pkg/apis/serving/v1beta1/predictor.go

ukclivecox · 2020-08-04T21:19:49Z

pkg/apis/routing/v1alpha1/inference_router.go

+	// Pipeline defines a series of steps which are depending on previous step.
+	// Only one root node is allowed and all other routes should specify dependencies with the consumes field.
+	// The graph should form a valid DAG, no cycles are allowed, branches are allowed but they needs
+	// be mutually exclusive. Each route on the pipeline can be either an InferenceService or an Inference Router.


Would we not allow any Destination (that supports the Data Plane)?

ukclivecox · 2020-08-04T21:29:04Z

pkg/apis/routing/v1alpha1/inference_router.go

+	Headers map[string]*StringMatch `json:"headers,omitempty"`
+	// This is a list of pipeline route names that should be executed before the current route.
+	// The field is only valid for pipeline routing type
+	Consumes []string `json:"consumes,omitempty"`


Not sure I like the Consumes which is only used in Pipeline. Seems confusing. Maybe can rethink later?

sure, open for ideas. The advantage is to reuse the same RouteSpec and not introducing a new pipeline CRD, or you can think of other routes types all have an implicit consumes for the root.

I might simply remove Consumes, for now. I think the whole model of defining dependencies might be the wrong direction. It results in a really challenging to understand semantic. You can build dags as follows:

Pipeline1 { Pipeline 2, Pipeline 3} Pipeline2 { Transformer1, InferenceService1 } Pipeline3{ Transformer1, InferenceService2 }

That means each pipeline is itself linear, but can be constructed into more complex shapes.

ukclivecox · 2020-08-04T21:29:40Z

pkg/apis/routing/v1alpha1/inference_router.go

+	// - `exact: "value"` for exact string match
+	// - `prefix: "value"` for prefix-based match
+	// - `regex: "value"` for regex-based match
+	Headers map[string]*StringMatch `json:"headers,omitempty"`


Just focusing on headers with simple matches makes sense. I assume this is the "Condition" field of the Pipeline design v1?

correct, I'd like to see how far header matching can go before we introduce more advanced matching.

njhill · 2020-08-05T05:27:07Z

Just a note that I would like to review this but may take a bit of time to get through it!

google-cla · 2020-08-08T18:24:18Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-08T23:48:25Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

google-cla · 2020-08-08T23:56:09Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

ellistarn · 2020-08-09T00:35:15Z

docs/samples/v1beta1/tensorflow/canary.yaml

+  name: "tensorflow-canary"
+spec:
+  predictor:
+    canaryTrafficPercent: 20


This is sweet. I'm a huge fan.

ellistarn · 2020-08-09T00:37:13Z

pkg/apis/serving/v1beta1/configmap.go

+
+// +kubebuilder:object:generate=false
+type TransformersConfig struct {
+	Feast TransformerConfig `json:"feast,omitempty"`


Love to see this.

pkg/apis/serving/v1beta1/configmap.go

pkg/apis/serving/v1beta1/explainer.go

google-cla · 2020-08-11T05:01:15Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

wengyao04 · 2020-08-11T19:52:27Z

@googlebot I consent

ellistarn · 2020-08-11T23:13:25Z

pkg/apis/serving/v1beta1/component.go

+// Components interface is implemented by predictor, transformer and explainer
+// +kubebuilder:object:generate=false
+type Component interface {
+	GetContainer(metadata metav1.ObjectMeta, containerConcurrency *int64, config *InferenceServicesConfig) *v1.Container


Now that containerconcurrency is in the component itself, do we need to pass this through?

it is on the ComponentExtensionSpec not the model server spec, ModelServerSpec maps to the container.

ellistarn

Nice work! Let's get this submitted.
/approve

ellistarn · 2020-08-11T23:18:43Z

pkg/apis/serving/v1beta1/configmap.go

+}
+
+// +kubebuilder:object:generate=false
+type InferenceServicesConfig struct {


very small nit: reading this again, I might call it InferenceServiceConfig, likewise for the method.

ellistarn · 2020-08-11T23:29:45Z

pkg/apis/serving/v1beta1/explainer.go

+	return nil
+}
+
+func (e *ExplainerSpec) GetExplainer() []Component {


I don't want to keep thrashing on this validation code, but it would probably be cleaner to have:

// Public method used to access explainer implementation. Uses returns getExplainers()[0]
GetExplainer() Component
// Private method used for internal validation
validateOnlyOneExplainer() error
// Private method used by validator and accessor
getExplainers() []Component

k8s-ci-robot · 2020-08-12T02:17:53Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ellistarn

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~~ [ellistarn]

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

k8s-ci-robot added the size/XXL label Aug 3, 2020

k8s-ci-robot requested review from jinchihe and pugangxa August 3, 2020 13:20

yuzisun mentioned this pull request Aug 3, 2020

Create KFServing's Stable Requirements using the Kubeflow Community's process as defined in the Application Requirement's template #737

Closed

ellistarn reviewed Aug 4, 2020

View reviewed changes

Makefile Outdated Show resolved Hide resolved

ellistarn reviewed Aug 4, 2020

View reviewed changes

docs/samples/v1beta1/custom/simple.yaml Outdated Show resolved Hide resolved

ellistarn reviewed Aug 4, 2020

View reviewed changes

docs/samples/v1beta1/tensorflow/liveness_probe.yaml Outdated Show resolved Hide resolved

ellistarn reviewed Aug 4, 2020

View reviewed changes

pkg/apis/serving/v1beta1/configmap.go Show resolved Hide resolved

ellistarn reviewed Aug 4, 2020

View reviewed changes

pkg/apis/serving/v1beta1/predictor.go Show resolved Hide resolved

ukclivecox reviewed Aug 4, 2020

View reviewed changes

wengyao04 mentioned this pull request Aug 6, 2020

Add multimodel configmap #992

Merged

yuzisun changed the title ~~InferenceService v1beta1 API and InferenceRouter v1alpha1 preview~~ InferenceService v1beta1 API Aug 8, 2020

ellistarn reviewed Aug 9, 2020

View reviewed changes

pkg/apis/serving/v1beta1/configmap.go Outdated Show resolved Hide resolved

ellistarn reviewed Aug 9, 2020

View reviewed changes

pkg/apis/serving/v1beta1/explainer.go Outdated Show resolved Hide resolved

ellistarn reviewed Aug 9, 2020

View reviewed changes

pkg/apis/serving/v1beta1/explainer.go Outdated Show resolved Hide resolved

yuzisun added 20 commits August 11, 2020 01:00

torch serve container

90a197f

Add model server tests

dd0e43d

Add pipeline

7ef746a

Fix v1beta1 examples

958c103

Add comments on inference route type

124fead

Move to advanced

9e10dea

Add onnx test

90727d0

Fix logger

07a5018

Add defaulter tests

514e30d

Separate inference router

7884fde

Code generator for inference service

197517a

Add component interface

7d128f0

Add explainer test

783fa40

Add transformer tests

d2dfc9e

Add predictor test

7628084

Add explainer test

2e1e7b7

Make runtimeVersion as *string

2354a5c

Use reflector for interface nil check

5a5500e

Add defauler tests

65fbae9

Add validation tests

b79c0fe

ellistarn reviewed Aug 11, 2020

View reviewed changes

Remove inference router

550d2da

ellistarn reviewed Aug 12, 2020

View reviewed changes

k8s-ci-robot added the approved label Aug 12, 2020

Use helm v3.0.2

f0553a7

yuzisun merged commit e133d7f into kserve:master Aug 12, 2020

yuzisun mentioned this pull request Aug 24, 2020

Implement KFServing v1beta1 controller #1042

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

InferenceService v1beta1 API #991

InferenceService v1beta1 API #991

yuzisun commented Aug 3, 2020 •

edited

kubeflow-bot commented Aug 3, 2020

google-cla bot commented Aug 3, 2020

google-cla bot commented Aug 3, 2020

ellistarn commented Aug 3, 2020

yuzisun commented Aug 3, 2020

ellistarn Aug 4, 2020

ellistarn Aug 4, 2020

yuzisun Aug 4, 2020

ellistarn Aug 8, 2020 •

edited

yuzisun Aug 8, 2020

ellistarn Aug 9, 2020

ukclivecox Aug 4, 2020

ukclivecox Aug 4, 2020

yuzisun Aug 4, 2020

ellistarn Aug 8, 2020 •

edited

ukclivecox Aug 4, 2020

yuzisun Aug 4, 2020

njhill commented Aug 5, 2020

google-cla bot commented Aug 8, 2020

google-cla bot commented Aug 8, 2020

google-cla bot commented Aug 8, 2020

ellistarn Aug 9, 2020

ellistarn Aug 9, 2020

google-cla bot commented Aug 11, 2020

wengyao04 commented Aug 11, 2020

ellistarn Aug 11, 2020

yuzisun Aug 11, 2020

ellistarn left a comment

ellistarn Aug 11, 2020

ellistarn Aug 11, 2020

k8s-ci-robot commented Aug 12, 2020

InferenceService v1beta1 API #991

InferenceService v1beta1 API #991

Conversation

yuzisun commented Aug 3, 2020 • edited

kubeflow-bot commented Aug 3, 2020

google-cla bot commented Aug 3, 2020

google-cla bot commented Aug 3, 2020

ellistarn commented Aug 3, 2020

yuzisun commented Aug 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn Aug 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn Aug 8, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

njhill commented Aug 5, 2020

google-cla bot commented Aug 8, 2020

google-cla bot commented Aug 8, 2020

google-cla bot commented Aug 8, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

google-cla bot commented Aug 11, 2020

wengyao04 commented Aug 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ellistarn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 12, 2020

yuzisun commented Aug 3, 2020 •

edited

ellistarn Aug 8, 2020 •

edited

ellistarn Aug 8, 2020 •

edited