Page MenuHomePhabricator

Pass tags to PipelineSubsetDef
ClosedPublic

Authored by johann on Aug 10 2020, 3:08 PM.

Details

Summary

Not positive this is the correct behavior, but it seems like it may have been an oversight.

https://github.com/dagster-io/dagster/issues/2792 found that pipeline tags weren't applied to subsets.

I can't reproduce the problem on my own EKS, so I'm not sure this would fix the issue.

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

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

PipelineDefinition constructor:

def __init__(
        self,
        solid_defs,
        name=None,
        description=None,
        dependencies=None,
        mode_defs=None,
        preset_defs=None,
        tags=None,
        _parent_pipeline_def=None,  # https://github.com/dagster-io/dagster/issues/2115
    ):

preset_defs and description aren't passed either, should they be?

johann retitled this revision from pass tags to pipeline subsets to Pass tags to PipelineSubsetDef.Aug 10 2020, 3:17 PM
johann edited the summary of this revision. (Show Details)
johann added reviewers: max, nate, alangenfeld.

passing tags looks good to me

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

preset_defs also has info about solid subset. we may want to leave it out of the PipelineSubsetDefinition -- im not sure if passing it to the sub pipeline def would conflict with the subsetted solids in the pipeline, because presets could be selecting some solids that are not present in the PipelineSubsetDefinition.

the subsetting path is definitely not in a good shape. we may also want to break the inheritance from PipelineDefinition, which is going to be a larger discussion but just throwing ideas/context here.

This revision is now accepted and ready to land.Aug 11 2020, 8:29 PM
python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_tags.py
41

this is good! but just to be extra safe, let's also test the instance.create_run(pipeline_def, solids_to_execute={'some_solid'}) path that is what the graphql layer would call when the user hit the "launch pipeline" in dagit

This revision was automatically updated to reflect the committed changes.