Page MenuHomePhabricator

MappableOutput(Definition)
ClosedPublic

Authored by alangenfeld on Dec 9 2020, 11:07 PM.

Details

Summary

Introduce the definition and object to yield for mapping / fan-out / dynamic orchestration.

One open question is the exact name(s) we prefer, Mappable, vs Mapping vs Mapped and whether we want to stick with one for grepabiltiy / consitency or if we want it to match the state ie MappingOutputDefinition & MappedOutput

reference prototype D4259

Test Plan

added tests

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.Dec 9 2020, 11:42 PM
Harbormaster failed remote builds in B22535: Diff 27395!

looks gooood to me! I personally like "mappable"

python_modules/dagster/dagster/core/definitions/events.py
505

nit: Name of the corresponding output definition. -> Name of the corresponding output.?

python_modules/dagster/dagster/core/execution/plan/compute.py
105

this seems to require that a "Mappable Solid" returns at least one MappableOutput -- I don't think we need to require that?

python_modules/dagster/dagster/core/execution/results.py
403

Update comment? "Keys of this dictionary are output names; for normal/non-mappable output, value is output value; for mappable output, value is a dictionary of mapping_key to value"

This revision is now accepted and ready to land.Dec 11 2020, 10:27 PM
python_modules/dagster/dagster/core/execution/results.py
442

adding a type for results in the future will probably help people interact w/ results.

I wonder if we should use a dummy mapping_key for non-mappable outputs. Not super opinionated

python_modules/dagster/dagster/core/execution/plan/compute.py
105

we just fire an info log so is harmless - can correct later if log is annoying

This revision was automatically updated to reflect the committed changes.