Skip to content
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

Implement launch single task execution #115

Merged
merged 18 commits into from
Jun 3, 2020
Merged

Implement launch single task execution #115

merged 18 commits into from
Jun 3, 2020

Conversation

katrogan
Copy link
Contributor

@katrogan katrogan commented May 26, 2020

TL;DR

This allows users to execute a previously registered task individually. Next up is register_and_execute for notebook and other usage.

Type

  • Bug Fix
  • Feature
  • Plugin

Are all requirements met?

  • Code completed
  • Smoke tested
  • Unit tests added
  • Code documentation added
  • Any pending items have an associated Issue

Complete description

This change does some refactoring to clarify the distinction between launch and execute such that launch now refers to an entity which can be used to trigger an execution, meanwhile execute still refers to something that can be individually executed.

Tracking Issue

flyteorg/flyte#256

Follow-up issue

flyteorg/flyte#221

@codecov-commenter
Copy link

codecov-commenter commented May 27, 2020

Codecov Report

Merging #115 into master will decrease coverage by 0.23%.
The diff coverage is 65.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #115      +/-   ##
==========================================
- Coverage   81.83%   81.60%   -0.24%     
==========================================
  Files         211      211              
  Lines       13661    13745      +84     
  Branches     1126     1133       +7     
==========================================
+ Hits        11179    11216      +37     
- Misses       2215     2262      +47     
  Partials      267      267              
Impacted Files Coverage Δ
flytekit/clis/sdk_in_container/launch_plan.py 70.83% <ø> (-0.31%) ⬇️
tests/flytekit/unit/models/test_launch_plan.py 100.00% <ø> (ø)
flytekit/engines/flyte/engine.py 62.50% <28.00%> (-4.86%) ⬇️
flytekit/clis/flyte_cli/main.py 37.91% <39.13%> (+0.08%) ⬆️
flytekit/engines/unit/engine.py 80.45% <50.00%> (-0.47%) ⬇️
flytekit/common/launch_plan.py 75.63% <66.66%> (-0.03%) ⬇️
flytekit/common/tasks/task.py 76.04% <66.66%> (-1.49%) ⬇️
flytekit/common/mixins/launchable.py 72.22% <80.00%> (ø)
flytekit/engines/common.py 71.90% <80.00%> (-0.14%) ⬇️
flytekit/models/common.py 94.15% <85.71%> (-0.85%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79b1117...7ba33f0. Read the comment docs.

EngHabu
EngHabu previously approved these changes May 28, 2020
@@ -100,7 +103,8 @@ def _return_mapping_object(self, sdk_node, sdk_type, name):
return _promise.NodeOutput(sdk_node, sdk_type, name)


class SdkNode(_six.with_metaclass(_sdk_bases.ExtendedSdkType, _hash_mixin.HashOnReferenceMixin, _workflow_model.Node)):
class SdkNode(_six.with_metaclass(_sdk_bases.ExtendedSdkType, _hash_mixin.HashOnReferenceMixin, _workflow_model.Node,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we making the node executable not the task?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated

@wild-endeavor
Copy link
Contributor

Can you give some insight into what get_node is used for? That's the bit that's confusing to me.

@katrogan
Copy link
Contributor Author

Can you give some insight into what get_node is used for?

that was stale, thanks for the catch. should be cleaned up

@@ -252,3 +255,28 @@ def __repr__(self):
task_type=self.type,
interface=self.interface
)

@_exception_scopes.system_entry_point
def launch(self, **input_map):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway this can use the same executable mixin thingy?

Copy link
Contributor Author

@katrogan katrogan Jun 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the SdkRunnableTask which inherits form this class defines an execute method with a different signature than what this launch method uses so i was worried that would be confusing/messy

@katrogan
Copy link
Contributor Author

katrogan commented Jun 1, 2020

friendly ping @wild-endeavor @EngHabu

@katrogan
Copy link
Contributor Author

katrogan commented Jun 2, 2020

@EngHabu mind taking another look? I refactored to use Launch in place of Execute like we discussed (and deprecated existing calls to launch_foo) however I wasn't sure if it made sense to change the flyte-cli command too along the same lines?

EngHabu
EngHabu previously approved these changes Jun 3, 2020
flytekit/common/mixins/launchable.py Outdated Show resolved Hide resolved
flytekit/common/mixins/launchable.py Outdated Show resolved Hide resolved
EngHabu
EngHabu previously approved these changes Jun 3, 2020
Copy link
Collaborator

@EngHabu EngHabu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
CC @wild-endeavor LGTU?

@katrogan katrogan merged commit 8155dff into master Jun 3, 2020
max-hoffman pushed a commit to dolthub/flytekit that referenced this pull request May 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants