Page MenuHomePhabricator

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

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

Details

Summary

Started this for https://github.com/dagster-io/dagster/issues/1874 - 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

unit

Diff Detail

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

Event Timeline

nate created this revision.Sun, Feb 9, 7:48 PM
nate edited the summary of this revision. (Show Details)Sun, Feb 9, 7:52 PM
nate added a reviewer: prha.
prha added a comment.Mon, Feb 10, 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.Mon, Feb 10, 5:01 PM

Cool, that sounds right to me! Will update.

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