Page MenuHomePhabricator

(more-config-work-5) Allow bare dict in lieu of Field(Dict(...))
ClosedPublic

Authored by schrockn on Dec 9 2019, 11:23 PM.

Details

Summary

Proposed change to implement #1908

Wanted to dicuss before doing work of adding full test coverage

https://github.com/dagster-io/dagster/issues/1908

Depends on D1622

Test Plan

BK

Diff Detail

Repository
R1 dagster
Branch
allow-bare-dict-for-dict-fields
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

schrockn created this revision.Dec 9 2019, 11:23 PM
schrockn updated this revision to Diff 7346.Dec 9 2019, 11:31 PM
schrockn edited the summary of this revision. (Show Details)

upmessage

schrockn updated this revision to Diff 7347.Dec 9 2019, 11:35 PM

another example

schrockn updated this revision to Diff 7348.Dec 9 2019, 11:46 PM

kick off builds and refresh phab UI

Harbormaster failed remote builds in B5948: Diff 7357!
alangenfeld added inline comments.Dec 10 2019, 4:28 PM
python_modules/dagster-graphql/dagster_graphql_tests/graphql/setup.py
261–271

If we're doing {...} --> Dict({...}) its worth considering X --> Field(X)

config={
    'field_any': Any,
    'field_one': String,
    'field_two': Field(String, is_optional=True),
    'field_three': Field(String, is_optional=True, default_value='some_value'),
    'nested_field': {
        'field_four_str': String,
        'field_five_int': Int,
        'field_six_nullable_int_list': Field(List[Optional[Int]], is_optional=True),
    },
},

In my opinion this would be reasonable to do

schrockn updated this revision to Diff 7448.Dec 10 2019, 5:07 PM

this is what type to field coercion looks like

schrockn added inline comments.Dec 10 2019, 5:13 PM
python_modules/dagster-graphql/dagster_graphql_tests/graphql/setup.py
261–271

it does make the before and after shot pretty good. i'm a bit concerned about being this loosey-goosey with our API, but it does make the common case nicer

Harbormaster failed remote builds in B6009: Diff 7449!

This is great! Comments below!

python_modules/dagster-graphql/dagster_graphql_tests/graphql/setup.py
271

This is perfect! It's a lot less verbose and a lot cleaner looking! Great work!

python_modules/dagster/dagster/core/types/field_utils.py
148–157

you probably know this but just calling out to delete this comment block for posterity.

python_modules/dagster/dagster_tests/py3_tests/test_type_examples_py3.py
420

To ensure folks don't get too loosey goosey with our API, what's the expectation for situations where someone passes an empty dict value (E.G: 'en': {}? I think before Dict would fail if you passed it 0 args right? If we want to throw an error, should we test for it?

schrockn added inline comments.Dec 10 2019, 5:40 PM
python_modules/dagster/dagster/core/types/field_utils.py
160

yeah ofc. this is just rfc. wanted to be able to go back to old code if we decided type=>Field coercion was not the way to go

python_modules/dagster/dagster_tests/py3_tests/test_type_examples_py3.py
420

i agree that should fail. can add that and a test case

goodcatch

schrockn added inline comments.Dec 10 2019, 6:59 PM
python_modules/dagster/dagster_tests/py3_tests/test_type_examples_py3.py
420

actually it turns out that this is valid because this allows us to specify keys in yaml that have no value. so this is actually valid

max added a comment.Dec 10 2019, 7:02 PM

love love lurve <3

schrockn updated this revision to Diff 7482.Dec 10 2019, 8:24 PM
schrockn retitled this revision from RFC: Allow bare dict in lieu of Field(Dict(...)) to (more-config-work-5) Allow bare dict in lieu of Field(Dict(...)).
schrockn edited the summary of this revision. (Show Details)

upmessage

ya the before & after samples in the diff are very compelling - I support moving forward with this and dropping config_field since its rendered redundant. lets get that 0.7.0 breaking changes train rollin

schrockn updated this revision to Diff 7493.Dec 10 2019, 9:04 PM

linty linty lint lint

btw will kill config_field in it's own follow on

schrockn updated this revision to Diff 7500.Dec 10 2019, 9:29 PM

Self review

schrockn updated this revision to Diff 7501.Dec 10 2019, 9:40 PM

ready for review

schrockn updated this revision to Diff 7503.Dec 10 2019, 9:44 PM

less duplicative error checking

schrockn updated this revision to Diff 7505.Dec 10 2019, 9:46 PM

clean up

alangenfeld requested changes to this revision.Dec 10 2019, 9:47 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/definitions/config.py
11 ↗(On Diff #7503)

not a fan of these type of comments - prefer github issue or just do it now

28 ↗(On Diff #7503)

this is a pretty core user facing API so we should go above and beyond on the error messages here

python_modules/dagster/dagster/core/types/config/field_utils.py
69 ↗(On Diff #7501)

clean

This revision now requires changes to proceed.Dec 10 2019, 9:47 PM
schrockn added inline comments.Dec 10 2019, 9:48 PM
python_modules/dagster/dagster/core/definitions/config.py
11 ↗(On Diff #7505)

can make issue

28 ↗(On Diff #7505)

making these error messages good is going to be a whole project (in terms of keep track of a stack etc). This actually does not make the current situation worse

schrockn updated this revision to Diff 7506.Dec 10 2019, 9:49 PM

feedback

alangenfeld added inline comments.Dec 10 2019, 9:52 PM
python_modules/dagster/dagster/core/definitions/config.py
28 ↗(On Diff #7505)

could we at least do something like "expected objects defining schema got {type}"

schrockn added inline comments.Dec 10 2019, 9:52 PM
python_modules/dagster/dagster/core/definitions/config.py
29 ↗(On Diff #7506)

actually i might be wrong here

Harbormaster completed remote builds in B6058: Diff 7505.
schrockn updated this revision to Diff 7507.Dec 10 2019, 10:07 PM

better errors; tracking even better ones in #1976

alangenfeld accepted this revision.Dec 10 2019, 10:09 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/definitions/config.py
31–33 ↗(On Diff #7507)

chefkiss

This revision is now accepted and ready to land.Dec 10 2019, 10:09 PM