Page MenuHomePhabricator

(more-config-work-6) Killing config_field
ClosedPublic

Authored by schrockn on Dec 10 2019, 10:34 PM.

Details

Summary

This kills config_field across all of our decorators and
definitions in favor of a single config field that knows how to
coerce types and dictionaries into Field as well as Field itself.

Note that this has exposed a problem with API as when you pass
a type (e.g. Int) to decorator it is treated as callable
which we interpret as a decorator called without parens.

Resolves https://github.com/dagster-io/dagster/issues/1974t

Test Plan

BK

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

There are a very large number of changes, so older changes are hidden. Show Older Changes
schrockn updated this revision to Diff 7553.Dec 11 2019, 12:21 AM
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)
schrockn added reviewers: max, alangenfeld, nate.

upmessage

schrockn updated this revision to Diff 7579.Dec 11 2019, 6:55 PM

got remaining Field(Dict( calls

max requested changes to this revision.Dec 11 2019, 9:59 PM

this seems a straightforward usability win. we should update the docstrings to be accurate (thus requesting changes). take a look at the docs for Field which are accurate (for the status quo ante). also, do we need to make tutorial changes?

python_modules/dagster/dagster/core/definitions/executor.py
14–22

DagsterType is not a thing

67–75

ditto

python_modules/dagster/dagster/core/definitions/logger.py
15–23

ditto

python_modules/dagster/dagster/core/definitions/system_storage.py
31–39

I think this is wrong -- DagsterType is not a thing

87–95

ditto

This revision now requires changes to proceed.Dec 11 2019, 9:59 PM

yeah DagsterType isn't a thing but we don't have a concise way of describing all the different ways types can be expressed.

@max I'm interested in feedback about what we should actually say for function signature.

There are a few use cases.

  1. Field
  2. dict --> (becomes a Field(Shape(...))
  3. python primitives (one of int, float, bool, str) or their dagster.* equivalents
  4. instance of any config type e.g. (Shape, Selector)

Not sure how to express all of those

alangenfeld requested changes to this revision.Dec 16 2019, 5:10 PM

worth another repo-wide search for config_field to make sure no other spots missed

python_modules/dagster/dagster/core/definitions/decorators.py
292–293

missed this spot too

711–719

worth updating this as well with whatever the best copy we come up with is

python_modules/dagster/dagster/core/definitions/executor.py
14–22
config (Optional[Field*]): 
    The schema for the config to be made available to executor_creation_fn via context. 

     * Non-Field values will be automatically wrapped in `Field` with `dict`s wrapped in `Field(Dict())`

^ does something like this work?

python_modules/dagster/dagster/core/types/config/field_utils.py
355–362

should this example change more?

This revision now requires changes to proceed.Dec 16 2019, 5:10 PM
schrockn added inline comments.Dec 16 2019, 5:31 PM
python_modules/dagster/dagster/core/definitions/executor.py
14–22

seems pretty good

python_modules/dagster/dagster/core/types/config/field_utils.py
362

yes good call

alangenfeld requested changes to this revision.Dec 16 2019, 5:40 PM

Q mgmt

This revision now requires changes to proceed.Dec 16 2019, 5:40 PM
max added inline comments.Dec 17 2019, 1:20 AM
python_modules/dagster/dagster/core/definitions/executor.py
14–22

I think these should be documented as Optional[Any], and then we need to describe the semantic restriction in the text

schrockn updated this revision to Diff 7867.Dec 17 2019, 4:13 AM
schrockn edited the summary of this revision. (Show Details)

upmessage

python_modules/dagster/dagster/core/definitions/executor.py
14–22

@max can you just write what you think it should say

max accepted this revision.Dec 17 2019, 10:11 PM
This revision is now accepted and ready to land.Dec 17 2019, 10:21 PM
This revision was automatically updated to reflect the committed changes.