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

Support custom client protobufs in grpc server #1420

Closed
groszewn opened this issue Feb 7, 2020 · 8 comments
Closed

Support custom client protobufs in grpc server #1420

groszewn opened this issue Feb 7, 2020 · 8 comments

Comments

@groszewn
Copy link
Contributor

groszewn commented Feb 7, 2020

We have been using gRPC internally for most of our models for quite some time, so they already have protobufs that are specific to each model's features. We would like to maintain the strong type safety provided by these custom protobufs across all of our models.

@groszewn groszewn added the triage Needs to be triaged and prioritised accordingly label Feb 7, 2020
@ukclivecox
Copy link
Contributor

With the new executor in master which is in golang this option becomes more likely. Be interesting to discuss how this would be done.

I see golang's v2 protobufs allow more dynamic usage: golang/protobuf#199

@groszewn
Copy link
Contributor Author

groszewn commented Feb 8, 2020

Thoughts on possibly allowing the serialized proto message all the way to the method implementations?

@ukclivecox
Copy link
Contributor

This should already be possible with the latest master as the http protocol is passed through as binary in the executor. So if you launched a pod that could decode the message it should work.

@groszewn
Copy link
Contributor Author

groszewn commented Feb 9, 2020

Gotcha, I mistook executor for the model service container in your previous statement.

I stumbled across an Envoy blog post here that discusses the tradeoffs between using Any and Struct for dynamically extending protobufs. Both of these seem like feasible approaches to introduce as few changes to the current executor and proto specs. In cases where someone would want to use their custom protobuf, the executor can act like a reverse proxy and offload the deserializing and message parsing to the model service container.

@groszewn
Copy link
Contributor Author

@cliveseldon let me know if you have any specific preferences in terms of this implementation. I can try to take a crack at it if we're okay iterating a bit.

@ukclivecox
Copy link
Contributor

So let me summarize the proposed solution to see if I understand:

  • Add a top level custom field to SeldonMessage of type Any in the proto defn
    • I don't see any changes to executor for this other than updated protos? or would the schema URL be checked inside the Any to reject early bad URLs payloads?
  • For the python wrapper you would need to ensure you used the predict_raw as you would need to get access to the custom field and parse the Any

@groszewn
Copy link
Contributor Author

That covers it. I don't see a clean way to make the executor aware of the type URLs as of yet, so it would just be updated protos for the executor.

@ukclivecox ukclivecox removed the triage Needs to be triaged and prioritised accordingly label Feb 13, 2020
@ukclivecox ukclivecox added this to the 1.2 milestone Feb 13, 2020
groszewn added a commit to groszewn/seldon-core that referenced this issue Mar 26, 2020
Adds custom protobuf support to the Seldon protobuf specification using
the `Any` proto type.

Addresses SeldonIO#1420
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 2, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 2, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 2, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 2, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 3, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 4, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 16, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 21, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
groszewn added a commit to groszewn/seldon-core that referenced this issue Apr 21, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses SeldonIO#1420

Signed off by Nick Groszewski (<[email protected]>)
seldondev pushed a commit that referenced this issue Apr 21, 2020
Additional `customData` field added to the prediction protobuf, as well
as updates to the python package to properly pass the raw protobuf
section down to the `user_model` implementation.

Addresses #1420

Signed off by Nick Groszewski (<[email protected]>)
@ukclivecox ukclivecox removed this from the 1.2 milestone Apr 23, 2020
@ukclivecox
Copy link
Contributor

The PR is merged so closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants