Page MenuHomePhabricator

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

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

Details

Summary

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

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.Sat, Nov 23, 3:13 PM
schrockn updated this revision to Diff 6845.Sat, Nov 23, 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.

upmessage

max accepted this revision.Mon, Nov 25, 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.

python_modules/dagster-graphql/dagster_graphql/schema/config_types.py
71–72

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.

121–122

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

139

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

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

D1481 resolves the issues with black / formatting

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

zubat

python_modules/dagster-graphql/dagster_graphql/schema/config_types.py
29

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

schrockn added inline comments.Mon, Nov 25, 10:04 PM
python_modules/dagster-graphql/dagster_graphql/schema/config_types.py
29

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

121–122

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

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

new black plus more pythonic kwarg handling

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

better name