All python function tasks can now be dynamic #320
Conversation
katrogan
requested review from
kumare3,
matthewphsmith and
wild-endeavor
as code owners
|
@kumare3 @wild-endeavor mind reviewing? I'd like to leave dynamic tasks as a separate mixin because of the way they overwrite the execute method |
|
ping @kumare3 |
|
@katrogan after thinking a little more, I feel we can covert the dynamic logic to be a boolean switch in the base task. this way, truly all tasks can by dynamic without extension. Thus if you use the |
|
@kumare3 I like it! i will update this PR |
|
PTAL @kumare3 @wild-endeavor not really sure if it's worth keeping the dynamic code as a mixing separate from PythonFunctionTask but the latter is quite clean/simple as is tested on my local flyte sandbox with dynamic sidecar and vanilla dynamic tasks |
katrogan
changed the title
| ) | ||
| ) as ctx: | ||
| with ctx.new_execution_context(mode=ExecutionState.Mode.TASK_EXECUTION) as ctx: | ||
| dynamic_job_spec = dynamic_pod_task.compile_into_workflow(ctx, dynamic_pod_task._task_function, a=5) |
There was a problem hiding this comment.
is there anything else here worth checking for?
There was a problem hiding this comment.
wrt the dynamic mixin? i used the existing dynamic unit test as a model which only verifies this
|
we can always decide in the future about the mixin status. |
| return self._task_function(**kwargs) | ||
| if self.execution_mode == self.ExecutionMode.DEFAULT: | ||
| return self._task_function(**kwargs) | ||
| elif self.execution_mode == self.ExecutionMode.DYNAMIC: |
There was a problem hiding this comment.
is this a new execution mode?
There was a problem hiding this comment.
ohh yes. should i call it something else?
flytekit/annotated/dynamic_mixin.py
Outdated
| from flytekit.models import literals as _literal_models | ||
|
|
||
|
|
||
| class DynamicWorkflowTaskMixin(object): |
There was a problem hiding this comment.
why keep this separate? Whats the use of a mixin? We will never use it elsewhere right?
There was a problem hiding this comment.
if you want you can just make these has helper methods?
There was a problem hiding this comment.
nope, see my comment above I wasn't sure whether it was cleaner to leave PythonFunctionTask as is given it's so simple. I'll fold this in.
| @@ -167,9 +174,17 @@ def __init__( | |||
| **kwargs, | |||
| ) | |||
| self._task_function = task_function | |||
| self._execution_mode = execution_mode if execution_mode is not None else self.ExecutionMode.DEFAULT | |||
There was a problem hiding this comment.
this has been bugging me to. Can we just start with an ExecutionMode always set to something like DEFAULT?
|
ping @kumare3 |
| self._wf.compile(**kwargs) | ||
|
|
||
| wf = self._wf | ||
| sdk_workflow = wf.get_registerable_entity() |
There was a problem hiding this comment.
this might impact what @wild-endeavor is doing
There was a problem hiding this comment.
I will wait and rebase after his change is in
There was a problem hiding this comment.
talked offline, i'll merge this as is
| if self.execution_mode == self.ExecutionBehavior.DEFAULT: | ||
| return self._task_function(**kwargs) | ||
| elif self.execution_mode == self.ExecutionBehavior.DYNAMIC: | ||
| return self.dynamic_execute(self._task_function, **kwargs) |
There was a problem hiding this comment.
ohh this is interesting. This will break for cases in which execute is overriden right? Which happens quite a few times? Are we sure this is the best place to do it?
There was a problem hiding this comment.
so the contract is that execute is overridable. but dispatch_execute should not be overriden
There was a problem hiding this comment.
I guess now that I have added pre-execute and post-execute, we can document that execute should not be overriden in most cases unless absolutely required and in that case, unless you replicate the dynamic execute call, ability to dynamically execute will be lost! I am ok with that
There was a problem hiding this comment.
so the contract is that execute is overridable. but dispatch_execute should not be overriden
yes precisely. added documentation that execute can be overwritten if dynamic execute is handled too
No description provided.