-
Notifications
You must be signed in to change notification settings - Fork 333
[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
Open
machichima
wants to merge
16
commits into
flyteorg:master
Choose a base branch
from
machichima:6277-map-container
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 13 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
281bd66
feat: update to support container task
machichima 1d16454
Merge branch 'flyteorg:master' into 6277-map-container
machichima 137ab21
Merge branch 'master' of github.com:flyteorg/flytekit into 6277-map-c…
machichima 70b8bef
feat: enable using container task in map task
machichima 0d34130
feat: enable running map+container task in local
machichima e96b480
test: for running map+container task locally
machichima c53a64e
style: lint
machichima 5c95774
fix: using Union intead of "|"
machichima 6aae613
Merge branch 'master' of github.com:flyteorg/flytekit into 6277-map-c…
machichima 0f6dd70
docs: make comment more clear
machichima 4827e55
feat: make container task return python type
machichima 006493a
Merge branch 'master' of github.com:flyteorg/flytekit into 6277-map-c…
machichima f15415b
Merge branch 'master' of github.com:flyteorg/flytekit into 6277-map-c…
machichima 0126bf3
Merge branch 'master' of github.com:flyteorg/flytekit into 6277-map-c…
machichima d8b4362
refactor: prepare_target do nothing for ContainerTask
machichima 2b9387d
test: add container task map exec integration test
machichima File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -166,11 +166,11 @@ def get_serializable_task( | |
if settings.should_fast_serialize(): | ||
# This handles container tasks. | ||
if container and isinstance(entity, (PythonAutoContainerTask, MapPythonTask, ArrayNodeMapTask)): | ||
# For fast registration, we'll need to muck with the command, but on | ||
# ly for certain kinds of tasks. Specifically, | ||
# 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) | ||
# For fast registration, we'll need to muck with the command, but | ||
# only for certain kinds of tasks. Specifically, 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 or []) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
# If the pod spec is not None, we have to get it again, because the one we retrieved above will be incorrect. | ||
# The reason we have to call get_k8s_pod again, instead of just modifying the command in this file, is because | ||
|
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 taskThere 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