Page MenuHomePhabricator

DynamicOutput re-execution
ClosedPublic

Authored by alangenfeld on Dec 15 2020, 11:02 PM.

Details

Summary

Steal alot of @catherinewu's work from D4936 to support re-execution of dynamic output pipelines. Python API and GraphQL.

Test Plan

added 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

Harbormaster returned this revision to the author for changes because remote builds failed.Dec 15 2020, 11:22 PM
Harbormaster failed remote builds in B22918: Diff 27879!
Harbormaster returned this revision to the author for changes because remote builds failed.Dec 16 2020, 12:15 AM
Harbormaster failed remote builds in B22924: Diff 27885!

Assuming this depends on D5542?

Unfortunately, the re-execution code paths are a bit messy.. There are a few ways to re-execute a pipeline:

  1. Python API reexecute_pipeline
  2. retry in graphql (the retry from failure button in dagit) [1]
  3. other re-execution options in dagit (i.e. "selection" and "from selected") [2]

In case you missed the case 3, in which the re-execution logic in dagit in JS land (where the code paths get messy :/) we should test if all the re-execution buttons still work - at least test the executionMetadata.stepKeys in graphql tests

python_modules/dagster-graphql/dagster_graphql/implementation/resume_retry.py
129

[1] covered in get_retry_steps_from_execution_plan ✅

142

[2] doubt if this would work for dynamic pipelines

This comes from https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/js_modules/dagit/src/runs/RunUtils.tsx#L149-170 where the step key transformation happens in toGraphQueryItems .

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_dynamic_pipeline.py
65–69

related to [2] maybe we can add a retry_two to test executionMetadata.stepKeys

Very concerned about using the step key DSL in _update_tracking_dict and friends. I think should be persisting the structured step key information DagsterEvent to reduce the surface area of our reliance on the DSL

python_modules/dagster-graphql/dagster_graphql/implementation/resume_retry.py
16–24

oof really don't like from_key calls in such a critical path. It looks like this also encodes the pseudo step key DSL to be persisted on DagsterEvent for all of time

python_modules/dagster/dagster/core/selector/subset_selector.py
103

This is pretty terrifying. I think we need to document this and test it more carefully

This revision now requires changes to proceed.Dec 21 2020, 4:05 PM

rebase, adding step_handle to DagsterEvent shown in D5755

Very happy with the tracking changes

Will let @yuhan do finally approval to sync with ObjectStoreOperation stuff

python_modules/dagster-graphql/dagster_graphql_tests/graphql/setup.py
826

change to map

python_modules/dagster/dagster/core/selector/subset_selector.py
103

complicated enough where extracting it into its own function and having tests to document would be useful

overall looks good to me. but the .map seems be the cause failing the tests

This revision is now accepted and ready to land.Sat, Jan 9, 12:54 AM
This revision was automatically updated to reflect the committed changes.