Switching from using model Metadata -> TaskMetadata #298
Conversation
TaskMetadata will be maintained as a shadow and allows decoupling of protocol buffer types from contributor code and user code. This allows more flexiblity
kumare3
requested review from
matthewphsmith and
wild-endeavor
as code owners
|
I think I'm not seeing the bigger picture. At least I don't understand how this is a decoupling? Ultimately, at serialization time, the Python class will have to be converted into the model class right? I feel like this is more delaying the coupling rather than decoupling. Which is fine, but I don't understand what this enables. +1 if you want. I think it's okay to use the model sometimes though - like currently we use the dynamic job spec model, literal map and parameter map models, all the literal models, auth, labels, annotations, etc. I don't think we should create parallel classes for all those. |
flytekit/annotated/base_task.py
Outdated
| interruptible: Boolean that indicates that this task is of for interruptions and can be scheduled on nodes | ||
| with lower QoS guarantees. This will directly reduce the `$`/`execution cost` associated, | ||
| at the cost of performance penalties due to potential interruptions | ||
| deprecated: This should be used to indicate that the task is deprecated |
There was a problem hiding this comment.
nit: can you provide suggestions for what the value should be? at first glance i would wonder why it's not a bool
There was a problem hiding this comment.
its the acutal deprecated message, i kept it as is, but will add a comment
flytekit/annotated/base_task.py
Outdated
| Args: | ||
| cache: Boolean that indicates if caching should be enabled | ||
| cache_version: Version string to be used for the cached value | ||
| interruptible: Boolean that indicates that this task is of for interruptions and can be scheduled on nodes |
There was a problem hiding this comment.
"is of for interruptions" ?
There was a problem hiding this comment.
this is just copy pasta, but will fix it
| timeout: Optional[Union[datetime.timedelta, int]] = None | ||
|
|
||
| def __post_init__(self): | ||
| if self.timeout: |
There was a problem hiding this comment.
can you also add a check here if cache is specified but cache_version isn't (and vice-versa)?
| self.timeout = datetime.timedelta(seconds=self.timeout) | ||
| elif not isinstance(self.timeout, datetime.timedelta): | ||
| raise ValueError("timeout should be duration represented as either a datetime.timedelta or int seconds") | ||
| if self.cache and not self.cache_version: |
There was a problem hiding this comment.
what if cache version is set but cache isn't?
There was a problem hiding this comment.
I think this happens a lot of times today (at lyft)
|
can you take a look at test failures? |
* Switching from using model Metadata -> TaskMetadata TaskMetadata will be maintained as a shadow and allows decoupling of protocol buffer types from contributor code and user code. This allows more flexiblity * addressed comments * unit test fix
* Switching from using model Metadata -> TaskMetadata (#298) * Switching from using model Metadata -> TaskMetadata TaskMetadata will be maintained as a shadow and allows decoupling of protocol buffer types from contributor code and user code. This allows more flexiblity * addressed comments * unit test fix * Formatting fixed
TaskMetadata will be maintained as a shadow and allows decoupling of
protocol buffer types from contributor code and user code. This allows
more flexiblity
another prime motivation behind this change is that - it allows making the interface less verbose. This makes it trivial to support default values for metadata.