Page MenuHomePhabricator

singularizing executor and intermediate defs
Needs ReviewPublic

Authored by leoeer on Jul 16 2020, 3:35 PM.

Details

Summary

allowing singular params instead of list params, handling config gracefully

Test Plan

Unit + add'l

Diff Detail

Repository
R1 dagster
Branch
singularization (branched from master)
Lint
Lint OK
Unit
No Unit Test Coverage

Event Timeline

leoeer created this revision.Jul 16 2020, 3:35 PM
leoeer updated this revision to Diff 18941.Jul 16 2020, 3:37 PM

not listifying none lol

leoeer updated this revision to Diff 18947.Jul 16 2020, 3:47 PM

ensuring single config succeeds without environment params

Harbormaster completed remote builds in B15475: Diff 18944.
leoeer updated this revision to Diff 18962.Jul 16 2020, 4:17 PM

test of singularized config

leoeer updated this revision to Diff 18992.Jul 16 2020, 6:59 PM

intermediate_storage now tested

leoeer updated this revision to Diff 19004.Jul 16 2020, 7:34 PM

testing executors

leoeer published this revision for review.Jul 16 2020, 7:38 PM

Singularization is ready, I think :)

I have some comments on the details, but I think this overall "graft" approach looks very reasonable.

My main comment is that, with this approach, it seems like it wouldn't be super hard to offer an intermediate_storage_def argument instead of building special logic around the length one case of intermediate_storage_defs?

python_modules/dagster/dagster/core/definitions/environment_configs.py
147

Would we be able to accept a separate intermediate_storage_def argument here instead and follow this branch if that argument is given?

python_modules/dagster/dagster/core/system_config/objects.py
206

The caller of this function has the mode, right? And the mode knows whether there's a single or multiple intermediate storages, based on the criteria above?

So would we be able to pass something in here that ? Then we wouldn't need to rely on the string matching, which would be fine in many situations, but is a little scary.

210

Could we set the name to None in this branch and, in context_creation_pipeline.intermediate_storage_def_from_config, which is I think the only place this gets used, do something like the following:

if mode_definition.intermediate_storage_def:
    return mode_definition.intermediate_storage_def
else:
    # everything else that's currently in that function
leoeer added inline comments.Jul 16 2020, 8:41 PM
python_modules/dagster/dagster/core/definitions/environment_configs.py
147

hmmm, we could, yes

python_modules/dagster/dagster/core/system_config/objects.py
206

So, to be fair about the string matching being scary, a _ton_ of other places require this top-level string to be config, which is why I felt safe doing it. The reason I didn't want to test for single mode is I didn't want to _make_ people not use their storage name/edit their runconfigs even when they switched to a single intermediate_storage_def

210

None has a special meaning in IntermediateStorageConfig, unfortunately (it generates config for the default, which is in_memory)

leoeer updated this revision to Diff 19037.Jul 17 2020, 2:52 PM

singular def

leoeer updated this revision to Diff 19049.Jul 17 2020, 4:20 PM

singularization changes

leoeer updated this revision to Diff 19050.Jul 17 2020, 4:29 PM

snaps and plurals

leoeer updated this revision to Diff 19057.Jul 17 2020, 5:16 PM

snap linting

leoeer updated this revision to Diff 19058.Jul 17 2020, 5:34 PM

slight docstring fix

schrockn requested changes to this revision.Jul 17 2020, 5:37 PM

Before we start more of these I want to have a discussion about 1) where our end state is going to be and 2) what is going to be our process.

I certainly don't have high enough confidence in our plan in this to consider thrashing everyone again. I'm concerned we are going to do this, and then decide we need to do a totally different thing, I think we need to discuss this and collaborate on a shared technical vision, because these changes have all sorts of implications.

This revision now requires changes to proceed.Jul 17 2020, 5:37 PM
leoeer updated this revision to Diff 19060.Jul 17 2020, 5:37 PM

double warning

schrockn requested changes to this revision.Jul 17 2020, 5:37 PM
This revision now requires changes to proceed.Jul 17 2020, 5:37 PM
leoeer updated this revision to Diff 19077.Jul 17 2020, 8:53 PM

slight docstring update

schrockn resigned from this revision.Jul 20 2020, 10:02 PM

I think we are going to hold off on this for a bit until we stabilize a long term plan around this stuff

sandyryza resigned from this revision.Jul 21 2020, 6:10 PM