Page MenuHomeElementl

[4/4] Remove ExecutionPlan.environment_config
ClosedPublic

Authored by dgibson on Jan 25 2021, 5:05 PM.

Details

Summary

To make it so that an ExecutionPlan can be created without being able to load a PipelineDefinition, replace the small number of methods on ExecutionPlan that require one with a method parameter rather than assuming it exists as state on the class.

Test Plan

Integration

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 25 2021, 5:24 PM
Harbormaster failed remote builds in B24809: Diff 30209!
Harbormaster returned this revision to the author for changes because remote builds failed.Jan 25 2021, 6:08 PM
Harbormaster failed remote builds in B24814: Diff 30216!
dgibson retitled this revision from Remove ExecutionPlan.environment_config to [4/4] Remove ExecutionPlan.environment_config.Jan 25 2021, 6:39 PM
This revision is now accepted and ready to land.Jan 26 2021, 10:12 PM

putting on ice for now

This revision is now accepted and ready to land.Mar 22 2021, 7:32 PM

this ended up changing a bit since the first round (in particular, build_subset_plan didn't change before), so putting back out just in case

minor stuff but requesting since i think its worth being careful with these changes

python_modules/dagster/dagster/core/execution/plan/plan.py
510

why do we need pipeline name?

703–721

is this change needed? it looks like you wrapped at all the call sites.

python_modules/dagster/dagster/core/instance/__init__.py
677–718

ugh

This revision now requires changes to proceed.Mar 24 2021, 6:30 PM
python_modules/dagster/dagster/core/execution/plan/plan.py
703–721

i did not wrap, but I will now :)

python_modules/dagster/dagster/core/instance/__init__.py
677–718

hard agree

remove pipeline_name, wrap callsites

There a lot of little weird complexity from dealing with environment_config in this diff - the real target is to drop pipeline from ExecutionPlan right? I am wondering if going all the way is a better approach since this is such an awkward half step

I was not planning to drop pipeline from ExecutionPlan (using a ReconstructablePipeline/CodePointer is fine and does not involve loading user code - it's calling get_definition() on it that is not possible in the host mode run worker).

EnvironmentConfig, on the other hand, has a hard dependency on user code / the PipelineDefinition in order to even be built - that's the reason it needs to go

another partial review interrupted by meeting- leaving comments will look at again. Something about this diff just makes me feel bad having hard time articulating

python_modules/dagster/dagster/core/execution/plan/plan.py
66–67

all this really comes down to just this huh

python_modules/dagster/dagster/core/execution/resolve_versions.py
147–158

there is something a bit odd here since run_config and environment_config represent the same information in slightly different ways - how much does just passing mode in instead of environment_config clean things up?

python_modules/dagster/dagster/core/instance/__init__.py
677–718

maybe just always construct the env config and thatd be cleaner ?

things to consider inline but theres not enough here to block on

This revision is now accepted and ready to land.Mar 25 2021, 11:09 PM

re: removal of pipeline from ExecutionPlan - that is a substantial change of its own that happens in https://dagster.phacility.com/D7155

This revision was landed with ongoing or failed builds.Mar 29 2021, 8:40 PM
This revision was automatically updated to reflect the committed changes.