Page MenuHomePhabricator

Remove backwards-compatible run_config from execution context (#1874)

Authored by nate on Feb 9 2020, 7:48 PM.



Started this for - seems like one thing to resolve is whether create_execution_plan should take a RunConfig or a PipelineRun as an argument, which I guess depends on how much we consider this a user-facing API?

There's a bit of both in this diff, would love to discuss and we can pick one and I'll follow-up here.

Test Plan


Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

nate created this revision.Feb 9 2020, 7:48 PM
nate edited the summary of this revision. (Show Details)Feb 9 2020, 7:52 PM
nate added a reviewer: prha.
prha added a comment.Feb 10 2020, 5:01 PM

My vote is to keep create_execution_plan taking in RunConfig.

I don't think create_execution_plan needs to be promoted as a public API, but that is not a point of significance for me. To me, the boundary between RunConfig and PipelineRun is whether we are actually in an execution context or not (e.g. creating the execution context).

It seems as if you should be able to create a plan without actually creating a run (which we do, in places, for pre-run validation).

nate planned changes to this revision.Feb 10 2020, 5:01 PM

Cool, that sounds right to me! Will update.

prha accepted this revision.Feb 10 2020, 9:33 PM
This revision is now accepted and ready to land.Feb 10 2020, 9:33 PM