Page MenuHomePhabricator

4/ Shed RunConfig
ClosedPublic

Authored by max on Thu, May 7, 12:44 AM.

Details

Summary

We are molting (searching for alternatives to knife emoji, "kill", "rip out", etc.). This removes the ability to set run_id or step_keys_to_execute when invoking execute_pipeline. Note that the GraphQL layer never used this codepath. Adds a convenience method, execute_run, to make the actual codepath clearer.

Resolves https://github.com/dagster-io/dagster/issues/2392

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

max planned changes to this revision.Thu, May 7, 12:44 AM
max created this revision.
max requested review of this revision.Thu, May 7, 2:03 AM
max planned changes to this revision.Thu, May 7, 5:32 AM
max updated this revision to Diff 13376.Thu, May 7, 5:33 AM

restore

do we want to wait for 0.8.0 to do this actual removal and pull the new reexexcution apis in to a separate diff?

Ya let’s def wait for cut to land this

Sent via Superhuman iOS ( https://sprh.mn/?vip=schrockn@elementl.com )

ah you meant 0.8.0.

honestly I think we can do it in the next cut. I would surprised if users do that a lot.

Sidenote: This would be good stuff to collect telemetry on so we have visibility into how much we are going to thrash users.

New re-execution APIs in their own diff would be nice

schrockn requested changes to this revision.Thu, May 7, 5:15 PM

up for chopping this up?

This revision now requires changes to proceed.Thu, May 7, 5:15 PM
max updated this revision to Diff 13398.Thu, May 7, 5:23 PM

rebase

max planned changes to this revision.Thu, May 7, 5:36 PM
alangenfeld added inline comments.Thu, May 7, 5:54 PM
python_modules/dagster/dagster/core/execution/api.py
129–139

if we're cool switching in to "master is 0.8.0" mode we can have execute_pipeline_def for executing defs directly and make *execute_pipeline* only take ExecutablePipeline

alangenfeld added inline comments.Thu, May 7, 6:14 PM
python_modules/dagster/dagster/core/execution/api.py
129–139

we could also add execute_pipeline_def and just pull the back compat union out in 0.8.0

max updated this revision to Diff 13440.Thu, May 7, 8:48 PM

rebase

max planned changes to this revision.Thu, May 7, 8:48 PM
max updated this revision to Diff 13441.Thu, May 7, 8:58 PM

defer rexecution apis

max edited the summary of this revision. (Show Details)Fri, May 8, 8:30 PM

lets add a changes.md entry too

python_modules/dagster/dagster/core/execution/api.py
26–124

nit: would it be better to push these private methods to the bottom and open the file with the brief guide

86–87

why cant this just be event_list = list(_execute_run_iterator_with_plan(...)) ?

130–138

I dont understand what these "supports reexecution" & its column values mean

python_modules/libraries/dagstermill/dagstermill_tests/test_event_callback.py
32–39

feels bad that we have to import this _ method - should this be execute_run since we dont need the result?

alangenfeld added inline comments.Fri, May 8, 8:32 PM
python_modules/dagster/dagster/core/execution/api.py
75–86

maybe we should have _execute_run_plan_with_plan_iterator and this should be _execute_run_with_plan_sync

it took me so long to piece together this refactor - im not sure how much more verbose / clearer names would help but its worth a shot

75–86

_execute_run_with_plan_iterator *

schrockn requested changes to this revision.Fri, May 8, 8:39 PM

q mgmt for alex's feedback

This revision now requires changes to proceed.Fri, May 8, 8:39 PM
max added inline comments.Fri, May 8, 8:50 PM
python_modules/dagster/dagster/core/execution/api.py
26–124

sure

75–86

yep, this stuff is a mess. the only thing this does that execute_run_iterator doesn't is pull out the pipeline_context so bits of it can be pasted into the PipelineExecutionResult's scoped_pipeline_context. ultimately this should go away once we get file manager and intermediates manager properly scoped. in the interim, we could fix it by refactoring execute_run_iterator to be an object -- i may just put a diff up for that

86–87

see above

python_modules/libraries/dagstermill/dagstermill_tests/test_event_callback.py
32–39

yep

max updated this revision to Diff 13558.Sat, May 9, 1:26 AM

Now with more dependencies

max planned changes to this revision.Sat, May 9, 1:33 AM
max updated this revision to Diff 13634.Mon, May 11, 6:19 PM

Rebase

max retitled this revision from Shed RunConfig to 4/ Shed RunConfig.Mon, May 11, 6:23 PM
max added a parent revision: D2869: 3/ Reorganize for clarity.
max removed a parent revision: D2848: 2/ Add execute_run API.
schrockn resigned from this revision.Mon, May 11, 9:47 PM

verygood

this change makes me very happy.

will leave to @alangenfeld to accept to ensure compatibility with new executable pipeline stuff

python_modules/dagster/dagster/core/execution/api.py
488–489

most excelletn

This revision is now accepted and ready to land.Mon, May 11, 9:50 PM
max updated this revision to Diff 13664.Mon, May 11, 10:13 PM

Rebase

max updated this revision to Diff 13686.Mon, May 11, 11:14 PM

Rebase

This revision was automatically updated to reflect the committed changes.