Out of core plugin

[pingsutw]

fixes flyteorg/flyte#3282

TL;DR

Define a new service and message for out of core plugin.
These messages are used by propeller to send the create / get / delete request to the backend plugin server

https://hackmd.io/k_hMtUsGTbKl2IksC3IjkA

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

^^^

Tracking Issue

flyteorg/flyte#3282

Follow-up issue

NA

[codecov]

Codecov Report

Merging #378 (731ab97) into master (bb5d4cd) will increase coverage by 2.37%.
The diff coverage is n/a.

❗ Current head 731ab97 differs from pull request most recent head 04e587c. Consider uploading reports for the commit 04e587c to get more accurate results

@@            Coverage Diff             @@
##           master     #378      +/-   ##
==========================================
+ Coverage   76.11%   78.49%   +2.37%     
==========================================
  Files          18       18              
  Lines        1390     1195     -195     
==========================================
- Hits         1058      938     -120     
+ Misses        280      205      -75     
  Partials       52       52              

Flag	Coverage Δ
unittests	?

Flags with carried forward coverage won't be shown. Click here to find out more.

see 18 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[hamersaw]

Looks great! The only two things I'm still not grasping are:

1. Where does the task type multiplexing happen? Each request here has a task_type variable which makes it sound like there will be a single server running that handles all requests and just has multiple plugins registered. However the flyteplugins PR has an EndpointForTaskTypes flag where you specify separate endpoints for each task type - is the idea to support both use-cases?
2. All but the delete responses have an error_message field which implies failure. We changes this in the create fucntion to a one_of meaning that the create either succeeded or failed, but this wasn't changed in the get response message. Is there a reason to add this explicit message or can we rely on the gRPC error handling to pass failures from server to client?

[pingsutw]

re multiplexing: both. In flyte, we will ship all the plugin in flyte repo to one plugin container. it also much easier for us to update the single binary and helm chart. Some users may want submit the job to different grpc server for development, so we'd like to support both. see more discussion here.

re error_message: yup, you're right. we should rely on the gRPC error handling. I just removed all the error message from idl. will update the grpc server in flytekit to return error code and message.

[hamersaw]

nit - does some like ExternalPluginService make more sense?

[pingsutw]

like it!

[hamersaw]

This seems to be a reasonable MVP. Would love another reviewer to go through in some depth though!