Page MenuHomePhabricator

Use dagster type display name for dagster event message
AbandonedPublic

Authored by rexledesma on Sep 22 2020, 8:33 PM.

Details

Reviewers
catherinewu
Summary

Rather than using the name property of the dagster type, we use the display name, which is guaranteed to be non-null.
Addresses https://github.com/dagster-io/dagster/issues/2773

Test Plan

Existing tests

Diff Detail

Repository
R1 dagster
Branch
rl-fix-dagster-list-type-logging-display (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Sep 22 2020, 8:54 PM
Harbormaster failed remote builds in B18601: Diff 22581!

switch back List type to have name None

python_modules/dagster/dagster/core/types/dagster_type.py
551–573

I don't think we should introduce this class just to set name = self.display_name (also don't think we should override display_name, type_check_method, inner_types, type_param_keys). I think your original suggestion of updating the error messages to use type_name=dagster_type.display_name instead of type_name=dagster_type.name would be πŸ‘Œ

861–863

hmm, this is weird because ContainerDagsterType forces the type to have a name, so we should respect that here

python_modules/dagster/dagster/core/types/dagster_type.py
551–573

This sort of solution is similar to how scalars subclass on BuiltinScalarDagsterType.

Updating the error messages to use type_name=dagster_type.display_name would only be a short term fix - then, in the future, developers must remember to use to .display_name in place of .name when printing dagster_type in string messages. I don't think that's sustainable.

Honestly, I think the best fix here would be to consolidate display_name and name together in one field somehow. DagsterType has this invariant where either key or name are required but then display_name must also be non-null, and that defaults to name so in the end, the actual invariant is both key and name must be provided, otherwise an error will show up.

Of course, the user can subclass DagsterType to override display_name to avoid this to pass in name as None but that doesn't seem like ergonomic behavior to me.

861–863

It looks like ContainerDagsterTypes (dict/set/list/optional) have no name so that this name uniqueness check is skipped for them, since they are usually constructed on the fly.

It should be fine to have names for these types as long as we remember to skip the check for them here.