Make core config accessible in dict get way

[merelcht]

Description

#1863

Development notes

• Changed AbstractConfigLoader to inherit from UserDict
• Moved regex patterns for core config locations from KedroContext to ConfigLoader and TemplatedConfigLoader
• Core config patterns are stored in a dictionary so they can be accessed with dict syntax
• config_patterns is added as an argument to the ConfigLoader and TemplatedConfigLoader

To discuss

Is allowing users to access config in this way something we want for the future 0.19.0 version as well? How does this allow for using non file based config, e.g stored in a database?

Discussion summary

• This change opens up to ability for users to change the config file patterns.
• The pattern paths are added to the ConfigLoader and TemplatedConfigLoader and not to the AbstractConfigLoader to not make add assumptions about file based config in the base abstract class.
• This also means that (in the future) users will be able to point to a database. The implementation of reading from databases would not happen on the Kedro side: it's not possible for us to build functionality for all possible databases that users have. However, the improvements to ConfigLoaderwill enable users to implement their own DB access so they can load their config.
• In 0.19.0 we will remove the custom .get() method and rely on OmegaConf for loading the actual config.

Checklist

• Read the contributing guidelines
• Opened this PR as a 'Draft Pull Request' if it is work-in-progress
• Updated the documentation to reflect the code changes
• Added a description of this change in the RELEASE.md file
• Added tests to cover my changes

[noklam]

I like the direction of this change and it should make it easier towards customisable path for all configurations ⭐️.

This is just something always in my mind since this question almost come up every week and I always has to point people to make a copy & paste CustomTemplateConfigLoader. Would be great if we can do something about it while we are making changes to these classes. This is definitely out of scope of this PR, but I just want bring it out so we can see if there are any opportunity to improve it.
#1527

[antonymilne]

tl;dr: I think this is a good start, definitely an improvement or where things are now and probably fine to merge after thinking about the inheritance a bit more, but I'm not sure exactly how the next step will work. I'd feel more confident about it if @idanov could weight in with his thoughts!

I think there were a few major requirements here:

• enable access through getitem syntax:config_loader["params"] etc. ✅
• change all our .get calls to __getitem__ ✅
• maintain backwards compatibility so that 3rd party .get calls don't need to change ✅

So I think that in basic terms we've achieved what we wanted to here.

But I'm left with several questions about exactly how it should be done:

1. what should AbstractConfigLoader inherit from? IMO at the moment it should just be ABC still. I would personally leave the dictionary inheritance for the future if and when it becomes a purely dict-like object
2. where should the __getitem__ definition go - abstract config loader or the implementations? At the moment it's just copied and pasted between the implementations which seems a bit unDRY.
3. likewise do core_config_patterns belong in the abstract class or the implementations?
4. is the intention to remove .get entirely in the future?

tbh these points are all kind of moot though given that it's not at all clear what the abstract config loader is actually for currently...

More practically, what is actually the expected user journey following this change if I want to (a) add spark config and (b) add *parameters as a glob pattern? This is #1864 but it feels a bit hard to judge the proposed change here without understanding how that would work. Currently I think it would be something like this:

class MyConfigLoader(ConfigLoader):
    def __init__(...):
         super().__init__(...)
         self.core_config_patterns.append("*parameters")
         self.core_config_patterns["spark"] = ["spark*", ...]
         # really we should have another object that's not called "core" to add spark to

I believe @idanov's plan was instead of doing class inheritance to make this patterns dictionary directly accessible in settings.py. But I don't quite see how that works. Would we then want to pass the patterns into the constructor of ConfigLoader? And how would that eventually enable different sort of config, e.g. database vs. file? We don't need to have it all worked out now, but I'd personally feel more comfortable with this change if I could see better what the ultimate consequence will look like.

[merelcht]

As always, thank you for your thorough comments and feedback @AntonyMilneQB ⭐

But I'm left with several questions about exactly how it should be done:

1. what should AbstractConfigLoader inherit from? IMO at the moment it should just be ABC still. I would personally leave the dictionary inheritance for the future if and when it becomes a purely dict-like object

Yes I think that would make sense, see my earlier comment: #1870 (comment)

2. where should the __getitem__ definition go - abstract config loader or the implementations? At the moment it's just copied and pasted between the implementations which seems a bit unDRY.
3. likewise do core_config_patterns belong in the abstract class or the implementations?

These are good questions, my feeling is it should be in the loader implementations because it's too specific to be in the abstract class. As in the mapping from "params" to the parameter regexes isn't something the abstract implementation should care about or anyone inheriting from it. But I also agree that it's not clear what the role of the AbstractConfigLoader is anymore..

4. is the intention to remove .get entirely in the future?

Yes as far as I understand it, and instead just leverage the dict get methods.

More practically, what is actually the expected user journey following this change if I want to (a) add spark config and (b) add *parameters as a glob pattern? This is #1864 but it feels a bit hard to judge the proposed change here without understanding how that would work. Currently I think it would be something like this:

class MyConfigLoader(ConfigLoader):
    def __init__(...):
         super().__init__(...)
         self.core_config_patterns.append("*parameters")
         self.core_config_patterns["spark"] = ["spark*", ...]
         # really we should have another object that's not called "core" to add spark to

I believe @idanov's plan was instead of doing class inheritance to make this patterns dictionary directly accessible in settings.py. But I don't quite see how that works. Would we then want to pass the patterns into the constructor of ConfigLoader? And how would that eventually enable different sort of config, e.g. database vs. file? We don't need to have it all worked out now, but I'd personally feel more comfortable with this change if I could see better what the ultimate consequence will look like.

I coded this up in an older (less clean) branch: https://github.com/kedro-org/kedro/compare/abstract-config-loader, but essentially you would have something like this in settings.py:

CONFIG_LOADER_CLASS = ConfigLoader
CONFIG_LOADER_ARGS = {
   "custom_patterns":{"spark" : ["spark*/"]}
}

and inside the ConfigLoader

 if custom_patterns:
    core_patterns.update(custom_patterns)
 self.config_patterns = core_patterns

So you can add whatever custom config you have as well as overwrite any of the "core" config locations.

[noklam]

1. what should AbstractConfigLoader inherit from? IMO at the moment, it should just be ABC still. I would personally leave the dictionary inheritance for the future if and when it becomes a purely dict-like object

I just want to point out that with this PR, we already assume config_loader is a dict-like object. If we are going with ABC, then we should also add __getitem__ as the class method.

@MerelTheisenQB @AntonyMilneQB
Example:

# context.py
 params = self.config_loader["parameters"]

This won't work if a user inherit AbstractConfigLoader and implement get but not __getitem__. With that said, i may prefer ABC + get + __getitem__ to define clearly what exactly is need. dict or UserDict is assuming more than we need.

[merelcht]

1. what should AbstractConfigLoader inherit from? IMO at the moment, it should just be ABC still. I would personally leave the dictionary inheritance for the future if and when it becomes a purely dict-like object

I just want to point out that with this PR, we already assume config_loader is a dict-like object. If we are going with ABC, then we should also add __getitem__ as the class method.

@MerelTheisenQB @AntonyMilneQB Example:

# context.py
 params = self.config_loader["parameters"]

This won't work if a user inherit AbstractConfigLoader and implement get but not __getitem__. With that said, i may prefer ABC + get + __getitem__ to define clearly what exactly is need. dict or UserDict is assuming more than we need.

Yes you're right. I think for the time being though we should not get too stuck on "if users inherit from AbstractConfigLoader", because from the interviews it became clear people aren't using the AbstractConfigLoader. It won't give you a usable configloader if you inherit from it currently.

[idanov]

Awesome, such a small code change enabling so much extra functionality!

Could you add a very short description of this to the RELEASE.md?

[noklam]

Nice PR! I think the development notes needed to be updated. I would add that the config_pattern is now an argument, although it's not configurable in settings.py yet. Maybe we need to update the RELEASE.md too?

[idanov]

Awesome! Should we add a small section in the docs to show how to change file patterns through settings.py?

[merelcht]

Awesome! Should we add a small section in the docs to show how to change file patterns through settings.py?

I was planning on doing this in the follow up tasks:
#1864
#1462