Page MenuHomePhabricator

enable reexecution when all required asset stores are non mem
ClosedPublic

Authored by yuhan on Nov 24 2020, 2:22 AM.

Details

Summary

Check if all the border steps of the current run have non-in-memory asset stores for reexecution.

Test Plan

bk

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

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 24 2020, 2:43 AM
Harbormaster failed remote builds in B21640: Diff 26265!
Harbormaster returned this revision to the author for changes because remote builds failed.Nov 24 2020, 9:15 PM
Harbormaster failed remote builds in B21698: Diff 26339!
yuhan requested review of this revision.Nov 24 2020, 9:45 PM
python_modules/dagster/dagster/core/execution/memoization.py
57

We already do this check in the outer method. If we want to check here, maybe better as an invariant?

60

I'm probably missing something, because I see this is used in copy_required_intermediates_for_execution as well, but do we need to fetch events from the instance? Can we not compare step_keys_to_execute with the set of steps in the full plan or something?

72

if we phrased this as

if not asset_store or asset_store == mem_asset_store:
    return False

we could avoid the continue

75

Are we able to raise an informative error here? I think that "Cannot perform reexecution with non persistent intermediates manager" will likely confuse users in the asset store world.

yuhan added inline comments.
python_modules/dagster/dagster/core/execution/memoization.py
60

switching to a simpler check

75

it's a bit tricky to raise the right error given we need to fallback to check intermediate storage

yuhan retitled this revision from enable reexecution when 'asset_store' is set to enable reexecution when all required asset stores are non mem.Nov 25 2020, 10:14 PM
yuhan edited the summary of this revision. (Show Details)

Looks good! It feels like there are some tricky edge cases here, e.g. if only some inputs of a solid come from a prior run, so maybe worth thinking through if there are tests to write that would give us confidence on them.

This revision is now accepted and ready to land.Nov 26 2020, 12:25 AM