Page MenuHomeElementl

ban io_manager_key and root_manager_key on composite solids
ClosedPublic

Authored by yuhan on Jun 7 2021, 6:05 PM.

Diff Detail

Repository
R1 dagster
Branch
yuhan/root-io-3656
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

yuhan requested review of this revision.Jun 7 2021, 6:30 PM

If you look in test_io_manager_composites.py, 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
yuhan requested review of this revision.Jun 11 2021, 9:08 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:

solids:
  my_composite_solid:
    solids:
      inner_solid:
        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

yuhan edited the summary of this revision. (Show Details)

up

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

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

python_modules/dagster/dagster/core/definitions/decorators/composite_solid.py
73–79 ↗(On Diff #39492)

InputDefinition and its name should probably be mentioned here

80–88 ↗(On Diff #39492)

^ same but output

python_modules/dagster/dagster/core/definitions/decorators/composite_solid.py
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
python_modules/dagster/dagster/core/definitions/decorators/composite_solid.py
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