Page MenuHomeElementl

Enable resource dependencies within memoizable io managers
ClosedPublic

Authored by cdecarolis on May 24 2021, 4:56 PM.

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.May 24 2021, 5:15 PM
Harbormaster failed remote builds in B31055: Diff 38195!

Fix resources_for_io_manager callsites

python_modules/dagster/dagster/core/execution/context/output.py
201

In what situations would this be None?

python_modules/dagster/dagster/core/execution/context/output.py
201

Approximately None

python_modules/dagster/dagster/core/execution/context/output.py
201

😂

In that case, can we make it non-optional? Then can avoid the cast below?

python_modules/dagster/dagster/core/execution/context/output.py
201

yeah honestly if its not a user facing API i think we should avoid = None ing stuff out and just be explicit at all call sites. Too many bugs from accidental omission of optional arguments

201–202

passing the whole mode and scoped builder separately feels a bit odd to me

  • maybe just take resource_defs directly instead of mode
  • maybe just take the resources directly and only allow that if step_context is None?

Address comments: get rid of defaults on get_output_context, pass resources directly to get_output_context

add test for pass both error

This revision is now accepted and ready to land.Wed, May 26, 4:05 PM