Page MenuHomePhabricator

Eliminate selector argument from PipelineDefinition
ClosedPublic

Authored by schrockn on Nov 27 2019, 11:09 PM.

Details

Summary

This argument is actually unnecessary because the selector property
can be derived from the solid definitions and the dependency graph. The
only code path that populates this is through build_sub_pipeline and that
codepath subsets the dependencies and the solid definitions. From this
information you can construct the set of solid instances that will execute

The current PipelineDefinition allowed for the pipeline to be constructed
in an invalid state, where the ExecutionSelection can potentially "lie" about
what will be executed, or vice versa.

This state of the world might be a bit awkward. (The constructing
a new PipelineDefinition that is a subset has always been a bit strange)

However post this diff the world is more consistent and the API
less error-prone

Test Plan

BK

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

schrockn created this revision.Nov 27 2019, 11:09 PM
schrockn updated this revision to Diff 6971.Nov 27 2019, 11:35 PM
schrockn edited the summary of this revision. (Show Details)

upmessage

schrockn updated this revision to Diff 6987.Nov 28 2019, 12:54 AM

upmessage

schrockn updated this revision to Diff 6988.Nov 28 2019, 12:56 AM

upmessage

prha accepted this revision.Dec 2 2019, 5:13 PM

This looks good to me...

Also has the added benefit of making the signature match the described arguments...

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

remove comment

This revision is now accepted and ready to land.Dec 2 2019, 5:13 PM
This revision was automatically updated to reflect the committed changes.