Page MenuHomeElementl

Fix re-execution in Dagit
ClosedPublic

Authored by yuhan on Dec 18 2020, 4:16 AM.

Details

Summary

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

The bug was found in D5678. It was because Dagit disables the step selection button based on ExecutionPlan.artifactsPersisted https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/js_modules/dagit/src/runs/RunActionButtons.tsx#L139:7 which was derived from intermediate_storage_def.is_persistent

So this diff does:

  • clean up the memoization check a bit: it moves the persistence check into execution plan because it actually doesn't depend on pipeline_context, i.e. we can determine the check before executing the pipeline.
  • drop artifacts_persisted from ExecutionPlan's args: again, it doesn't need to be passed in, because execution plan already has enough info to determine it.
  • artifacts_persisted now is a property on ExecutionPlan which checks both "if intermediate storage is persistent" and "if the current run has corresponding non-in-memory object managers to support reexecution"
Test Plan

bk

arc patch D5678 and tested the re-execution flow in Dagit:

image.png (964×2 px, 183 KB)

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

yuhan retitled this revision from object-manager-reexecution 1/ execution_plan.can_reexecute to Fix re-execution in Dagit.Dec 18 2020, 4:45 AM
yuhan edited the summary of this revision. (Show Details)
yuhan edited the test plan for this revision. (Show Details)
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 18 2020, 4:46 AM
Harbormaster failed remote builds in B23189: Diff 28202!
yuhan edited the summary of this revision. (Show Details)

up

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 18 2020, 5:07 AM
Harbormaster failed remote builds in B23190: Diff 28203!
yuhan requested review of this revision.Dec 18 2020, 5:28 AM
This revision is now accepted and ready to land.Dec 22 2020, 12:48 PM
This revision was automatically updated to reflect the committed changes.