Page MenuHomePhabricator

fix fan-in input mappings in composites

Authored by alangenfeld on Nov 24 2020, 5:56 PM.



Currently if you attempt to fan-in any input mappings in a composite definition we error out. This takes an approach of fixing this without changing the input/output mapping APIs. It's not obvious to me how the output mappings would work in that version of the world, and breaking changes have their own inherent cost. This approach is a little awkward, but I think maybe the way to go.

depends on D5217


Test Plan

added test cases

manual verify snapshots including fan-in input mappings render properly in dagit by executing the composition toy pipeline and loading the snapshot in dagit --empty-workspace

Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 24 2020, 6:14 PM
Harbormaster failed remote builds in B21669: Diff 26301!
alangenfeld retitled this revision from fix fan-in input mappings to fix fan-in input mappings (without breaking changes).Nov 24 2020, 6:48 PM
alangenfeld edited the test plan for this revision. (Show Details)
alangenfeld edited the summary of this revision. (Show Details)
alangenfeld added a parent revision: D5217: StepInput refactor.
alangenfeld retitled this revision from fix fan-in input mappings (without breaking changes) to fix fan-in input mappings in composites.Nov 24 2020, 9:54 PM
alangenfeld added a reviewer: schrockn.
alangenfeld added inline comments.

deleting in D5269

rebase and get accidental changes back in to parent diff

When you say "fixing things", is there a github issue or something that describes what needs to be fixed?

When you say "fixing things", is there a github issue or something that describes what needs to be fixed?

oops - updated summary

schrockn requested changes to this revision.Dec 1 2020, 6:47 PM

my concerns mostly around passing fan_in_index around


where is a call site where this is not hardcoded to None? This seems really problematic


should we have a pointer object that captures (solid_name, input_name, fan_in_index) or (input_name, fan_in_index)?

threading that around gives me bad vibes


this function getting quite long and breaking it up might be nice

This revision now requires changes to proceed.Dec 1 2020, 6:47 PM

will think on feedback, might need to grab you to talk through ideas


we only use container_mapped_input directly at [1] with an index without doing the check container_maps_input first



address feedback, cleanup after rebase


looking in to this

to resolve conflicts with D5391


@sandyryza definitely take a pass too given how active you are right now in these codepaths


should this be optional? seems like if it is None this should be an InputPointer

This revision is now accepted and ready to land.Dec 3 2020, 11:38 PM
This revision was automatically updated to reflect the committed changes.