Agents protocol updates #4775
Agents protocol updates #4775
Conversation
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Signed-off-by: Haytham Abuelfutuh <[email protected]>
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #4775 +/- ##
==========================================
+ Coverage 58.50% 59.02% +0.52%
==========================================
Files 494 645 +151
Lines 42788 55176 +12388
==========================================
+ Hits 25032 32568 +7536
- Misses 15759 20029 +4270
- Partials 1997 2579 +582
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Haytham Abuelfutuh <[email protected]>
| @@ -21,6 +20,12 @@ enum State { | |||
| SUCCEEDED = 4; | |||
| } | |||
|
|
|||
| // TaskOverrides is a set of overrides that can be applied to a task. | |||
| message TaskOverrides { | |||
There was a problem hiding this comment.
did you not want to use this -
just to keep in separate and name clean? (I agree)
| }; | ||
|
|
||
| // CreateTask sends a task create request to the agent service. | ||
| rpc CreateTask (stream flyteidl.admin.CreateTaskRequest) returns (flyteidl.admin.CreateTaskResponse){ |
There was a problem hiding this comment.
why should this be a stream? isnt that potentially dangerous?
| // included in this map. If not required and not provided, defaults apply. | ||
| // +optional | ||
| core.LiteralMap inputs = 1; | ||
| oneof part { |
There was a problem hiding this comment.
this feels odd and does not seem to have a clear understanding of what will be sent.
Inputs and header should always be sent i feel.
IMO, CreateTask should remain sync, we can add a ExecuteSyncStream that sounds like a better option if we need stream
| // Prefix for where task output data will be written. (e.g. s3://my-bucket/randomstring) | ||
| string output_prefix = 3; | ||
| string output_prefix = 2; |
| // Get job status. | ||
| rpc GetTask (flyteidl.admin.GetTaskRequest) returns (flyteidl.admin.GetTaskResponse){}; | ||
| rpc GetTask (flyteidl.admin.GetTaskRequest) returns (stream flyteidl.admin.GetTaskResponse){ |
There was a problem hiding this comment.
this stream also seems odd?
Maybe this is the problem with async vs sync.
Async being a stream is really odd?
| // Send a task create request to the agent server. | ||
| rpc CreateTask (flyteidl.admin.CreateTaskRequest) returns (flyteidl.admin.CreateTaskResponse){}; | ||
| // ExecuteTaskSync streams the create request and inputs to the agent service and streams the outputs back. | ||
| rpc ExecuteTaskSync (stream flyteidl.admin.ExecuteTaskSyncRequest) returns (stream flyteidl.admin.ExecuteTaskSyncResponse){ |
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
Signed-off-by: Kevin Su <[email protected]>
|
closing in favor of #4874 |
Tracking issue
https://github.com/flyteorg/flyte/issues/
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link