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.

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

Allow both http and grpc #2574

Merged
merged 27 commits into from Nov 16, 2020
Merged

Allow both http and grpc #2574

merged 27 commits into from Nov 16, 2020

Conversation

ukclivecox
Copy link
Contributor

@ukclivecox ukclivecox commented Oct 24, 2020

What this PR does / why we need it:

  • Updates python wrapper, operator, executor, prepackaged servers to support both REST and gRPC at same time

Which issue(s) this PR fixes:

Fixes #2378
Fixes #2299

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Python models will need to be rebuilt to take advantage of the exposing of both HTTP and gRPC.
Upgrading the operator will cause an upgrade to all models running.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@seldondev
Copy link
Collaborator

Sun Oct 25 11:52:17 UTC 2020
The logs for [pr-build] [3] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/3.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=3

@seldondev
Copy link
Collaborator

Sun Oct 25 11:52:27 UTC 2020
The logs for [lint] [4] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/4.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=4

@ukclivecox
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

Sun Oct 25 12:13:09 UTC 2020
The logs for [integration] [5] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/5.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=5

@ukclivecox
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

Sun Oct 25 15:05:02 UTC 2020
The logs for [integration] [6] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/6.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=6

@ukclivecox
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

Sun Oct 25 17:25:49 UTC 2020
The logs for [integration] [7] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/7.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=7

@ukclivecox
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

Sun Oct 25 18:22:39 UTC 2020
The logs for [lint] [9] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/9.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=9

@seldondev
Copy link
Collaborator

Sun Oct 25 18:22:53 UTC 2020
The logs for [pr-build] [8] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/8.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=8

@seldondev
Copy link
Collaborator

Sun Oct 25 18:24:44 UTC 2020
The logs for [integration] [10] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/10.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=10

@ukclivecox
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

Mon Oct 26 10:45:32 UTC 2020
The logs for [pr-build] [13] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/13.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=13

@seldondev
Copy link
Collaborator

Mon Oct 26 10:45:36 UTC 2020
The logs for [lint] [14] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/14.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=14

@seldondev
Copy link
Collaborator

Mon Oct 26 10:47:38 UTC 2020
The logs for [integration] [15] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/15.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=15

@ukclivecox
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

Mon Oct 26 13:39:27 UTC 2020
The logs for [pr-build] [16] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/16.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=16

@seldondev
Copy link
Collaborator

Mon Oct 26 13:40:15 UTC 2020
The logs for [integration] [18] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/18.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=18

@RafalSkolasinski
Copy link
Member

RafalSkolasinski commented Nov 11, 2020

Checking backward compatibility now: took a model that was working fine for 1.4.0 in both gRPC and REST flavour.

Built with wrapper 1.4.0 and deployed with this branch's operator / executor:

  • REST flavour works completely fine
  • gRPC does not start -> debugging now

Built with this branch's wrapper:

  • REST flavour does not start: seldon-core-microservice: error: unrecognized arguments: REST
  • gRPC flavour does not start: seldon-core-microservice: error: unrecognized arguments: GRPC

@RafalSkolasinski
Copy link
Member

Built with this branch's wrapper:

  • REST flavour does not start: seldon-core-microservice: error: unrecognized arguments: REST
  • gRPC flavour does not start: seldon-core-microservice: error: unrecognized arguments: GRPC

We could allow second positional argument to be set, ignore it, and log deprecation warning.
This is probably only issue if someone is building image using Dockerfile and not s2i mechanism though.

Copy link
Member

@RafalSkolasinski RafalSkolasinski left a comment

Choose a reason for hiding this comment

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

Massive but extremely useful addition!

doc/source/reference/upgrading.md Outdated Show resolved Hide resolved
"cell_type": "markdown",
"metadata": {},
"source": [
"# Backwards Compatability Examples with Different Protocols\n",
Copy link
Member

Choose a reason for hiding this comment

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

Extending notebook to cover gRPC will be useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -19,138 +19,9 @@
"pip install seldon-core\n",
Copy link
Member

Choose a reason for hiding this comment

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

Section about training locally and wrapping to test with Docker was quite useful, IMO. Very introductory to wrapping models.


Reply via ReviewNB

@@ -0,0 +1,985 @@
{
Copy link
Member

Choose a reason for hiding this comment

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

Not sure what is prepackaged iris testing here, it will be just deployed using old server image so it's just

"old operator + "old sklearn server"


Reply via ReviewNB

@@ -20,6 +20,20 @@
"```"
Copy link
Member

Choose a reason for hiding this comment

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

Nice one, may be worth contributing back this magic to Jupyter :)


Reply via ReviewNB

@@ -228,7 +204,6 @@ def main():
sys.path.append(os.getcwd())
parser = argparse.ArgumentParser()
parser.add_argument("interface_name", type=str, help="Name of the user interface.")
parser.add_argument("api_type", type=str, choices=["REST", "GRPC", "FBS"])
Copy link
Member

Choose a reason for hiding this comment

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

We should mention removal of api_type argument in upgrading section.

@@ -298,10 +273,17 @@ def main():
)

parser.add_argument(
"--port",
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, we should mention change in wrapper API in upgrading section.

@RafalSkolasinski
Copy link
Member

RafalSkolasinski commented Nov 11, 2020

Built with wrapper 1.4.0 and deployed with this branch's operator / executor:

  • REST flavour works completely fine
  • gRPC does not start -> debugging now

It seems that in case of gRPC model it is instructed properly to serve gRPC traffic on port 9500 but executor tries to check 9000/ 9001:

{"level":"error","ts":1605106209.0915442,"logger":"SeldonRestApi","msg":"Ready check failed","error":"dial tcp [::1]:9001: connect: connection refused" .....

(it's 9001 as I am looking at two-node model)

@seldondev
Copy link
Collaborator

Thu Nov 12 08:20:53 UTC 2020
The logs for [pr-build] [72] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/72.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=72

@seldondev
Copy link
Collaborator

Thu Nov 12 08:20:59 UTC 2020
The logs for [lint] [73] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/73.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=73

@seldondev
Copy link
Collaborator

Thu Nov 12 14:42:38 UTC 2020
The logs for [pr-build] [74] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/74.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=74

@seldondev
Copy link
Collaborator

Thu Nov 12 14:42:48 UTC 2020
The logs for [lint] [75] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/75.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=75

@seldondev
Copy link
Collaborator

Thu Nov 12 14:43:45 UTC 2020
The logs for [pr-build] [76] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/76.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=76

@seldondev
Copy link
Collaborator

Thu Nov 12 14:44:01 UTC 2020
The logs for [lint] [77] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/77.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=77

@ukclivecox
Copy link
Contributor Author

/test integration

@seldondev
Copy link
Collaborator

Thu Nov 12 14:58:52 UTC 2020
The logs for [integration] [78] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/78.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=78

@ukclivecox
Copy link
Contributor Author

/test notebooks

@seldondev
Copy link
Collaborator

Fri Nov 13 07:52:06 UTC 2020
The logs for [notebooks] [79] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/79.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=79

@axsaucedo
Copy link
Contributor

/approve

@seldondev
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: axsaucedo

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

@seldondev
Copy link
Collaborator

Mon Nov 16 14:16:57 UTC 2020
The logs for [lint] [81] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/81.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=81

@seldondev
Copy link
Collaborator

Mon Nov 16 14:17:04 UTC 2020
The logs for [pr-build] [80] will show after the pipeline context has finished.
https://github.com/SeldonIO/seldon-core/blob/gh-pages/jenkins-x/logs/SeldonIO/seldon-core/PR-2574/80.log

impatient try
jx get build logs SeldonIO/seldon-core/PR-2574 --build=80

@adriangonz adriangonz merged commit b5e64ad into SeldonIO:master Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support gRPC and HTTP protocols at the same time Allow Inference Graphs to mix Protocols with the Executor
5 participants