Page MenuHomePhabricator

[dagit] add launch run button
ClosedPublic

Authored by alangenfeld on Jan 6 2020, 9:32 PM.

Details

Summary

Test Plan

Configure a run launcher on the instance - load execute tab and launch a run.

Diff Detail

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

alangenfeld updated this revision to Diff 8373.Jan 6 2020, 9:32 PM
alangenfeld created this revision.

make graphql

alangenfeld edited the summary of this revision. (Show Details)Jan 6 2020, 9:40 PM
alangenfeld edited the test plan for this revision. (Show Details)
alangenfeld added reviewers: bengotow, max, prha.
alangenfeld updated this revision to Diff 8379.Jan 6 2020, 9:58 PM
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld edited the test plan for this revision. (Show Details)

rebase

prha added a comment.Jan 7 2020, 12:03 AM

this just hangs for me right now...

js_modules/dagit/src/execute/ExecutionStartButton.tsx
97

maybe add props to change the starting state text (e.g. from Starting to Launching)?

This looks good to me, just added some comments and suggestions inline. Nothing super big!

I'm a little concerned Start and Launch are interchangeable and don't convey what's different about the operations here - I think it makes sense in the context of a RunLauncher, but I'd sort of prefer startPipelineExecution and startPipelineExecutionUsingLauncher or something like that?

js_modules/dagit/src/execute/PipelineExecutionButtonGroup.tsx
25

I'm a little nervous about having the getVariables() method return an untyped object - would be kinda nice if we could detect that the function was out-of-date / broken if the variable schema changes. Maybe use StartPipelineExecutionVariables | LaunchPipelineExecutionVariables here?

26

May not need to be optional since the UI shows a spinner in place of this button until the pipeline is ready.

js_modules/dagit/src/execute/PipelineExecutionContainer.tsx
208โ€“209

It looks like this may be unused now?

If it's not unused, we may still want to move ownership of this mutation down into the PipelineExecutionButtonGroup, it's odd to have execution kicked off in two separate code paths.

You could potentially expose a public method on a PipelineExecutionButtonGroup class component and use a ref to invoke it ala:

this.executionButtonGroupRef.current.start()

261

Silly nit but I think this can just be pipeline.name here since we validate pipeline is non null above.

this just hangs for me right now...

ya you cant launch on to your self - something weird with flask making a request to itself

alangenfeld planned changes to this revision.Jan 7 2020, 3:41 PM
alangenfeld marked 4 inline comments as done.Jan 7 2020, 5:29 PM

I'm a little concerned Start and Launch are interchangeable and don't convey what's different about the operations here

Ya this is definitely just a first pass to be able to test things - needs some UX thought and love from you ๐Ÿ˜‰

bengotow accepted this revision.Jan 7 2020, 8:55 PM

Ok sounds good to me then ๐Ÿ˜‰

This revision is now accepted and ready to land.Jan 7 2020, 8:55 PM
alangenfeld updated this revision to Diff 8429.Jan 7 2020, 9:36 PM

dont add redundant tooltips

alangenfeld updated this revision to Diff 8432.Jan 7 2020, 9:56 PM

fix up apollo scripts

This revision was automatically updated to reflect the committed changes.