Page MenuHomePhabricator

7/ Subset for execution
ClosedPublic

Authored by max on Fri, May 8, 3:37 PM.

Details

Summary

Break sub pipelines out into their own class and rename to reflect new semantics

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 created this revision.Fri, May 8, 3:37 PM
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, May 8, 3:51 PM
Harbormaster failed remote builds in B10995: Diff 13509!
Harbormaster returned this revision to the author for changes because remote builds failed.Fri, May 8, 4:44 PM
Harbormaster failed remote builds in B11006: Diff 13520!
max requested review of this revision.Fri, May 8, 6:21 PM
schrockn resigned from this revision.Fri, May 8, 6:58 PM

please add-me when this is ready to go (reviewable). trying to email thrash when there are lots of changes happening

alangenfeld added inline comments.Fri, May 8, 7:57 PM
python_modules/dagster/dagster/core/definitions/pipeline.py
477

I think I prefer
SubPipelineDefinition + get_sub_pipeline_def
SubsetPipelineDefinition + get_subset_pipeline_def

I guess i dont like that the name is so different but the classes are basically the same

490

why not just fail 100% of the time here with "SubsetPipelineDefinitions can not be subset again"?

max added 1 blocking reviewer(s): schrockn.Sat, May 9, 12:49 AM

I do not have any attachment to any of these names but you both have concerns.

python_modules/dagster/dagster/core/definitions/pipeline.py
490

we can do that, i'll defer to @schrockn who I think wanted these to be idempotently subsettable at different levels of the execution codepaths

schrockn added inline comments.Sat, May 9, 1:19 PM
python_modules/dagster/dagster/core/definitions/pipeline.py
477

i think inheritance is a good intermediate step (and an incremental improvement) but i don't think it is the long term terminal state. This is especially true in a world post Alex's ExecutablePipeline refactor.

490

oh i would definitely prefer a hard failure. I assumed that wasn't being done because it was too hard/too much of a pain for now

max added inline comments.Sat, May 9, 11:16 PM
python_modules/dagster/dagster/core/definitions/pipeline.py
477

yep

490

great, much nicer

schrockn requested changes to this revision.Sun, May 10, 6:20 PM

q mgmt. sounds like we are just going to make it raise?

This revision now requires changes to proceed.Sun, May 10, 6:20 PM
max updated this revision to Diff 13639.Mon, May 11, 7:10 PM

Rebase

max planned changes to this revision.Mon, May 11, 7:14 PM
max retitled this revision from Subset for execution to 7/ Subset for execution.
max updated this revision to Diff 13648.Mon, May 11, 8:05 PM

Hard fail on subsetting a subset

schrockn accepted this revision.Mon, May 11, 9:51 PM

great step forward

Macro yesbaby:

python_modules/dagster/dagster/cli/api.py
27–30

minor nit: generally like to avoid reassigning variables (e.g. pretend that it is const in js)

python_modules/dagster/dagster/core/definitions/pipeline.py
491

awesome

python_modules/dagster/dagster/core/instance/__init__.py
457

len check unnecessary

This revision is now accepted and ready to land.Mon, May 11, 9:51 PM
max added inline comments.Mon, May 11, 11:37 PM
python_modules/dagster/dagster/cli/api.py
27–30

yah i didn't write this, it was introduced in https://dagster.phacility.com/D2794

python_modules/dagster/dagster/core/instance/__init__.py
457

it's a question whether you think .subset_for_execution['solid_a', 'solid_a'] should magically be the same as .subset_for_execution['solid_a'], i think it would be better to throw

schrockn added inline comments.Mon, May 11, 11:39 PM
python_modules/dagster/dagster/core/instance/__init__.py
457

ah i see. we should probably disallow dups or just changed solid_subset to use set through. makes sense.

max updated this revision to Diff 13698.Tue, May 12, 12:25 AM

Rebase

max updated this revision to Diff 13703.Tue, May 12, 1:14 AM

rebase

This revision was automatically updated to reflect the committed changes.