Page MenuHomePhabricator

(dict-where-type-is-used-1) Allow usage of raw dict where config_type is expected; improve and simplify error handling
ClosedPublic

Authored by schrockn on Jan 8 2020, 6:44 PM.

Details

Summary

This diff does two main things.

  1. Allows the more condense config type definition syntax anywhere where

resolve_to_config_type is called. This includes the arguments to
Field(), where previously one had to use the proper Dict type. The goal
is here is so that end users really never have to use Dict and we can
rename that to Shape and divorce ourselves from the runtime names

  1. Simplify error handling there. This is both an improvement and a

regression. This gets rid of the some of the context-specific error messages.
There is instead a generic error path for all config type conversions.
This now includes a stack of where you are in the config spec.

Removing the context-specific error messaging simplifies the code quite
a bit. I think that error message in combination with the stack trace
is sufficient to debug cases, but I could be persuaded that is wrong.

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

schrockn created this revision.Jan 8 2020, 6:44 PM
schrockn updated this revision to Diff 8455.Jan 8 2020, 7:01 PM
schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: alangenfeld, max.

upmessage

schrockn updated this revision to Diff 8457.Jan 8 2020, 7:07 PM

upmessage

schrockn updated this revision to Diff 8482.Jan 9 2020, 12:11 AM

organizing file

alangenfeld requested changes to this revision.Jan 9 2020, 10:21 PM

for the error messages I agree that we can tolerate losing the bit of context since the stack trace should point you in the right direction - but I think we can do better than whats in the diff as it stands

python_modules/dagster/dagster/core/errors.py
43–49

this should probably just go in the error message - will make learning much better for new users

60

cannot be converted to type

ya reads a little weird to me - I think it moving the rules in to the error message and tweaking this to "cannot be resolved, the value can be a:" (followed by rules) or something would be better

python_modules/dagster/dagster_tests/core_tests/config_types_tests/test_invalid_config_arguments.py
18–19

seeing this reenforces above comments - I think I would be puzzled for a bit if i hit this for the first time

This revision now requires changes to proceed.Jan 9 2020, 10:21 PM
schrockn updated this revision to Diff 8547.Jan 9 2020, 10:51 PM

alex feedback on error messages

alangenfeld accepted this revision.Jan 9 2020, 10:59 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/errors.py
40

just put inline if its not re-used?

42

remove the rst markup for the text in the error message not the doc block

This revision is now accepted and ready to land.Jan 9 2020, 10:59 PM
schrockn added inline comments.Jan 9 2020, 11:03 PM
python_modules/dagster/dagster/core/errors.py
40

meh don't want to deal with indenting

42

rgr

42

will address in next PR