Page MenuHomePhabricator

(incremental-meta-3) Make dauphin config type layer dumb objects

Authored by schrockn on Nov 23 2019, 3:13 PM.



This is a step in making it possible to flow through
the new meta objects into the graphql layer. In order to do this
incrementally, sometimes we will have to construct the dauphin
objects from the old config type objects, and sometimes we
will construct them from the meta objects. The meta objects
required threading around a schmea object as well to do
type lookups by key so it will be bit involved.

The config types hang out the runtime type system and
the solids so this codepath will hang around until the system
is 100% converted to using the serializable meta layer

Test Plan

BK and load dagit

Diff Detail

R1 dagster
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

schrockn created this revision.Nov 23 2019, 3:13 PM
schrockn updated this revision to Diff 6845.Nov 23 2019, 3:54 PM
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)
schrockn added reviewers: max, alangenfeld.


max accepted this revision.Nov 25 2019, 6:50 PM

ok -- please add some comments tying the black issue back to our issue so that we can untangle this when black fixes their bugs and we can upgrade. i've added a couple of comments expressing unease with some of the naming here.


should this be a method on ConfigType? tbh this whole setup makes me uneasy -- _ctor_kwargs and parent_kwargs are difficult names to unpack, especially because someone who comes along and asks, which parent, is led to dauphin.ObjectType.


this isn't new in this diff, but what's a regular config type?


cf. use of # fmt: off and #fmt: on in dagster_airflow.factory._make_ariflow_dag l. 83-88. -- at least we should link to here. this issue may be fixed in black 19.10b0, i'll check later. added our own tracking issue:

This revision is now accepted and ready to land.Nov 25 2019, 6:50 PM
max added a comment.Nov 25 2019, 7:06 PM

D1481 resolves the issues with black / formatting

alangenfeld accepted this revision.Nov 25 2019, 7:43 PM



this parent_kwargs pattern feels real weird - not sure any other approach is any better

schrockn added inline comments.Nov 25 2019, 10:04 PM

i can just explode it out here and the pass it up through us **kwargs. might be more pythonic


So right now it actually ends up being any scalar or the any type, as well as any custom config type. It's a bit of a mess. I'm making notes as I go along

schrockn updated this revision to Diff 6875.Nov 25 2019, 10:13 PM

new black plus more pythonic kwarg handling

schrockn updated this revision to Diff 6876.Nov 25 2019, 10:19 PM

better name