up

This looks great.

up

LGTM!

This is looking close - I added a few more comments,.

What concerned me about doing structuring the migration guide at the major version level was that, if someone is migrating from, say, 0.8.5 to 0.9.0, the migration guide tells about a bunch of changes / deprecations from 0.8.1-4 that aren't relevant to them. Which makes migration scarier and more confusing.

This looks great! My one lingering concern is about whether we need to maintain compatibility with the TypeStoragePlugin interface for now.

Regarding the open questions you left inside the config_map functions, I don't have direct experience with logger / executor configuration edge cases. However, my high level understanding of how it should work is:

• If we've successfully processed config, every logger/executor/resource entry in the run config dict will correspond to a logger/executor/resource definition in the mode.
• The reverse is not true. I.e. not all definitions in the mode will correspond to an entry in the run config dict. For resources, it's not true because some resources don't require configuration, in which case they don't need to be mentioned in the run config dict. For executors, it's not true because the mode definition makes a set of executors definitions available, but only one is chosen in config.

up

@yuhan - great point, I added in the schedule page.

👍

up

up

up

up

up

up

up

up

up

up

up

up

@schrockn thanks for the accept, but I had noticed some trickiness around classes and wanted to see if you had an opinion on the approach. Comment repeated here for your convenience:

LGTM! Put an underscore before configured_config_mapping_fn to be consistent with previous change?

Had one small comment. Otherwise this LGTM!

up

up

up

The class situation is actually trickier. Decorating __init__ just gives a warning saying "__init__ is experimental". Ideally we'd include the class name in the warning. A few potential solutions:

• A decorator for the class. I implemented this in the latest version. A hitch is that there's no built-in equivalent of functools.wraps for classes, so would need to implement our own if we want the class to behave exactly like the undecorated version. Here's one way to do this: https://stackoverflow.com/a/6394966
• Expect the user to explicitly name the class when decorating __init__.
• Expect the user to call experimental_class_warning from their __init__.

up

The warnings library already has pretty robust functionality around letting users mute warnings: https://docs.python.org/3/library/warnings.html

I noticed a final few small issues. Once those are fixed, this looks 100% good to go!

for classes would be annotate the init method?

What about experimental arguments to existing APIs?

up

This LGTM! Maybe we should just replace the non-run-scoped one with this. Thoughts @max?

up

LGTM!

Would it make sense to raise an error when configured is called on any resource that has an empty config schema? From the perspective of configured, a resource with all its configuration curried in is identical to a resource that had no configuration in the first place.

This LGTM! Should we unmark those xfail test?