Page MenuHomePhabricator

add 'source' config type
ClosedPublic

Authored by alangenfeld on Wed, Feb 5, 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.Wed, Feb 5, 9:07 PM
alangenfeld added inline comments.Wed, Feb 5, 9:08 PM
python_modules/dagster/dagster/core/instance/source_types.py
2

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.Wed, Feb 5, 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
4

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

12

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

17โ€“18

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
21

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

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

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.Wed, Feb 5, 9:16 PM
alangenfeld updated this revision to Diff 9342.Thu, Feb 6, 10:07 PM

version using post processing - still a bit rough

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

???

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
22

๐Ÿ‘๐Ÿป

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

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.Fri, Feb 7, 11:35 PM
python_modules/dagster/dagster/config/errors.py
387

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

python_modules/dagster/dagster/config/post_process.py
37

*declared

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

+1

17โ€“18

+1

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

will we doc this in a follow-on?

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

req'ing on account of the self.key shenanigans

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

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

This revision now requires changes to proceed.Fri, Feb 7, 11:41 PM
schrockn requested changes to this revision.Mon, Feb 10, 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
387

Maybe a point the post_process method could help?

python_modules/dagster/dagster/config/post_process.py
33

_resolve_defaults?

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

nice i like that idea

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

rename stuff

schrockn accepted this revision.Mon, Feb 10, 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.Mon, Feb 10, 7:42 PM
alangenfeld updated this revision to Diff 9454.Mon, Feb 10, 7:48 PM

key on ScalarUnion

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