Fix TODOs and improve testing code #31

yuzisun · 2019-04-19T06:37:22Z

populate default/canary traffic percent in status
refactor testing code to reuse some of the boilerplate test types in knative_service_test.go
fix the reconcile status expectation TODOs in kfservice_controller_test.go

This change is

yuzisun · 2019-04-19T11:52:50Z

/assign @ellis-bigelow @cliveseldon @gaocegege

ellistarn · 2019-04-19T15:54:32Z

pkg/controller/kfservice/kfservice_controller.go

@@ -159,6 +159,15 @@ func (r *ReconcileService) updateStatus(before *kfservingv1alpha1.KFService, ksv
 	} else {
 		after.Status.Canary.Name = ksvc.Status.LatestCreatedRevisionName
 	}
+	for _, traffic := range ksvc.Status.Traffic {


Is it possible that default + canary wouldn't sum to 100?

it could be 0 when default is not in ready status(does not pass readiness probe), but as soon as default is ready the traffic should sum to 100, for example when default is ready and user adds a canary which does not pass the readiness probe, 100% traffic stays with default, as soon as canary goes into ready state the traffic splits between the two and sum to 100.

ellistarn · 2019-04-19T15:55:27Z

pkg/controller/kfservice/kfservice_controller_test.go

-		ObjectMeta: metav1.ObjectMeta{
-			Name:      "foo",
-			Namespace: "default",
+var instance = &servingv1alpha1.KFService{


There's a SampleTFProto in the types file. Do you think it would be a good pattern to import it for tests or better to be explicit?

I prefer to be explicit in place as when the imported type changes test can get broken.

pkg/controller/kfservice/kfservice_controller_test.go

ellistarn · 2019-04-19T16:00:13Z

Mostly lgtm

pkg/controller/kfservice/kfservice_controller.go

ukclivecox · 2019-04-19T17:35:04Z

Very useful tests I can add to for custom container work.

ellistarn · 2019-04-22T20:05:00Z

/lgtm
/approve

k8s-ci-robot · 2019-04-22T20:05:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ellis-bigelow

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~~ [ellis-bigelow]

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

* KServe Go module and API group change -------------------------------------- Updated go source files with new go module. Changed apigroup in yaml files. Made changes in Makefile to represent the repository and apigroup names. Verified `make test` is passing. Removed reference for old names. * Pointed correct repo reference for ko images (#23) * Rename changes python - New (#19) * python changes-initial-commit * updated api gen. Renamed package references in python. * Renamed python/kfserving to python/kserve * renamed python folder according to package kserve * Ran client-gen. Updated swagger.json * renamed constants from KFSERVING_ to KSERVE_. Yet to change namespace from kfserving-system to kserve and hence not changed in python side. * Updated kfserving reference to kserve Updated version for Python Package Index to 0.7.0rc0 Renamed KFServingSampleModel to KServeSampleModel * Python package and corresponding directory change has to be reflected in test and dockerfiles * updated a specific version in requirments to address issue with pip resolver. Added more specific versions in requirements,txt and updated paddleserver setup.py with kserve dependency. * Changed namespace from kfserving-system to kserve (#31) * Update local dev scripts (#33) * initial commit of dev-scripts update * Updated quick-install * Updated python Makefile * Generated install for new release * Updated generate-install script with new release * Reverting controller name in makefile. Will be updated in separate pr. * Github action changes (#34) * Github action changes Migrated images publish to kserve repo. Added github workflows for the ones that was being pushed to gcr.io from release/triggers. * Removed batcher publisher since batcher.Dockerfile does not exist Updated tf2openapi.Dockerfile to correct directory and package. * Fixed issue with working directory of tf2openapi workflow * Removed the deptrecated logger docker * Excluded tests from linting. (#36) They were pointing to older directory and package names and hence were not being excluded while linting. * Update license (#20) * Fixed errors thrown by flake8 linter (#37) * Kfserving kserve manifests changes and code reference changes (#39) * kfserving -> kserve migartion initial commit * kfserving -> kserve config changes * Changed constants. Updated image reference from kserve to andyarok * Reverted kserve models web app image repo. * go changes for kfserving-kserve * Updated KFServing refernce in python code * Updated KFServing to KServe in python doc comments. Updated references of v1alpha2 from duplicate aliases to just use pkg name. Updated docs samples references with proper name. Made changes to use Constant for container name instead of literal "kserve-container". * Updated gitignore for generated files that came in after folder rename. Removed the generated files. Fixed lint warnings for go. * Removed travis yml file. * KServe doc update (#38) * Update main and pythonserver readme * Update python docs * Fix role binding * Update kserve sdk doc * Fix e2e test script * Fix import * Kserve error fix (#40) * Fixed linting errors in python * Fixed make test. Issue with storage initializer name/path. * Fixed failing python test. Updated ray version. Updated KFServingClient reference to KServeClient in kserve init. * Updated ray version from 1.4.0 to 1.5.0 * Fixed error with e2e due to kfserving reference in test overlay (#42) Updated quick install and added 0.7.0-rc0 install manifests. Updated e2e namespace. * Controller repo update (#45) Made changes in workflow to update image repo to kserve instead of kfserving * Controller repo update (#45) Made changes in workflow to update image repo to kserve instead of kfserving * update pytorch image * Fix transformer dockerfile Co-authored-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Co-authored-by: andyi2it <87992092+andyi2it@users.noreply.github.com>

* Change e2e test job to point to kserve repo * Fix prow config * Remove dir dep * fix image-transformer built image tag (#44) * fix image-transformer built image tag Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com> * rename ecr registry directory for kserve Signed-off-by: Theofilos Papapanagiotou <theofilos@gmail.com> * merging kserve branch to master (#35) * KServe Go module and API group change -------------------------------------- Updated go source files with new go module. Changed apigroup in yaml files. Made changes in Makefile to represent the repository and apigroup names. Verified `make test` is passing. Removed reference for old names. * Pointed correct repo reference for ko images (#23) * Rename changes python - New (#19) * python changes-initial-commit * updated api gen. Renamed package references in python. * Renamed python/kfserving to python/kserve * renamed python folder according to package kserve * Ran client-gen. Updated swagger.json * renamed constants from KFSERVING_ to KSERVE_. Yet to change namespace from kfserving-system to kserve and hence not changed in python side. * Updated kfserving reference to kserve Updated version for Python Package Index to 0.7.0rc0 Renamed KFServingSampleModel to KServeSampleModel * Python package and corresponding directory change has to be reflected in test and dockerfiles * updated a specific version in requirments to address issue with pip resolver. Added more specific versions in requirements,txt and updated paddleserver setup.py with kserve dependency. * Changed namespace from kfserving-system to kserve (#31) * Update local dev scripts (#33) * initial commit of dev-scripts update * Updated quick-install * Updated python Makefile * Generated install for new release * Updated generate-install script with new release * Reverting controller name in makefile. Will be updated in separate pr. * Github action changes (#34) * Github action changes Migrated images publish to kserve repo. Added github workflows for the ones that was being pushed to gcr.io from release/triggers. * Removed batcher publisher since batcher.Dockerfile does not exist Updated tf2openapi.Dockerfile to correct directory and package. * Fixed issue with working directory of tf2openapi workflow * Removed the deptrecated logger docker * Excluded tests from linting. (#36) They were pointing to older directory and package names and hence were not being excluded while linting. * Update license (#20) * Fixed errors thrown by flake8 linter (#37) * Kfserving kserve manifests changes and code reference changes (#39) * kfserving -> kserve migartion initial commit * kfserving -> kserve config changes * Changed constants. Updated image reference from kserve to andyarok * Reverted kserve models web app image repo. * go changes for kfserving-kserve * Updated KFServing refernce in python code * Updated KFServing to KServe in python doc comments. Updated references of v1alpha2 from duplicate aliases to just use pkg name. Updated docs samples references with proper name. Made changes to use Constant for container name instead of literal "kserve-container". * Updated gitignore for generated files that came in after folder rename. Removed the generated files. Fixed lint warnings for go. * Removed travis yml file. * KServe doc update (#38) * Update main and pythonserver readme * Update python docs * Fix role binding * Update kserve sdk doc * Fix e2e test script * Fix import * Kserve error fix (#40) * Fixed linting errors in python * Fixed make test. Issue with storage initializer name/path. * Fixed failing python test. Updated ray version. Updated KFServingClient reference to KServeClient in kserve init. * Updated ray version from 1.4.0 to 1.5.0 * Fixed error with e2e due to kfserving reference in test overlay (#42) Updated quick install and added 0.7.0-rc0 install manifests. Updated e2e namespace. * Controller repo update (#45) Made changes in workflow to update image repo to kserve instead of kfserving * Controller repo update (#45) Made changes in workflow to update image repo to kserve instead of kfserving * update pytorch image * Fix transformer dockerfile Co-authored-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Co-authored-by: andyi2it <87992092+andyi2it@users.noreply.github.com> * Generate 0.7.0 release candidate and update roadmap/release process (#46) * Fix apiVersion for examples and docs (#55) Fix apiVersion for examples and docs change from serving.kubeflow.org to serving.kserve.io * add default deployment and fix startup without knative to master branch (#57) * add default deployment and fix startup without knative * fix suite_test error * fix suit_test error * add constant vb for RawDeployment and Serverless * update predictor string RawDeployment to constants.RawDeployment * modify getDeploymentMode return type * Update kserve layer diagram (#58) * update diag (#59) * Remove kubeflow copyright (#56) * Adding kfs_architect.drawio and changing kfs_architect.png's "KFserving" name to "KServe" (#60) * Delete kfs_architect.png * Add files via upload Adding kfs_architect.drawio and changing kfs_architect.png's "KFserving" name to "KServe" * Delete kfs_architect.png * Delete kfs_architect.drawio * added Metric scrapping title * Delete kfs_architect.png * Delete kfs_architect.drawio * fixed typo * add Nick Hill as a reviewer (#66) also removing rkelkar * Separate out web-app repo (#68) * Sepate out web-app repo * Remove models web-app github action * Fix feast example Co-authored-by: Theofilos Papapanagiotou <theofilos@gmail.com> Co-authored-by: Andrews Arokiam <andrews.arokiam@ideas2it.com> Co-authored-by: andyi2it <87992092+andyi2it@users.noreply.github.com> Co-authored-by: Chin Huang <chhuang@us.ibm.com> Co-authored-by: Qingshan Chen <38182824+Iamlovingit@users.noreply.github.com> Co-authored-by: Animesh Singh <singhan@us.ibm.com> Co-authored-by: js-ts <79689323+js-ts@users.noreply.github.com>

* Add LICENSE file * Delete LICENSE * Create LICENSE * Delete kserve_layer.png * Updated kserve_layer file

k8s-ci-robot requested review from animeshsingh and gaocegege April 19, 2019 06:37

k8s-ci-robot added the size/L label Apr 19, 2019

k8s-ci-robot assigned ukclivecox, ellistarn and gaocegege Apr 19, 2019

ellistarn reviewed Apr 19, 2019

View reviewed changes

pkg/controller/kfservice/kfservice_controller_test.go Show resolved Hide resolved

ukclivecox reviewed Apr 19, 2019

View reviewed changes

pkg/controller/kfservice/kfservice_controller.go Show resolved Hide resolved

yuzisun mentioned this pull request Apr 22, 2019

Race condition on kfservice_controller_test #44

Closed

Fix TODOs and improve testing code

5fabc79

k8s-ci-robot added the lgtm label Apr 22, 2019

k8s-ci-robot added the approved label Apr 22, 2019

k8s-ci-robot merged commit 5345763 into kserve:master Apr 22, 2019

yuzisun deleted the knservice branch May 4, 2019 19:52

ellistarn pushed a commit to ellistarn/kfserving that referenced this pull request Jul 28, 2020

Fix TODOs and improve testing code (kserve#31)

f32cf05

Surya-smart619 pushed a commit to Surya-smart619/kserve that referenced this pull request May 11, 2022

Add LICENSES (kserve#31)

1e6857f

* Add LICENSE file * Delete LICENSE * Create LICENSE * Delete kserve_layer.png * Updated kserve_layer file

rafvasq pushed a commit to rafvasq/kserve that referenced this pull request Jul 21, 2023

fix broken links in trainedmodel.md (kserve#31)

60d96f9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix TODOs and improve testing code #31

Fix TODOs and improve testing code #31

yuzisun commented Apr 19, 2019 •

edited by jlewi

yuzisun commented Apr 19, 2019

ellistarn Apr 19, 2019

yuzisun Apr 21, 2019

ellistarn Apr 19, 2019

yuzisun Apr 21, 2019

ellistarn commented Apr 19, 2019

ukclivecox commented Apr 19, 2019

ellistarn commented Apr 22, 2019

k8s-ci-robot commented Apr 22, 2019

Fix TODOs and improve testing code #31

Fix TODOs and improve testing code #31

Conversation

yuzisun commented Apr 19, 2019 • edited by jlewi

yuzisun commented Apr 19, 2019

ellistarn Apr 19, 2019

Choose a reason for hiding this comment

yuzisun Apr 21, 2019

Choose a reason for hiding this comment

ellistarn Apr 19, 2019

Choose a reason for hiding this comment

yuzisun Apr 21, 2019

Choose a reason for hiding this comment

ellistarn commented Apr 19, 2019

ukclivecox commented Apr 19, 2019

ellistarn commented Apr 22, 2019

k8s-ci-robot commented Apr 22, 2019

yuzisun commented Apr 19, 2019 •

edited by jlewi