Page MenuHomeElementl

correctly process configs of io managers on solids inside composite
ClosedPublic

Authored by yuhan on Jun 14 2021, 9:13 PM.

Details

Summary

depends on D8272 (which bans io managers on composite solids)

this diff makes sure solids inside composite can use their own custom io managers and the configs are processed correctly.

for example https://github.com/dagster-io/dagster/issues/3656
the code will be:

@root_input_manager(input_config_schema={"test": str})
def my_root(context):
    return context.config["test"]

@solid(input_defs=[InputDefinition("data", dagster_type=str, root_manager_key="my_root")])
def inner_solid(_, data):
    return data

@composite_solid
def my_composite():
    return inner_solid()

and the correct run config would be:

solids:
  my_composite:
    solids:
      inner_solid:
        inputs:
	  data:
            test: hello # instead of composite:inputs, we get the input config from inner solid

this should also address https://github.com/dagster-io/dagster/issues/4130 where we will only respect the io managers set on inner solids

Test Plan

units

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

yuhan requested review of this revision.Jun 14 2021, 9:57 PM
python_modules/dagster/dagster/core/definitions/pipeline.py
693–844

why the change to how resource_reqs is accumlated? this current state is a weird mix of fill pass by reference set and return value usage

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

is there not an unprotected accessor for this? maybe add one?

check inlines before landing - i think its worth cleaning up

venusaur

This revision is now accepted and ready to land.Jun 18 2021, 4:30 PM
yuhan marked 2 inline comments as done.

feedback

python_modules/dagster/dagster/core/definitions/pipeline.py
838–842

when we recurse in to composites here [1]

859–877

[1] this will fire since it doesn't account for the mapped input being hooked up at the outer/parent dependency structure