Continuing the work of moving logic that requires loading definitions in the run worker to execution plan generation.
BK (verify that leaving this out breaks tests)
launch a multiprocess run in dagit without setting an intermediate storage - now pops up an error right away rather than waiting until the run worker starts
OK, this balooned a bit, as these changes are wont to do. We need to be able to load the Executor itself within the plan generation, which requires passing the instance down into ExecutionPlan.build and create_execution_plan, which kind of spreads throughout the system in lots of places.
probably 80% of this diff is passing DagsterInstances around so that we can access it during plan creation to validate the executor, I could split that part out into a separate diff if that would make it easier to review
oof - all this threading of instance to construct and discard an Executor.
What about a property on ExecutorDefinition? One of the few custom executors I know have been built is in process so likely wouldn't want to enforce these constraints
ExecutorProcessSetting giving you pause?
Yea I was giving my self some time to think on it since theoretically this is public API surface area that we cant change easily.
- The framing of "process setting" doesn't feel super intuitive to me
- Theres a fundamental question of whether we go this direction of trying to capture some broad property of the executor and enforce requirements based on that, or whether the executor should directly communicate what requirements it should enforce, ie ExecutorRequirement.NO_IN_MEMORY_IO or something. If we get it right, the former allows us to build additional stuff around this captured understanding, but if we're wrong its a pain. In this specific case it's unclear to me if we should try to capture the distinction between multiple processes on one machine versus multiple processes on multiple machines.
What do you think of the approach of having a requirements: Optional[List[ExecutorRequirement]] ? Existing custom executors will gracefully enforce nothing and we can enforce what we want on our custom executors. If we continue to build on this the list of enum entries feels like it can grow somewhat gracefully.
this should have a doc block and an apidocs entry
these docs should be updated