Page MenuHomePhabricator

Guard against inscrutable error messages when passing badly-behaved values in place of types
ClosedPublic

Authored by max on Thu, Nov 7, 7:29 PM.

Details

Summary

Resolves https://github.com/dagster-io/dagster/issues/1717, in place of D1365. Stacked on D1382.

This solves for an exotic class of inscrutable errors that arise when ill-behaved objects
(either nonhashable or noncomparable) find their way into APIs that expect to see types or the
typealikes from typing, etc.

Test Plan

Unit

Diff Detail

Repository
R1 dagster
Branch
check-dagster-type
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

max created this revision.Thu, Nov 7, 7:29 PM
max updated this revision to Diff 6345.Thu, Nov 7, 7:37 PM

Black

Harbormaster failed remote builds in B5109: Diff 6349!
max updated this revision to Diff 6352.Thu, Nov 7, 9:41 PM

py27

max updated this revision to Diff 6354.Thu, Nov 7, 9:47 PM

py27

schrockn requested changes to this revision.Thu, Nov 7, 10:18 PM
schrockn added inline comments.
python_modules/dagster/dagster/core/types/runtime.py
655

This error also a bit confused imo. I think it should be more like:

Dagster types must be a type in python.typings, a python primitive type, a type imported by the dagster module (e.g. dagster.Int), or a class annotated by as_dagster_type or @dagster_type.

Use the same verbiage for the hashable case.

Thoughts?

python_modules/dagster/dagster/core/types/typing_api.py
64

why this change?

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_definition_errors.py
327

maybe note the exact use case that made this necessary (pandas?)

This revision now requires changes to proceed.Thu, Nov 7, 10:18 PM
max added inline comments.Thu, Nov 7, 10:38 PM
python_modules/dagster/dagster/core/types/runtime.py
655

Yeah, this is also not accurate, but I can maybe try to write something accurate.

python_modules/dagster/dagster/core/types/typing_api.py
64

this is a lint thing but for good reason -- this not is the reason why anything that overrides boolean comparison will fail (like pd.DataFrame), vs. the is comparison to None which you can do on any object

python_modules/dagster/dagster_tests/core_tests/definitions_tests/test_definition_errors.py
327

sure

schrockn accepted this revision.Thu, Nov 7, 10:48 PM
schrockn added inline comments.
python_modules/dagster/dagster/core/types/runtime.py
620

let's eliminate the copypasta before commit (there should be a single function that generates this string)

python_modules/dagster/dagster/core/types/typing_api.py
64

πŸ‘πŸ»

This revision is now accepted and ready to land.Thu, Nov 7, 10:48 PM
max added inline comments.Thu, Nov 7, 10:49 PM
python_modules/dagster/dagster/core/types/runtime.py
620

it's two different messages -- this is likely to be raised when someone has accidentally passed an instance rather than a type

max updated this revision to Diff 6380.Thu, Nov 7, 11:12 PM

Black