Page MenuHomePhabricator

Fix execution plan snapshot subsetting and dagit rendering bug
ClosedPublic

Authored by schrockn on Sat, May 9, 8:04 PM.

Details

Summary

Execution plans are strange beasts at the moment when it comes
to expressing sub plans. All the steps are there, and it is the
step_keys_to_execute property that tells the engine what steps to
actually execute.

The snapshot layer did not account for this and never persisted that
information.

Interestingly, this only happened because this bug pre-existed the
migration to snaphots. We were incorrectly rendering execution plans
in re-execution scenarios.

In this screenshot, we executed a single step but the other steps
are there but in a skipped state

Before --

With this change we only render the executed subplan

After --

This was tricky to fix because we encode step_keys_to_execute in
multiple locations. The invariant check in the create_run machinery
ensures (hopefully) that no one screws up until we fix it properly.

This may be hotfix worthy given that this information in persisted.
Then again, the bug this fixes is a fairly minor display bug. However
correctly persisting the snapshot ASAP would be pretty nice.

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

schrockn created this revision.Sat, May 9, 8:04 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Sat, May 9, 8:18 PM
Harbormaster failed remote builds in B11040: Diff 13566!

Before:

After:

schrockn updated this revision to Diff 13572.Sat, May 9, 9:07 PM
schrockn retitled this revision from Fix execution plan snapshots to Fix execution plan snapshot subsetting and dagit rendering bug.
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: alangenfeld, prha, max, yuhan.

up

Also will add test in dagster-graphql that exercises this one we decide if this is good product change or not (that is just the one filter in the graphql layer, the rest is good)

schrockn requested review of this revision.Sat, May 9, 9:21 PM
alangenfeld accepted this revision.Mon, May 11, 5:27 PM
This revision is now accepted and ready to land.Mon, May 11, 5:27 PM
yuhan added a comment.Mon, May 11, 6:53 PM

the "after" looks good -- it was a bit confusing to move skipped steps to the end before too.

once we persist the snapshot correctly, when it's a re-execution, maybe we should have a full picture of the pipeline in the same order as the original run but greyed out the skipped step.

@yuhan definitely. i suspect we will iterate on that alongside the run group viewer work.

max added inline comments.Mon, May 11, 7:09 PM
python_modules/dagster/dagster_tests/compat_tests/test_change_snapshot_structure.py
32

hm, this isn't a change in 0.7.10 anymore..?