Allow different modes of data download and upload in flyte co-pilot #65
Conversation
protos/flyteidl/core/tasks.proto
Outdated
| enum BlobDownload { | ||
| // All data will be downloaded before the main container is executed | ||
| BEFORE_STARTUP = 0; | ||
| // Data will be downloaded as a stream and an End-Of-Stream marker will be written to indicate all data has been downloaded. Refer to protocol for details | ||
| STREAM = 1; | ||
| // Large objects (offloaded) will not be downloaded | ||
| DO_NOT_DOWNLOAD = 2; | ||
| } |
There was a problem hiding this comment.
These names will get all mangled up, right?
They will look something like DataLoadingConfig_BEFORE_STARTUP, DataLoadingConfig_ON_EXIT... at least in golang, you won't know which enum they belong to...
You might want to create a nested message, something like this:
| enum BlobDownload { | |
| // All data will be downloaded before the main container is executed | |
| BEFORE_STARTUP = 0; | |
| // Data will be downloaded as a stream and an End-Of-Stream marker will be written to indicate all data has been downloaded. Refer to protocol for details | |
| STREAM = 1; | |
| // Large objects (offloaded) will not be downloaded | |
| DO_NOT_DOWNLOAD = 2; | |
| } | |
| message BlobDownload { | |
| enum Strategy { | |
| BEFORE_STARTUP = 0; | |
| ... | |
| } | |
| } | |
| BlobDownload.Staregy blob_download_strategy = 5; |
There was a problem hiding this comment.
ya they get mangled up. But creating a message is an overhead? Maybe i can create a message called
message IOStrategy{
enum Download
enum Upload
}
protos/flyteidl/core/tasks.proto
Outdated
| // Data will not be uploaded, only references will be written | ||
| DO_NOT_UPLOAD = 2; | ||
| } | ||
| BlobDownload download_strategy = 5; |
There was a problem hiding this comment.
| BlobDownload download_strategy = 5; | |
| BlobDownload blob_download_strategy = 5; |
protos/flyteidl/core/tasks.proto
Outdated
| // Flag enables DataLoading Config. If this is not set, data loading will not be used! | ||
| bool enabled = 4; | ||
| // Strategy to use when dealing with Blob, Schema, or multipart blob data (large datasets) | ||
| enum BlobDownload { |
There was a problem hiding this comment.
Don't we need the same for Schema type?
Should we have the setting say something like OffloadedTypeDownload.Staregy instead? and apply that to blobs and schemas (and potentially later to array outputs)
There was a problem hiding this comment.
Second, I'd much prefer to not have Blob in the name.
There was a problem hiding this comment.
Can you explain what the use-case for this is in the PR description? We're trying to control whether blobs and schemas get downloaded before the container starts running?
If a Blob input is not necessary for the user container, why is it an input to the task? I'd like to better understand the situations where you want the input but don't want the actual input.
Like would a broader input mask/whitelist work better for these scenarios?
protos/flyteidl/core/tasks.proto
Outdated
| // Flag enables DataLoading Config. If this is not set, data loading will not be used! | ||
| bool enabled = 4; | ||
| // Strategy to use when dealing with Blob, Schema, or multipart blob data (large datasets) | ||
| enum BlobDownload { |
There was a problem hiding this comment.
Second, I'd much prefer to not have Blob in the name.
I am not sure if the description was updated when you looked at the PR, can you look again and let me know
|
|
Cool, merging! |
TL;DR
Blob Handling in CoPilot should have different options. We will implement only one in the beginning) but this provides ease of extensibility
Type
Are all requirements met?
Complete description
Eventually, we want to support Blob download as a stream (overlap with processing), eager (downloaded all at once) or no download (user will handle their own downloading)
We also want to support similar modes for uploads
Tracking Issue
flyteorg/flyte#297
Follow-up issue
NA