allowing singular params instead of list params, handling config gracefully
Diff Detail
- Repository
- R1 dagster
- Branch
- singularization (branched from master)
- Lint
Lint OK - Unit
No Unit Test Coverage
Event Timeline
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 |
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) |
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.
I think we are going to hold off on this for a bit until we stabilize a long term plan around this stuff