Page MenuHomePhabricator

(refactor-input-source-to-thunk-1) Push loading behavior to StepInputSource and make it a thunk

Authored by schrockn on Nov 28 2020, 5:26 PM.



I've been prototyping input loading/asset store variants to
ground discussions with sandy in the current underlying reality of the
system. I've found adding new loading methods and consolidating them
with old methods to be quite challenging, so I've been refactoring along
the way.

I think this is net positive even if I don't end up commiting further

This builds on alex's work to organize these paths, and pushes more
behavior into polymorphic methods in the StepInputSource hierarchy.
I found _value_for_input_source to be very confusing for a couple

  1. In the list case the dagster type on FromStepOutput was List[T]

event though it pointed to an output of T. This required a call
of get_inner_type_for_fan_in in _value_for_input_source which
was quite confusing. Instead we change FromStepOutput to contain
a dagster_type property which accurately reflections the type
of the output where it came from

  1. We also curry in the behavior of check_for_missing which

makes it so we no longer need to thread that around.

This makes it when we add other source types (I think we will
keep a few around for backwards compat and I think we may experiment
with things as well)

Follow up diffs do this similarly for required_resource_keys()
and compute_version()

Test Plan


Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

schrockn retitled this revision from Push loading behavior to StepInputSource and make it a thunk to (refactor-input-source-to-thunk-1) Push loading behavior to StepInputSource and make it a thunk.
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: sandyryza, alangenfeld, yuhan.



This callsite is useful to understand. Instead of calling get_inner_type_for_fan_in and threading down check_for_missing in the loading codepath, instead we just curry them here.

very nice, this seems obvious in hindsight



This revision is now accepted and ready to land.Nov 30 2020, 5:24 PM