Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Kserve documents and Tests - issue# 2311 #2355

Merged
merged 14 commits into from
Feb 7, 2023
Merged

Kserve documents and Tests - issue# 2311 #2355

merged 14 commits into from
Feb 7, 2023

Conversation

andyi2it
Copy link
Contributor

@andyi2it andyi2it commented Jan 12, 2023 •
edited
Loading

Which issue is resolved by this Pull Request:
Resolves #2311 for kserve

Description of your changes:

  • Adds Readme and Upgrade docs for kserve.
  • Adds e2e for kserve
  • Updates kserve manifests to v0.10

Checklist:

  • Unit tests pass:
    Make sure you have installed kustomize == 3.2.1
    1. make generate-changed-only
    2. make test

@andyi2it andyi2it changed the title Issue 2311 Kserve documents and Tests - issue# 2311 Jan 13, 2023
@yuzisun
Copy link
Member

yuzisun commented Jan 20, 2023

@kimwnasptd Can you help run the CI test?

@andyi2it
Add README.md
59b5fd6 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Add UPGRADE.md, makefile
baf9c18 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Add support for upgrading models web app, add tests and update docs
95e63fb 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Improve ci/cd workflow test
8effb5e 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
fix makefile, readme, and upgrade.md
29dae0a 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Add notes to readme
0710991 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Update leader election name to have kserve reference
c45e798
@andyi2it
Remove cluster local gateway
1002b97 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Update manifests to kserve 0.10
74e2748 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@juliusvonkohout
Copy link
Member

Are you sure that you are not mixing up cluster-local-gateway with knative-local-gateway? I think cluster-local-gateway should stay.

@andyi2it
Copy link
Contributor Author

@juliusvonkohout
Thanks for pointing out. Sorry, there was a misunderstanding on my part. Will fix it.

@andyi2it
Add istio cluster-local-gateway and remove knative gateway
adc00b8 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@kimwnasptd kimwnasptd mentioned this pull request Jan 31, 2023
Closed
@kimwnasptd kimwnasptd mentioned this pull request Jan 31, 2023
Merged
annajung
annajung reviewed Jan 31, 2023
View reviewed changes
Copy link
Member

@annajung annajung left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this! I know this is still in draft, but did a quick review in case it helps!

.github/workflows/kserve_kind_test.yaml Show resolved Hide resolved
.github/workflows/kserve_kind_test.yaml Show resolved Hide resolved
contrib/kserve/tests/requirements.txt Outdated Show resolved Hide resolved
@annajung annajung mentioned this pull request Jan 31, 2023
Closed
@andyi2it
Try fixing issue with knative installation
69edcb8 
Most likely a separate PR?

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it andyi2it force-pushed the issue-2311 branch 2 times, most recently from 5bdd8b2 to ed8a095 Compare February 2, 2023 18:50
@kimwnasptd
Copy link
Member

@andyi2it it looks like the test is failing with a timeout connection

requests.exceptions.ConnectionError: HTTPConnectionPool(host='10.96.249.222', port=80): Max retries exceeded with url: /v1/models/isvc-sklearn:predict (Caused by NewConnectionError('<urllib3.connection.HTTPConnection object at 0x7f85d1ea80a0>: Failed to establish a new connection: [Errno 110] Connection timed out'))

I think the issue is with how it the test code gets the Service IP and tries to hit that directly. I have a hunch we'll need to make some proxying here to reach the ISVC. Will try to take a look and run it locally as well

@kimwnasptd
Copy link
Member

@andyi2it indeed, after running this locally fails as well. Looks like the code tries to use the Cluster IP of the istio-ingressgateway Service. But with KinD this IP is not accessible to the local machine AFAIK.

Since we'd like to have the updated KServe to cut the RC, could you:

  1. Update this PR and just remove the last part that runs the pytest script
  2. Keep the previous logic that applies the ISVC and waits for it to become ready
  3. Send a follow-up PR just for extending the test to query the ISVC, and we keep debugging there

This is to unblock the KF 1.7 RC and continue with a smaller scoped PR just for the test

@andyi2it
Add port forwarding
a8d966a 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it
Changed models web app name in conditional wait
861d36c 
Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
@andyi2it andyi2it marked this pull request as ready for review February 7, 2023 06:31
@andyi2it
Copy link
Contributor Author

andyi2it commented Feb 7, 2023

@kimwnasptd
Sorry, I had a fix for it. Had to add port forwarding, couldn't push yesterday.
Have pushed it and should be working now, have tested in a separate fork.

@kimwnasptd
Copy link
Member

Everything worked as expected, amazing work @andyi2it!

/lgtm
/approve

@google-oss-prow google-oss-prow bot added the lgtm label Feb 7, 2023
@google-oss-prow
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyi2it, kimwnasptd

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:

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

@google-oss-prow google-oss-prow bot merged commit f7dcb1c into kubeflow:master Feb 7, 2023
kevin85421 pushed a commit to juliusvonkohout/manifests that referenced this pull request Feb 28, 2023
@andyi2it @kevin85421
Kserve documents and Tests - issue# 2311 (kubeflow#2355)
1e064d7 
* Add README.md

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Add UPGRADE.md, makefile

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Add support for upgrading models web app, add tests and update docs

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Improve ci/cd workflow test

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* fix makefile, readme, and upgrade.md

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Add notes to readme

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Update leader election name to have kserve reference

* Remove cluster local gateway

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Update manifests to kserve 0.10

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Add istio cluster-local-gateway and remove knative gateway

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Try fixing issue with knative installation

Most likely a separate PR?

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Update docs

* Add port forwarding

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

* Changed models web app name in conditional wait

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>

---------

Signed-off-by: Andrews Arokiam <andrews.arokiam@ideas2it.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@annajung annajung annajung left review comments

@kimwnasptd kimwnasptd kimwnasptd left review comments

@yuzisun yuzisun yuzisun left review comments

@elikatsis elikatsis Awaiting requested review from elikatsis

@juliusvonkohout juliusvonkohout Awaiting requested review from juliusvonkohout

Assignees

@kimwnasptd kimwnasptd

Labels
approved lgtm size/XXL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tracking for /contrib components evaluation and clean up
5 participants
@andyi2it @yuzisun @juliusvonkohout @kimwnasptd @annajung