Retrieve launch plan interfaces from admin #103

wild-endeavor · 2020-04-03T22:44:10Z

TL;DR

When Propeller comes across a launch plan node in a DynamicJobSpec it will hit Admin to retrieve the interface for the LP to do the interface check.

Type

Bug Fix
Feature
Plugin

Are all requirements met?

Complete description

Please see the issue linked below and also the SDK PR flyteorg/flytekit#92 for more information. A sample dynamic job spec object has been uploaded here as well. Please see the text file for the type of dynamic job spec this PR is meant to support.
dynamic_job_spec.txt

Added a GetLaunchPlan function to a launchplan/Reader interface which sits alongside the launchplan/Executor interface. Admin client wrapper now satisfies both interfaces.
Added a call to that function in the dynamic job handler buildContextualDynamicWorkflow function.

Tracking Issue

flyteorg/flyte#139

Follow-up issue

flyteorg/flyte#246
This PR will be deprecated upon completion of this issue.

kumare3 · 2020-04-03T22:50:02Z

What is this for? Dynamic LP?

wild-endeavor · 2020-04-03T22:50:15Z

Yes.

pkg/controller/nodes/dynamic/handler.go

EngHabu · 2020-04-03T23:42:28Z

pkg/compiler/admin.go

+	"github.com/lyft/flyteidl/gen/pb-go/flyteidl/core"
+)
+
+// Should we call this something else? It can be used more generally if ever necessary.


Historically: I wanted to make both tasks and launchplan parameters in the compiler to take InterfaceProviders... but then launch plans diverged (with expected* stuff) maybe there is still hope?

I can't think of a good name... InterfaceProviderProvider was the best I could come up with, so I'm leaving it as is for now.

codecov-io · 2020-04-04T23:40:55Z

Codecov Report

Merging #103 into master will increase coverage by 0.11%.
The diff coverage is 71.73%.

@@            Coverage Diff             @@
##           master     #103      +/-   ##
==========================================
+ Coverage   50.01%   50.13%   +0.11%     
==========================================
  Files         128      129       +1     
  Lines        7966     8007      +41     
==========================================
+ Hits         3984     4014      +30     
- Misses       3594     3601       +7     
- Partials      388      392       +4

Impacted Files	Coverage Δ
...kg/controller/nodes/subworkflow/launchplan/noop.go	`73.33% <0.00%> (-11.29%)`	⬇️
pkg/controller/nodes/dynamic/handler.go	`64.09% <61.11%> (+0.30%)`	⬆️
...g/controller/nodes/subworkflow/launchplan/admin.go	`75.23% <66.66%> (-1.11%)`	⬇️
pkg/compiler/admin.go	`100.00% <100.00%> (ø)`
pkg/compiler/workflow_compiler.go	`80.00% <100.00%> (ø)`
pkg/controller/nodes/handler_factory.go	`47.82% <100.00%> (ø)`

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 fd3571d...7fee006. Read the comment docs.

pkg/controller/nodes/subworkflow/launchplan/launchplan.go

kumare3 · 2020-04-07T21:00:49Z

pkg/controller/nodes/subworkflow/launchplan/admin.go

@@ -97,6 +97,26 @@ func (a *adminLaunchPlanExecutor) GetStatus(ctx context.Context, executionID *co
 	return item.ExecutionClosure, item.SyncError
 }

+func (a *adminLaunchPlanExecutor) GetLaunchPlan(ctx context.Context, launchPlanRef *core.Identifier) (*admin.LaunchPlan, error) {


Having this method here is fine, but having a separate interface for this method is useful to differentiate the use cases and usage paths

kumare3 · 2020-04-08T21:10:56Z

pkg/compiler/admin.go

+	"github.com/lyft/flyteidl/gen/pb-go/flyteidl/core"
+)
+
+// This object is meant to satisfy github.com/lyft/flytepropeller/pkg/compiler/common.InterfaceProvider


This comment does not answer why this is needed here?

We need a struct to satisfy the interface because the function that we pass it to, compiler.CompileWorkflow takes the common.InterfaceProvider interface.

kumare3 · 2020-04-08T21:21:32Z

Thank you for the changes.

kumare3

LGTM

* Initial copy from propeller * Remove travis * Linking the makefile * add k8s integration * add description for bumping versions * add permissions * add permissions * load my awesome image * use flyteadmin:test * Pick a running admin pod * Setup a clean cluster for integration tests * use builder phase * Request more memory for postgres * Fix script

# TL;DR When Propeller comes across a launch plan node in a `DynamicJobSpec` it will hit Admin to retrieve the interface for the LP to do the interface check. ## Type - [ ] Bug Fix - [x] Feature - [ ] Plugin ## Are all requirements met? - [x] Code completed - [x] Smoke tested - [x] Unit tests added - [x] Code documentation added - [x] Any pending items have an associated Issue ## Complete description Please see the issue linked below and also the SDK PR flyteorg/flytekit#92 for more information. A sample dynamic job spec object has been uploaded here as well. Please see the text file for the type of dynamic job spec this PR is meant to support. [dynamic_job_spec.txt](https://github.com/lyft/flytepropeller/files/4430252/dynamic_job_spec.txt) * Added a `GetLaunchPlan` function to a `launchplan/Reader` interface which sits alongside the `launchplan/Executor` interface. Admin client wrapper now satisfies both interfaces. * Added a call to that function in the dynamic job handler `buildContextualDynamicWorkflow` function. ## Tracking Issue flyteorg/flyte#139 ## Follow-up issue flyteorg/flyte#246 This PR will be deprecated upon completion of this issue.

retrieve launch plan interfaces from admin

4fbf31a

EngHabu reviewed Apr 3, 2020

View reviewed changes

Yee Hing Tong added 8 commits April 3, 2020 16:59

make generate

f383e55

fix existing tests

17dfae3

fixing tests, unit tests

1dbea10

change comment and go mod tidy

835f04d

more unit test

fd389a6

one last unit test

b94f527

lint

23a13d9

more lint

7fee006

Yee Hing Tong added 2 commits April 7, 2020 08:28

merge master

cd39925

go mod tidy

3418d03

wild-endeavor mentioned this pull request Apr 7, 2020

[Feature] Move Propeller creation of CompiledWorkflowClosure to Admin flyteorg/flyte#246

Open

13 tasks

kumare3 reviewed Apr 7, 2020

View reviewed changes

pkg/controller/nodes/subworkflow/launchplan/launchplan.go Show resolved Hide resolved

kumare3 reviewed Apr 7, 2020

View reviewed changes

wild-endeavor and others added 3 commits April 7, 2020 18:09

[Ignore] PR into PR #103 (#106)

ee88e0f

return the interface since the struct is private and lint is complaining

e22b5aa

merge master and go mod tidy

0990994

kumare3 reviewed Apr 8, 2020

View reviewed changes

kumare3 approved these changes Apr 8, 2020

View reviewed changes

wild-endeavor merged commit 89406fb into master Apr 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Retrieve launch plan interfaces from admin #103

Retrieve launch plan interfaces from admin #103

wild-endeavor commented Apr 3, 2020 •

edited

Loading

kumare3 commented Apr 3, 2020

wild-endeavor commented Apr 3, 2020

EngHabu Apr 3, 2020

wild-endeavor Apr 4, 2020

codecov-io commented Apr 4, 2020

kumare3 Apr 7, 2020

kumare3 Apr 8, 2020

wild-endeavor Apr 8, 2020

kumare3 commented Apr 8, 2020

kumare3 left a comment

Retrieve launch plan interfaces from admin #103

Retrieve launch plan interfaces from admin #103

Conversation

wild-endeavor commented Apr 3, 2020 • edited Loading

TL;DR

Type

Are all requirements met?

Complete description

Tracking Issue

Follow-up issue

kumare3 commented Apr 3, 2020

wild-endeavor commented Apr 3, 2020

EngHabu Apr 3, 2020

Choose a reason for hiding this comment

wild-endeavor Apr 4, 2020

Choose a reason for hiding this comment

codecov-io commented Apr 4, 2020

Codecov Report

kumare3 Apr 7, 2020

Choose a reason for hiding this comment

kumare3 Apr 8, 2020

Choose a reason for hiding this comment

wild-endeavor Apr 8, 2020

Choose a reason for hiding this comment

kumare3 commented Apr 8, 2020

kumare3 left a comment

Choose a reason for hiding this comment

wild-endeavor commented Apr 3, 2020 •

edited

Loading