Page MenuHomePhabricator

singularizing executor and intermediate defs

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



allowing singular params instead of list params, handling config gracefully

Test Plan

Unit + add'l

Diff Detail

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

Event Timeline

ensuring single config succeeds without environment params

intermediate_storage now tested

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?


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


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.


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
    # everything else that's currently in that function

hmmm, we could, yes


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


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

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
This revision now requires changes to proceed.Jul 17 2020, 5:37 PM

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

max abandoned this revision.
max edited reviewers, added: leoeer; removed: max.