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 created this revision.Dec 21 2019, 12:57 AM
schrockn updated this revision to Diff 8114.Dec 21 2019, 7:09 PM

upmessage

schrockn updated this revision to Diff 8115.Dec 21 2019, 7:14 PM
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 updated this revision to Diff 8266.Mon, Dec 30, 7:54 PM

upmessage

schrockn updated this revision to Diff 8267.Mon, Dec 30, 8:07 PM

upmessage

schrockn updated this revision to Diff 8268.Mon, Dec 30, 8:16 PM

upmessage

schrockn updated this revision to Diff 8286.Mon, Dec 30, 9:44 PM

upmessage

schrockn updated this revision to Diff 8288.Mon, Dec 30, 9:52 PM

upmessage

schrockn updated this revision to Diff 8294.Mon, Dec 30, 10:01 PM
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,

schrockn added inline comments.Mon, Dec 30, 10:02 PM
python_modules/dagster/dagster_tests/py3_tests/test_type_examples_py3.py
261

This is the current proposal for pretty printing these things

schrockn updated this revision to Diff 8295.Mon, Dec 30, 10:04 PM

upmessage

schrockn edited the summary of this revision. (Show Details)Mon, Dec 30, 10:06 PM
alangenfeld requested changes to this revision.Thu, Jan 2, 6:39 PM

looks like a legit test failure - to your queue

This revision now requires changes to proceed.Thu, Jan 2, 6:39 PM
alangenfeld added inline comments.Thu, Jan 2, 6:42 PM
python_modules/dagster/dagster/core/types/config/config_type.py
20

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
494

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

schrockn added inline comments.Mon, Jan 6, 9:49 PM
python_modules/dagster/dagster/core/types/config/config_type.py
20

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 accepted this revision.Tue, Jan 7, 5:42 PM
alangenfeld added inline comments.
python_modules/dagster/dagster/core/types/config/config_type.py
20

I still cant come up with anything better

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

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.Tue, Jan 7, 5:42 PM
schrockn added inline comments.Tue, Jan 7, 5:44 PM
python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_validate.py
494

yes will add. was just getting tests working

schrockn added inline comments.Tue, Jan 7, 8:25 PM
python_modules/dagster/dagster_tests/core_tests/config_types_tests/evaluator_tests/test_validate.py
494

and yes that will work

This revision was automatically updated to reflect the committed changes.