Page MenuHomeElementl

execution context naming revamp, remove resources and intermediate storage from run worker
ClosedPublic

Authored by cdecarolis on Apr 9 2021, 6:22 PM.

Details

Summary

This diff attempts a complete system-execution-context naming and structure revamp, such that we distinguish between "orchestration" (coordination of processes to perform actual execution of steps) and "execution" (actual execution of steps). As a side-effect, resources and intermediate storage are no longer initialized in the run process.

Test Plan

A ton of unit tests changes, integration

Diff Detail

Repository
R1 dagster
Branch
resources_in_run_worker
Lint
Lint Errors
SeverityLocationCodeMessage
Errorpython_modules/libraries/dagstermill/dagstermill/solids.py:222E0602Undefined Variable
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
python_modules/dagster/dagster/core/execution/context/system.py
450–458

why this inheritance?

python_modules/dagster/dagster/core/execution/plan/external_step.py
46–99

can these actually be orchestration contexts?

definitely loving this direction - I think we can trim even further as highlighted with some inline comments

python_modules/dagster/dagster/core/execution/api.py
767

this is surprising to me - what case caused this to get added?

python_modules/dagster/dagster/core/execution/context/__init__.py
0

feel like we should just delete this and move to having comments on the classes - this file just isnt natural to open to find this imo

python_modules/dagster/dagster/core/execution/context/step.py
0

I don't think this class needs to exist - we should just push the impl down to SolidExecutionContext

python_modules/dagster/dagster/core/execution/context/system.py
203

this inheritance feels a little weird since we cant do the same on the Step context classes - just makes the inheritance tree a little odd

python_modules/dagster/dagster/core/execution/api.py
767

This happens if there is a failure during resource initialization when booting up the PlanExecutionContext. Previously was a pipeline init failure, changed it to a pipeline failure.

Get rid of UserStepExecutionContext, ComputeStepExecutionContext, simplify hook context so that everything exposed at the top level is user facing

Get rid of hook_context.step usage, fix plan context inheritance

python_modules/dagster/dagster/core/events/__init__.py
148–1022
  • minimize API changes unless they are necesarry
  • mypy stuff (can remove check)
python_modules/dagster/dagster/core/execution/context/__init__.py
0

^

python_modules/dagster/dagster/core/execution/context/system.py
108–118
  • switch to typing.NamedTuple
  • comments / docblocks
python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
321

get this stuff back in the host mode file

python_modules/dagster/dagster_tests/core_tests/hook_tests/test_hook_run.py
130 ↗(On Diff #35271)

is the solid name available instead?

  • add comments / docblocks to the context classes explaining the thought in the current structure
  • make sure to turn on integration tests too
python_modules/dagster/dagster/core/execution/api.py
636–638

safest to add both new params at the end

This revision now requires changes to proceed.Apr 12 2021, 9:54 PM
python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
321

I don't think I can do that because I enter alternate entrypoints on PlanOrchestrationManager based on whether I am in host mode

python_modules/dagster/dagster/core/execution/context_creation_pipeline.py
321

well even if you do that - you can import the function to use here even if it lives in that file

That said - unless I am missing something - doing the host_mode bool thing doesn't seem substantially better than just having a different context manager like the current approach [1]

python_modules/dagster/dagster/core/execution/host_mode.py
223–248

[1] maybe rename to Host Orchestration

cdecarolis marked an inline comment as done.

Update error handling for resource init failure, rearrange tests to compensate, fix logging for steps

Get rid of context hierarchy tree, get rid of changes to test_hook_run.py

cdecarolis marked an inline comment as done.

Fixed tests, added documentation

python_modules/dagster/dagster/core/execution/api.py
637–662

instead of adding these args to this public API just for use in the in process executor - lets just dupe this small block of code in the in process executor

python_modules/dagster/dagster/core/execution/context/system.py
42

doc block [2]

108–127

mention presence / absence of access to user code

140

[2]

203

[2]

261

[2]

Fix error tests for resource init

Added comments to context classes, in process executor doesn't use execute_plan_iterator

Fix bug with create_execution_data

Add executor def test, use initialized loggers for pipeline init failure

Super excited for this change. Big step forward. Thanks @cdecarolis for working through this. Will let others approve.

This revision is now accepted and ready to land.Apr 15 2021, 5:53 PM

land late tonight / early tomorrow for max soak between now and next release