Page MenuHomePhabricator

(refactor-input-source-to-thunk-4) Add checks to catch errors when new source types are added
ClosedPublic

Authored by schrockn on Nov 28 2020, 7:10 PM.

Details

Summary

For cases where we don't capture behavior differences in polymorphic methods, adding
invariants to ensure that we don't miss anything when source types are added.

Test Plan

BK

Diff Detail

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

Event Timeline

I like the intention, but am not sure about this implementation. In imagining hitting these errors, I think AllSourceTypes feels odd and I believe it would be better to just refer to all the types directly in the sites worth defending. Maybe have a union or marker interface for FromConfig & FromDefaultValue to communicate "I already have all the information i need and dont depend on external".

python_modules/dagster/dagster/core/execution/plan/active.py
131–148 ↗(On Diff #26523)

i think this particular check is specific enough that it doesn't need defending against

python_modules/dagster/dagster/core/execution/plan/execute_plan.py
165–171
elif isinstance(source, (FromConfig, FromDefaultValue))
  pass # value is sourced directly
else:
  check.failed(f'Unhandled step input source {source}')
python_modules/dagster/dagster/core/storage/intermediate_storage.py
64–69
elif isinstance(source, (FromConfig, FromDefaultValue))
  pass # value is sourced directly
else:
  check.failed(f'Unhandled step input source {source}')
This revision now requires changes to proceed.Nov 30 2020, 7:26 PM

excellent feedback. will update

python_modules/dagster/dagster/core/execution/plan/active.py
148 ↗(On Diff #26523)

i'm a bit concerned about missing this if a type parallel to FromStepOutput is created (e.g split out two cases for asset store vs intermediates) but won't fight it

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