Page MenuHomePhabricator

RFC: Update TypeStoragePluginRegistry to take list of tuples instead of dict
ClosedPublic

Authored by sashank on Mon, Jan 13, 7:37 PM.

Details

Summary

Since we plan to make RuntimeTypes not hashable in D1829, we can't use them as dictionary keys. This breaks multiple tests.

Currently in the codebase, we only use RuntimeTypes as keys when constructing the dict argument to TypeStoragePluginRegistry. The proposed simple fix is to just turn the dict argument to a list of tuples. Alternatively, we can figure out a way to make RuntimeType hashable.

Test Plan

unit

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

sashank created this revision.Mon, Jan 13, 7:37 PM
sashank retitled this revision from Update TypeStoragePluginRegistry to take list of tuples instead of dict to RFC: Update TypeStoragePluginRegistry to take list of tuples instead of dict.Mon, Jan 13, 7:37 PM
sashank edited the summary of this revision. (Show Details)
sashank updated this revision to Diff 8661.Mon, Jan 13, 8:54 PM

Use a subclass check instead of an instance check

alangenfeld accepted this revision.Thu, Jan 16, 10:53 PM

this seems fine, double check if you need to add the check stuff

python_modules/dagster/dagster/check/__init__.py
58–72

*

576–581

*

python_modules/dagster/dagster/core/storage/type_storage.py
46

this is already happening too

This revision is now accepted and ready to land.Thu, Jan 16, 10:53 PM

I think it might be best to leave it for type documentation purposes, even though it's being checked right after again.