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.
minor stuff but requesting since i think its worth being careful with these changes
why do we need pipeline name?
is this change needed? it looks like you wrapped at all the call sites.
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
all this really comes down to just this huh
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?
maybe just always construct the env config and thatd be cleaner ?