Page MenuHomeElementl

ban io_manager_key and root_manager_key on composite solids

Authored by yuhan on Jun 7 2021, 6:05 PM.
Referenced Files
Unknown Object (File)
Fri, Jun 2, 5:49 AM
Unknown Object (File)
Sun, May 21, 1:58 PM
Unknown Object (File)
Fri, May 19, 3:26 PM
Unknown Object (File)
May 3 2023, 5:11 AM
Unknown Object (File)
Apr 28 2023, 12:31 AM
Unknown Object (File)
Apr 6 2023, 3:27 PM
Unknown Object (File)
Mar 19 2023, 10:51 AM
Unknown Object (File)
Mar 18 2023, 11:33 PM

Diff Detail

R1 dagster
Lint Not Applicable
Tests Not Applicable

Event Timeline

If you look in, only the IOManagers on the leaf solids take effect. So my expectation was that for root input managers, we would ban them on composite solids for consistency. Are there reasons that doesn't make sense?

we would ban them on composite solids for consistency

looks like we don't hard error on io_manager_key being set on composite, should we? Then also error on root_input_manager on composites?

I think the only place that makes sense to set these keys is on the solid, I'm having a hard time making sense of an other scheme.

looks like we don't hard error on io_manager_key being set on composite, should we?

I was imagining that we should

request review if you disagree with proposed alternative approach

This revision now requires changes to proceed.Jun 8 2021, 8:51 PM

I think the ideal behavior is to ban io_manager_key and root_input_manager on @composite_solid but allow these to be set on the inner @solids (which is quite the opposite of this diff). reasons are:

  1. users can still specify io managers for solids inside composite - imo composite is a way to better organizing solids but limit existing functionalities of its inner solids.
  2. we can keep solids' reusability - a solid that requires custom io managers can still be included in a composite.
  3. the dagster machinery can still treat composite as a container - meaning a composite has nothing to do with how io works and it would just defer to its inner solids' set up.

however, to enable inner solids to use custom io managers (which usually take custom inputs/outputs config), here's what's blocking us to do so:
currently, a user can't supply "inputs" and "outputs" for a inner solid in run config, such as:

        inputs: <input value or config> # composite doesn't take "inputs" or "outputs" config field

if we agree on this plan, i'll go ahead and make composites take "inputs" and "outputs" config for the inner solids.

error on root_manager_key and io_manager_key on composite solids

658 ↗(On Diff #39451)

this should happen in CompositeSolid constructor unless theres a reason i am missing

move _check_io_managers_on_composite_solid to _CompositeSolid so we error as soon as the composite solid is defined

73–79 ↗(On Diff #39492)

InputDefinition and its name should probably be mentioned here

80–88 ↗(On Diff #39492)

^ same but output

13–27 ↗(On Diff #39492)

I expected CompositeSolidDefinition - you would have to poke at the definitions via the mappings there but technically its the better place to put it since theoretically someone can construct these via that layer

yuhan retitled this revision from error when solids inside composite have root_manager_key to ban io_manager_key and root_manager_key on composite solids.Jun 16 2021, 6:28 PM
yuhan marked 3 inline comments as done.
  • improve error msg
  • check in CompositeSolidDefinition
  • improve error msg
  • check in CompositeSolidDefinition
73–79 ↗(On Diff #39492)

image.png (120×2 px, 44 KB)

This revision is now accepted and ready to land.Jun 18 2021, 4:08 PM