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 mixed rest/grpc graphs in new golang based executor #1820

Closed
thjwhite opened this issue May 11, 2020 · 4 comments
Closed

Allow mixed rest/grpc graphs in new golang based executor #1820

thjwhite opened this issue May 11, 2020 · 4 comments
Assignees

Comments

@thjwhite
Copy link

In the newly released version (1.1) one of the breaking changes is to disallow mixed rest/grpc graphs

The reason we would like to have this setup is that we have a model deployed with v1.0 which has a mixed rest/grpc graph. This model is an image processing based model and is data size and latency sensitive. In our testing we were not able to get the full end to end GRPC working within our existing infrastructure components, and full REST had too high of a latency. With GRPC we got the execution under 100ms which met our requirements. There was some latency difference between the mixed protocol graph and pure GRPC, but overall it was still in the acceptable range.

  • The pod itself with full GRPC transport worked correctly, however when integrating with Ambassador and other parts of our infrastructure (AWS ELB, TLS termination etc..) we were not able to make successful connections.
  • There was a configuration where I was able to use "grpcurl" to make the connection but it did not work with the python client. I think this was due to AWS ELB's not fully supporting GRPC, or TLS termination just not being compatible with GRPC and HTTP/2. Particularly the protocol negotiation/upgrade part (ALPN). 1.20.0.pre3 can't connect to server behind ELB (ALPN check) grpc/grpc#18710 . I understand this is not particularly a Seldon issue, but given that you guys support ambassador as a recommended API gateway, I think it is useful information for your users. I will probably make an issue in the Ambassador github relating to this as well if it doesn't already exist.
  • Converting the graph to REST was fairly straight forward but we encountered latency of up to 1 second. I think this is probably due to the conversion between TFTensor/ndarray/json when it get's sent to TFServing. https://github.com/SeldonIO/seldon-core/blob/v1.0.2/integrations/tfserving/TfServingProxy.py#L102

seldon-github-issue

For reference our graph looks like this

      graph:
        children:
        - children:
          - endpoint:
              service_host: localhost
              service_port: 9003
              type: REST
            implementation: TENSORFLOW_SERVER
            modelUri: s3://redacted
            name: redacted
            parameters:
            - name: signature_name
              type: STRING
              value: serving_default
            - name: model_name
              type: STRING
              value: redacted
            - name: model_input
              type: STRING
              value: in
            - name: model_output
              type: STRING
              value: out
            type: MODEL
          endpoint:
            service_host: localhost
            service_port: 9002
            type: REST
          implementation: UNKNOWN_IMPLEMENTATION
          name: output-transformer
          type: OUTPUT_TRANSFORMER
        endpoint:
          service_host: localhost
          service_port: 9001
          type: REST
        implementation: UNKNOWN_IMPLEMENTATION
        name: input-transformer
        type: TRANSFORMER

I'm open to any suggestions, it is entirely possible I am missing something. I am fairly new to using Seldon. In the meantime I think we'll be ok with 1.0.2, however I would like to make the switch to the new golang executor soon, provided we can successfully migrate our models.

@thjwhite thjwhite added the triage Needs to be triaged and prioritised accordingly label May 11, 2020
@thjwhite
Copy link
Author

To clarify, the topology of the infrastructure components looks like this:
(AWS ELB) -> (Ambassador) -> (linkerd service mesh)-> (Seldon Model)

The ELB, Ambassador, and linkerd are configured for TLS transport. The DNS entry given to my client application points to the ELB.

@ukclivecox
Copy link
Contributor

The solution we are trying to move towards is to multiplex REST and grpc. The initial work can be tracked in #1762

@thjwhite
Copy link
Author

Thanks, that's great to hear, let me know if there is anything you need to know with respect to our use case.

@ukclivecox ukclivecox removed the triage Needs to be triaged and prioritised accordingly label May 14, 2020
@ukclivecox
Copy link
Contributor

We allow both REST and gRPC now for all deployments. We might look to have a switch to allow users to externally use REST and internal gRPC bit mixed is not on roadmap

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

3 participants