Page MenuHomePhabricator

fix fan-in input mappings in composites
ClosedPublic

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

Details

Summary

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

resolves https://github.com/dagster-io/dagster/issues/3176

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

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

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.
python_modules/dagster/dagster/core/definitions/solid_container.py
162–163

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

python_modules/dagster/dagster/core/definitions/dependency.py
145–149

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

python_modules/dagster/dagster/core/definitions/graph.py
224–230

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

371–375

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

python_modules/dagster/dagster/core/definitions/dependency.py
143–165

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

python_modules/dagster/dagster/core/execution/plan/plan.py
273

[1]

address feedback, cleanup after rebase

python_modules/dagster/dagster/core/snap/solid.py
341

looking in to this

to resolve conflicts with D5391

thumbsup

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

python_modules/dagster/dagster/core/definitions/input.py
160

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.