Page MenuHomePhabricator

(intermediate-storage-instance-config-2) Substanially restructure ConfigurableClass as ConfigPlugin

Authored by schrockn on Oct 20 2019, 4:43 PM.


Group Reviewers
Restricted Project

First this is a relatively big change in one diff. I'm up for
splitting this up but I do believe it is manageable as is.

I was sorting through the ConfigurableClass stuff and was a bit
confused. It seemed overly complex for what it was trying to accomplish,
so I decided to rethink this a little bit. I think the core problem
was the multiple inheritance, which muddled the clarity of this
subsystem. This is a capability which is really a lot simpler
and is in the province of the config type system, rather than where
it used to live, in serdes.

A config plugin boils down to a config schema and a function that
takes a value validated by that schema and produces an object. It's
very similar to input schema and output schema actually, except
that the config is only evaluated lazily rather than upfront.

With that new framing, we can express this more simply as a decorated
function. Instead of inheriting from ConfigurableClass you just implement
a function decorated by @config_plugin.

An implementation looks like this:

@config_plugin(SystemNamedDict('PostgresRunStorageConfig', {'postgres_url': Field(String)}))
def postgres_event_log_storage_config_plugin(plugin_config):
    return PostgresEventLogStorage(plugin_config['postgres_url'])

To do this I moved the "inst_info" functionality up to the
DagsterInstance, which knows whether or not the objects were
constructed via the plugin system or not. This keeps the
fact there is a plugin system at all out of the core objects,
which is preferable in my view.

I was throughly confused by the arbitary kwargs flowed up and down
this codepath. It was unused, so I eliminated it. I think shuffling
the config values to the init method manually is far more explicit
and clear than relying on kwargs explosion.

This ends up simplifying both the implementation (negative LOC) and
the user-facing API, and is a nice win all-around.

Note that I will have to test the dagster-aws stuff in a follow on diff,
because the best it can is run against master, rather than a local copy.

Test Plan

BK. View instance info in Dagit

Diff Detail

R1 dagster
Lint OK
No Unit Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
schrockn updated this revision to Diff 5932.Oct 20 2019, 7:17 PM
schrockn retitled this revision from (intermediate-storage-instance-config-2) do we need these props to (intermediate-storage-instance-config-2) Substanially restructure ConfigurableClass as ConfigPlugin.
schrockn edited the summary of this revision. (Show Details)
schrockn edited the test plan for this revision. (Show Details)
schrockn added reviewers: Restricted Project, max, alangenfeld.


schrockn updated this revision to Diff 5933.Oct 20 2019, 7:21 PM
schrockn edited the summary of this revision. (Show Details)


looks good to me, ill give @max a chance to look


should we make this backwards compatible til 0.7.0 and accept plugin or class?

schrockn added inline comments.Oct 22 2019, 1:14 PM

uh oh are we going to have to add is_deprecated to dagster

max added a comment.Oct 23 2019, 4:29 PM

This is fine. I would prefer Plugin or ConfigurablePlugin to ConfigPlugin, which is a very tricky name -- people are going to write their own, and also swap them in and out of instance config. I'm fine with ripping out the kwargs passing and just requiring schemas to be explicit.






these are pretty bad names to be user-facing

max accepted this revision.Oct 23 2019, 4:29 PM
This revision is now accepted and ready to land.Oct 23 2019, 4:29 PM
alangenfeld added inline comments.Oct 23 2019, 4:30 PM

I do think we should avoid breaking peoples dagster.yaml so its worth addressing this before landing

Let's chat name when I get in. I think it's pretty subtle.

@alangenfeld I will do the backwards compat thing

schrockn added inline comments.Oct 25 2019, 1:42 PM


schrockn added inline comments.Oct 25 2019, 1:44 PM

@alangenfeld thinking about this a little more, we are going to break them anyways because the names of the plugins are changing. We could assign the old class name to the new plugin instance, but this is getting pretty far into the weeds. Thoughts?

alangenfeld added inline comments.Oct 25 2019, 3:06 PM

I think this is a pretty bad user experience left unmitigated - maybe we should just write a fixup in the loading path. Its pretty safe to just see the old names and replace them with the new ones. I doubt anyone has a custom plugin - but its fair to assume some folks (any one using dagster-aws) have set up postgres backed storage

schrockn planned changes to this revision.Oct 25 2019, 8:02 PM

going to do migration stuff per discussion with @alangenfeld

This revision is now accepted and ready to land.Oct 25 2019, 10:18 PM
schrockn planned changes to this revision.Dec 21 2019, 4:37 PM

I still strongly prefer to this composition-based approach to the mixin-approach. However I think this change should be done when we do an inevitable breaking iteration on the core instance config stuff.

schrockn abandoned this revision.Fri, Jun 19, 4:45 PM