Page MenuHomePhabricator

6/ Allow solid_subset to be set explicitly in execute_pipeline
ClosedPublic

Authored by max on Wed, May 6, 7:15 PM.

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
yuhan accepted this revision.Wed, May 6, 10:09 PM
This revision is now accepted and ready to land.Wed, May 6, 10:09 PM
max planned changes to this revision.Wed, May 6, 10:24 PM
max updated this revision to Diff 13378.Thu, May 7, 5:35 AM

rebase

This revision is now accepted and ready to land.Thu, May 7, 5:35 AM
max planned changes to this revision.Thu, May 7, 5:36 PM
max updated this revision to Diff 13418.Thu, May 7, 7:04 PM

rebase

This revision is now accepted and ready to land.Thu, May 7, 7:04 PM
schrockn requested changes to this revision.Thu, May 7, 7:17 PM

req'ing based on my q, which i think is important

python_modules/dagster/dagster/core/execution/api.py
70–71

i think it would be a very good idea to ensure that the pipeline passed in itself was not built with build_sub_pipeline itself

This revision now requires changes to proceed.Thu, May 7, 7:17 PM
max planned changes to this revision.Thu, May 7, 7:28 PM
max planned changes to this revision.Thu, May 7, 8:10 PM
max updated this revision to Diff 13447.Thu, May 7, 9:17 PM

stack

max updated this revision to Diff 13453.Thu, May 7, 9:22 PM

rebase

schrockn requested changes to this revision.Thu, May 7, 9:36 PM

i think we should also consider renaming build_sub_pipeline to indicate idempotency/updated semantics.

@alangenfeld also want to make sure you are onboard with this

python_modules/dagster/dagster/core/definitions/pipeline.py
427–447

this is not strict enough. per discussion in slack it should be identical or fail

python_modules/dagster/dagster/core/execution/api.py
271

the solid_subset on the parent should always be None with the new invariant in place

271

samesame

This revision now requires changes to proceed.Thu, May 7, 9:36 PM
max updated this revision to Diff 13461.Thu, May 7, 9:49 PM

tighten subset restriction

what do you think about renaming build_sub_pipeline to reflect new semantics??

max updated this revision to Diff 13467.Thu, May 7, 10:06 PM

rebase

schrockn requested changes to this revision.Thu, May 7, 10:06 PM

Req'ing changes to ensure you saw question: "what do you think about renaming build_sub_pipeline to reflect new semantics??"

This revision now requires changes to proceed.Thu, May 7, 10:06 PM
max added a comment.Thu, May 7, 10:12 PM

i don't have a strong view on this. subset_pipeline() or subset_for_execution() maybe.

max updated this revision to Diff 13478.Thu, May 7, 11:24 PM

fix test

max added inline comments.Fri, May 8, 3:19 AM
python_modules/dagster/dagster/core/execution/api.py
271

different concept of parent, actually

max updated this revision to Diff 13493.Fri, May 8, 3:36 AM

Revert mistaken change to parent subset

max added inline comments.Fri, May 8, 3:37 AM
python_modules/dagster/dagster/core/execution/api.py
271

this is lineage, not subset

max updated this revision to Diff 13505.Fri, May 8, 2:35 PM

set equality

schrockn requested changes to this revision.EditedFri, May 8, 8:42 PM
This comment has been deleted.
python_modules/dagster/dagster/core/execution/api.py
271

oic. got it. thx for clarification

This revision now requires changes to proceed.Fri, May 8, 8:42 PM
schrockn accepted this revision.Fri, May 8, 8:44 PM

great stuff

re: name

consider: resolve_to_subset or even construct_subset_idempotently for extreme explicitness. however take as suggestion, definitely not for sure better.

This revision is now accepted and ready to land.Fri, May 8, 8:44 PM
max planned changes to this revision.Sat, May 9, 12:50 AM
max added a comment.Sat, May 9, 1:36 AM

See D2835 for renaming

max updated this revision to Diff 13638.Mon, May 11, 6:59 PM

Rebase

This revision is now accepted and ready to land.Mon, May 11, 6:59 PM
max planned changes to this revision.Mon, May 11, 7:00 PM
max requested review of this revision.
max retitled this revision from Allow solid_subset to be set explicitly in execute_pipeline to 6/ Allow solid_subset to be set explicitly in execute_pipeline.
This revision is now accepted and ready to land.Mon, May 11, 7:02 PM
max updated this revision to Diff 13669.Mon, May 11, 10:31 PM

Rebase

max updated this revision to Diff 13697.Tue, May 12, 12:04 AM

Rebase on master & abandon reexecute_pipeline APIs