Page MenuHomePhabricator

Make re-execute a two-part button so it’s one-click by default #2687
ClosedPublic

Authored by bengotow on Jul 14 2020, 3:42 PM.

Details

Summary

Ok this one has been kicking around for a while but I think we're in good shape now.

This diff changes the re-execute button into a two-part button so a single click handles the common case. If you have a selected step in the gaant view and that selection has finished running (and you are persisting artifacts), the default is to re-execute the selection only:

If you have nothing selected, the default is to re-execute the entire pipeline and it looks like this, with the (*) syntax reinforcing that you are re-running everything in the current view:

If you click the disclosure triangle, you can see the options listed out and conditionally enabled. There is a new option "From Selection", which runs everything downstream of the step(s) you have selected by creating a step*-style query, evaluating it, and sending the step keys.

When I was revisiting this diff, I talked with Yuhan and restructured the JS side of re-execution just a bit with a few goals in mind:

  • In the future, we're going to be shifting the inflation of step keys / evaluation of the step query to the python side.
  • The JS code presenting the options and then invoking re-execution mutations is tricky because some re-execution options cause others to be ignored or have dependencies (for example, resumeRetry causes stepKeys to be ignored, passing a stepQuery is required but is actually just used for display while stepKeys is used to scope the run, env yaml is sometimes provided explicitly but sometimes already on the Run object). Because of this, the JS functions that assemble the mutation take many optional arguments, which made it hard to think through.

To address this, I defined a new ReExecutionStyle type which makes the relevant arguments required for each case. I also created a StepSelection type which wraps a set of step keys with the step query that generated them, because we almost always pass both together these days.

Test Plan

Run tests

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

Two primary issues here IMO:

  1. I think the "Launch Re-execution" text needs indicate which of the "three options" is selected. If we allow single button re-execution the user needs to understand what is being re-executed.
  2. When there is a subselection selected I think it should default to that. Alternatively it should *always* default to that and the "full pipeline" case is just the selector with "*"

Req'ing changes.

Talked about this live.

  • Biggest core concern here is the subtle distinction between using the step selection box to select "*" within an execution that itself was a step subset execution and the ambiguity around that. This probably not a just a UI issue, but a core conceptual issue in the system
This revision now requires changes to proceed.Jul 14 2020, 5:54 PM
bengotow edited the summary of this revision. (Show Details)

Rebase, CI is angry

Rebase, remove .py file merged in the wrong direction

slightly relevant issue here: https://github.com/dagster-io/dagster/issues/2858 this is a leftover from the run grouping effort


it's not clear whether these queries are step selection or solid selection. shall we make it something like *[*], ingest_cost*[*]? not sure whats the best (least confusing) way.

this looks good to me.

would be good to address the confusion around solid vs step selection in a more intuitive way in the next iteration

One suggestion: Instead of "Launch Re-execution" how about "Re-execute" and special case "Re-execute(*)" as "Re-execute all" or something.

Otherwise good improvement!

This revision is now accepted and ready to land.Wed, Sep 30, 11:24 PM

Thanks for the feedback folks! I definitely agree Yuhan re: future improvement πŸ‘

  • Pre-rebase changes
  • Merge branch 'master' into dbg/one-click-2
  • Resolve merge conflicts