Page MenuHomePhabricator

Make anonymous types in config type system have stable keys
ClosedPublic

Authored by schrockn on Wed, Nov 20, 3:32 PM.

Details

Summary

This has been a long-standing technical liability that IMO
become intolerable with the upcoming work to persist the type system
metadata in a database. Instead of just assigning an incremental
counter to Dicts, Selectors, etc to assign key names (so that we
can do things like have the config editor work) instead this
assigns the key based on the fields passed into types.

Critically this makes it so that import order will no longer alter
the keys.

Test Plan

BK and run dagit.

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.Wed, Nov 20, 3:32 PM
schrockn updated this revision to Diff 6740.Wed, Nov 20, 3:56 PM
schrockn retitled this revision from make dictionary keys stable to Make anonymous types in config type system have stable keys.
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)

upmessage

Probably a good idea to do this after: https://dagster.phacility.com/D1358

schrockn updated this revision to Diff 6742.Wed, Nov 20, 4:46 PM

test field order does not matter

alangenfeld requested changes to this revision.Wed, Nov 20, 5:37 PM

test for recursive /deeply nested dict structure

This revision now requires changes to proceed.Wed, Nov 20, 5:37 PM

otherwise looks solid

max resigned from this revision.Wed, Nov 20, 7:19 PM

agree with alex we should be rigorous about test coverage but this is such a good thing to finally put to bed. resigining in favor of @alangenfeld ๐Ÿ‘

python_modules/dagster/dagster_tests/core_tests/types_tests/test_field_hash.py
39

specifically, i'd like to make sure *this* is still true for deeply nested structures

schrockn updated this revision to Diff 6753.Wed, Nov 20, 7:41 PM

nested mcnestyface

This revision is now accepted and ready to land.Wed, Nov 20, 11:13 PM