Page MenuHomePhabricator

[dagit] enable multi-step re-execution via step subset selector #2453
ClosedPublic

Authored by yuhan on Fri, May 15, 7:06 AM.

Details

Summary

https://github.com/dagster-io/dagster/issues/2453

a few changes

  • untie the step selection from log filter
    • code-wise, ReexecuteWithData now owns
      • selectedSteps which was previously generated from Run's state logsFilter
      • query which was previously owned by GaantChart. We lifted it up b/c click event on single step will also update query now.
    • behavior-wise, click on a step will 1) select a step, 2) filter the log of this step, and 3) update the selector query. Removing the filter won't deselect the step.
  • click a selected step will deselect it, remove it from log filter, and clear the selector query
  • the "type a step subset" input will now change the selectedSteps therefore enable multi-step re-execution
    • the step selector no longer hide unselected steps by default. we now have a checkbox to give users the option to show/hide unselected steps

single step selection via single-click

arbitrary step subset selection via selector syntax

Test Plan

click around dagit

  • playground
  • 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

yuhan created this revision.Fri, May 15, 7:06 AM
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, May 15, 7:21 AM
Harbormaster failed remote builds in B11486: Diff 14116!
yuhan edited the summary of this revision. (Show Details)Fri, May 15, 7:33 AM
yuhan updated this revision to Diff 14118.Fri, May 15, 7:38 AM
yuhan edited the summary of this revision. (Show Details)

up

yuhan requested review of this revision.Fri, May 15, 7:56 AM
max added a comment.Fri, May 15, 4:26 PM

I *really* like the second interaction; maybe we can talk about the first on this morning's call. It does feel a little hard to clear the subset selection.

schrockn requested changes to this revision.Fri, May 15, 7:42 PM

Req'ing changes based on call this am.

To summarize:

  1. Change the UI so there is only one notion of selection, driven by the execution selector at the bottom and kept in sync with actions like clicking on a single step
  2. Have a checkbox which optionally hides unselected steps. The default should be to show all steps.
This revision now requires changes to proceed.Fri, May 15, 7:42 PM
yuhan updated this revision to Diff 14143.EditedSat, May 16, 11:14 PM
  1. the selector syntax is linked to all types of step selection:
    • single click: query becomes stepKey
    • double click: query becomes *stepKey*
    • type in the selector: take any query
  2. Have a checkbox which optionally hides unselected steps. The default should be to show all steps.
yuhan edited the summary of this revision. (Show Details)Sat, May 16, 11:24 PM
yuhan updated this revision to Diff 14144.Sat, May 16, 11:29 PM
yuhan marked 2 inline comments as not done.
yuhan edited the summary of this revision. (Show Details)

up

yuhan updated this revision to Diff 14145.Sat, May 16, 11:33 PM

update snapshot

max added a comment.Mon, May 18, 11:01 PM

I think this is a huge step forwards but will defer to @schrockn for ultimate sign off

One last thing that may not feel cohesive is that the filter won't follow the changes in the DSL selector. issue: https://github.com/dagster-io/dagster/issues/2472

schrockn accepted this revision.Mon, May 18, 11:16 PM

this is a huge step forward for this use case. really excite for run groups

verygood

This revision is now accepted and ready to land.Mon, May 18, 11:16 PM