Steal alot of @catherinewu's work from D4936 to support re-execution of dynamic output pipelines. Python API and GraphQL.
Details
Details
Diff Detail
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
Comment Actions
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:
- Python API reexecute_pipeline
- retry in graphql (the retry from failure button in dagit) [1]
- 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 | |
Comment Actions
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 | |
Comment Actions
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 | |
