Page MenuHomePhabricator

add 'source' config type
ClosedPublic

Authored by alangenfeld on Feb 5 2020, 9:07 PM.

Details

Summary

Adds a scalar union type that to allow loading values from environment variables.

Test Plan

added test

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

alangenfeld created this revision.Feb 5 2020, 9:07 PM
alangenfeld added inline comments.Feb 5 2020, 9:08 PM
python_modules/dagster/dagster/core/instance/source_types.py
1

not sure best place to put the file - put it in instance since ill be using it here first

schrockn requested changes to this revision.Feb 5 2020, 9:16 PM

This is mostly q mgmt because this yields interesting points of discussion.

Consider any other names other than StringSource?

python_modules/dagster/dagster/core/instance/source_types.py
3

not your fault, but seems like we should rename these to String, Int etc

11

lack of custom config types kind painful. i wonder what the best way to model this is

16โ€“17

the impedance mismatch between validate_config returning a result object and the contract requiring a throw is.... annoying.

python_modules/dagster/dagster_tests/core_tests/config_types_tests/test_source_types.py
20

wrap this in context manager or try finally and undo the pollution

python_modules/libraries/dagster-postgres/dagster_postgres/utils.py
8

unused?

  1. since it's a type case as EnvStr?
  2. This is another good argument for a singleton named String *or* pushing the resolve call into the ctor although that is circular dep-y
This revision now requires changes to proceed.Feb 5 2020, 9:16 PM
alangenfeld updated this revision to Diff 9342.Feb 6 2020, 10:07 PM

version using post processing - still a bit rough

schrockn requested changes to this revision.Feb 7 2020, 5:56 PM
schrockn added inline comments.
python_modules/dagster/dagster/config/config_type.py
58

???

python_modules/dagster/dagster/config/post_process.py
31โ€“56

naming kind of rough here. a bit unclear of the semantics of why another post process pass is needed.

it might be helpful to have a function that encapsulate all the ifs, name it something meaningful. Would also prevent the reassignment of the variable which would be slightly nicer

python_modules/dagster/dagster_tests/core_tests/config_types_tests/test_source_types.py
21

๐Ÿ‘๐Ÿป

This revision now requires changes to proceed.Feb 7 2020, 5:56 PM
max added inline comments.Feb 7 2020, 11:33 PM
python_modules/dagster/dagster/core/test_utils.py
110

i think this doesn't work... if a new env variable is set in env it won't be deleted by the finally clause

max added inline comments.Feb 7 2020, 11:35 PM
python_modules/dagster/dagster/config/errors.py
386

is it going to be clear to the user what "post processing" means?

python_modules/dagster/dagster/config/post_process.py
36

*declared

python_modules/dagster/dagster/core/instance/source_types.py
11

+1

16โ€“17

+1

max added a comment.Feb 7 2020, 11:36 PM

will we doc this in a follow-on?

schrockn requested changes to this revision.Feb 7 2020, 11:41 PM

req'ing on account of the self.key shenanigans

python_modules/dagster/dagster/core/instance/source_types.py
13

this is pretty sketchy. I would rather treat key as private and change (and document) the ctor

This revision now requires changes to proceed.Feb 7 2020, 11:41 PM
schrockn requested changes to this revision.Feb 10 2020, 3:59 PM

idea: change Enum to implement post_process and then rename the walk by kind to _resolve_defaults?

python_modules/dagster/dagster/config/errors.py
386

Maybe a point the post_process method could help?

python_modules/dagster/dagster/config/post_process.py
32

_resolve_defaults?

This revision now requires changes to proceed.Feb 10 2020, 3:59 PM

nice i like that idea

alangenfeld updated this revision to Diff 9453.Feb 10 2020, 7:38 PM

rename stuff

schrockn accepted this revision.Feb 10 2020, 7:42 PM

excellent

may want to take another look at error message per @max's feedback

This revision is now accepted and ready to land.Feb 10 2020, 7:42 PM
alangenfeld updated this revision to Diff 9454.Feb 10 2020, 7:48 PM

key on ScalarUnion

This revision was landed with ongoing or failed builds.Feb 11 2020, 1:35 AM
This revision was automatically updated to reflect the committed changes.