Page MenuHomePhabricator

(scalar-unions-2) Add ScalarUnion Type
ClosedPublic

Authored by schrockn on Dec 21 2019, 12:57 AM.

Details

Summary

Adds a new type to the config system: ScalarUnionType.

This allows one to specify config schema that accepts a scalar value
OR a non-scalar value like a List, Dict, or Selector. This way the parse
is unambiguous (as far as I know)

The motivation here is to allow runtime scalars to be configured without
a dictionary with the key "value" and instead just use the scalar value
directly. However we still also want to leave the option to load scalars
from a json or pickle file.

e.g.

solids:
  multiply_the_word:
    inputs:
      word:
        value: bar

becomes, optionally,

solids:
  multiply_the_word:
    inputs:
      word: bar

Depends on D1734

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

schrockn retitled this revision from (naked-config-runtime-scalars-1) Add ScalarUnion Type to (scalar-unions-2) Add ScalarUnion Type.
schrockn edited the summary of this revision. (Show Details)

upmessage

schrockn edited the summary of this revision. (Show Details)
schrockn added reviewers: max, alangenfeld.

upmessage

If everyone thinks this is a good idea I will convert all examples and tutorials in a follow-on,

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

This is the current proposal for pretty printing these things

looks like a legit test failure - to your queue

This revision now requires changes to proceed.Jan 2 2020, 6:39 PM
python_modules/dagster/dagster/core/types/config/config_type.py
19

name feels a little off but im not sure i have a better one -when i first read this i think "I can use this to build an 'enum' by listing a set of valid scalar values in an union" as opposed to "just the scalar or a fancy way to get the scalar"

python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_validate.py
493

add a test for List[ScalarUnion]? That should work right?

python_modules/dagster/dagster/core/types/config/config_type.py
19

Yeah to be clear I want to be thinking about this as purely a feature of the config system of which the runtime scalar use case is one of potentially many. With that viewpoint in mind totally open to other names. Agree that it is not the best

alangenfeld added inline comments.
python_modules/dagster/dagster/core/types/config/config_type.py
19

I still cant come up with anything better

python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_validate.py
493

To clarify - I think its worth adding something like:

int_or_dict = ScalarUnion(
    scalar_type=resolve_to_config_type(int), non_scalar_type=Dict({'a_string': str})
)
list_of = List[int_or_dict]

assert validate_config(list_of, [2, 4, 6, 8]).success
assert validate_config(list_of, [{"a_string": "a"}]).success
# not sure about this one
assert validate_config(list_of, [{"a_string": "a"}, 4, 5]).success

unless im mistaken that this should work

This revision is now accepted and ready to land.Jan 7 2020, 5:42 PM
python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_validate.py
493

yes will add. was just getting tests working

python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_validate.py
493

and yes that will work

This revision was automatically updated to reflect the committed changes.