-
Notifications
You must be signed in to change notification settings - Fork 332
[Core feature] map_task to support ContainerTask #3249
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: machichima <[email protected]>
…ontainer -e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3249 +/- ##
==========================================
- Coverage 83.35% 76.36% -7.00%
==========================================
Files 347 215 -132
Lines 28791 22528 -6263
Branches 2960 2966 +6
==========================================
- Hits 23999 17203 -6796
- Misses 3956 4507 +551
+ Partials 836 818 -18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| monkeypatch.undo() | ||
|
|
||
|
|
||
| @pytest.mark.skipif( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modification in this file before this line is simply caused by formatter
-e Signed-off-by: machichima <[email protected]>
| # tasks that rely on user code defined in the container. This should be encapsulated by the auto container | ||
| # parent class | ||
| container._args = prefix_with_fast_execute(settings, container.args) | ||
| container._args = prefix_with_fast_execute(settings, container.args or []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args for ContainerTask is None, adding this to prevent error
flytekit/core/array_node_map_task.py
Outdated
| try: | ||
| o = self._run_task.execute(**single_instance_inputs) | ||
| # For Container task, it will return the LiteralMap. We need to convert it to native | ||
| # type here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@machichima can you add comment please that this is only for local execution for container tasks? this code doesn't run for backend cluster runs of container task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I make ContainerTask return Python type instead of LiteralMap. So this part is not needed anymore.
Thanks!
…ontainer -e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
2c2bd66 to
0f6dd70
Compare
-e Signed-off-by: machichima <[email protected]>
|
I think let ConatinerTask return python type break tests for plugins. Will have a look Update: I think it's not my problem. Merging the master branch and fixed |
…ontainer -e Signed-off-by: machichima <[email protected]>
…ontainer -e Signed-off-by: machichima <[email protected]>
a63386e to
f15415b
Compare
|
Bito Review Skipped - No Changes Detected |
1 similar comment
|
Bito Review Skipped - No Changes Detected |
flytekit/core/array_node_map_task.py
Outdated
| if isinstance(self._run_task, ContainerTask): | ||
| return self.python_function_task.get_container(settings) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
We can modify self.prepare_target, do nothing for container task
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! Thanks
…ontainer -e Signed-off-by: machichima <[email protected]>
-e Signed-off-by: machichima <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add an intergration test to make sure it works well in remote execution?
| if isinstance(self._run_task, ContainerTask): | ||
| yield | ||
| return | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just return directly?
update: we need to return a generator, please ignore my comment.
-e Signed-off-by: machichima <[email protected]>
Added! Thanks |
Tracking issue
flyteorg/flyte#6277
Why are the changes needed?
Currently, we cannot use
ContainerTaskin map task. This PR is trying to make this integration works.What changes were proposed in this pull request?
ContainerTaskwork with map task in local and remoteContainerTaskwith map task locallyHow was this patch tested?
unit test & example code below:
Execute following code locally and remotly should pass:
pyflyte run containertask_maptask.py wfpyflyte run --remote containertask_maptask.py wfSetup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link
Summary by Bito
This pull request enhances map task functionality by integrating ContainerTask support for both local and remote execution. It includes significant refactoring of the execute method to return native Python types, improving usability and maintainability. Comprehensive unit tests validate this integration and confirm expected behavior, ensuring expected functionality in both environments.