Allow registering of custom resolvers to OmegaConfigLoader

[ankatiyar]

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

Resolve #2622

Development notes

• Add a new parameter custom_resolvers and a new function _register_new_resolvers to OmegaConfigLoader
custom_resolvers is a dict of resolver name -> resolver and can be passed through the project's settings.py

from kedro.config import OmegaConfigLoader
CONFIG_LOADER_CLASS = OmegaConfigLoader

CONFIG_LOADER_ARGS = {
    "custom_resolvers": {
        "add_2": lambda x: x+2,
        "plus_10": lambda y: y+10,
    }
}

oc.env can be activated like this -

from kedro.config import OmegaConfigLoader
CONFIG_LOADER_CLASS = OmegaConfigLoader

from omegaconf.resolvers import oc

CONFIG_LOADER_ARGS = {
    "custom_resolvers": {
        "oc.env": oc.env,
    }
}

• The function OmegaConfigLoader._register_new_resolvers() uses the omegaconf function register_new_resolver() under the hood. The replace parameter is set to True so that builtin resolvers can be overwritten.
• Added a test for custom resolvers to test_omegaconf_config.py
• Documentation ticket to follow separately - Document the use of custom resolvers with OmegaConfigLoader #2624
• TODO - add this to release notes but holding off till Kedro 0.18.12 release.

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]

Leave some comments, thanks for the PR the description is clear and help a lot for review :)

[noklam]

Does it generates any log when a resolve is replaced?

[noklam]

Why do we want to override built-in resolver? Maybe I am missing some context in the issue.

[ankatiyar]

I think we decided in the tech design to have this flag set to true so users have the option to overwrite the built-in resolvers.
No logs generated, I think it might show up twice per resolver because config loader is initialised twice during one run (once when logging is set up and once again when context is created)

[noklam]

I think it would be nice to produce some log here. Either via OmegaConf builtin logging (if any) or we should have log when it is overwritten.

I think it should be an INFO level log because normally you won't overwrite a built-in resolver. Is there any risk if one want to overwrite built-in?

However, if it is producing duplicate logs, maybe it should be a DEBUG level.

Note that Logging no longer need config_loader, see #2642 This is merged into develop only (0.19)

[noklam]

is str(tmp_path) strictly needed?

[ankatiyar]

I think so, conf_source is required argument

[noklam]

Sorry, I mean if tmp_path alone works? I check the definition of AbstractConfigLoader have a signature of conf_source: str, but I expect it should be conf_source : str | Path.

A quick experiment suggests that it should work

cc @merelcht

[merelcht]

I think this generally looks good. I agree with @noklam on having a test to check the overwriting works. We should also mention clearly in the docs that replace=True is set when the register new resolvers is called.

[ankatiyar]

I've added a note to #2624 to document the fact that replace flag is set to True

[merelcht]

Is it possible to add a check/assert first to show that "test_size" is set to 20 and then after the overwriting it will be 30? Maybe just by calling omegaconf directly on the param_config?

[ankatiyar]

Updated the test

[merelcht]

LGTM! 😄

[ankatiyar]

@merelcht, I've turned off "replace=True" after discussion with @noklam. The only use case for that would be overwriting the built-in resolvers (oc.create, oc.decode, oc.deprecate, oc.select, oc.dict) which probably not a lot of people would want to do. To switch on oc.env does not require it being replaced since we clear it entirely at initialisation of OmegaConfigLoader. OmegaConf also does not allow replacing resolvers by default. Either way, it doesn't make too much of a difference to the implementation. What do you think?

Also updated the test to check if registering oc.env works properly.

[merelcht]

@merelcht, I've turned off "replace=True" after discussion with @noklam. The only use case for that would be overwriting the built-in resolvers (oc.create, oc.decode, oc.deprecate, oc.select, oc.dict) which probably not a lot of people would want to do. To switch on oc.env does not require it being replaced since we clear it entirely at initialisation of OmegaConfigLoader. OmegaConf also does not allow replacing resolvers by default. Either way, it doesn't make too much of a difference to the implementation. What do you think?

I think that's a fair point. If it turns out that users want to be able to replace resolvers we could potentially add another argument to CONFIG_LOADER_ARGS where they can overwrite it, e.g. replace_resolvers. Thanks for discussing this in more detail. I'm happy with this approach 🙂

Also updated the test to check if registering oc.env works properly.

👍

[merelcht]

Don't forget to update the release notes! Otherwise, this looks all good from my side 👍

[ankatiyar]

For advanced users that want to overwrite the built-in resolvers, they can always do so directly in settings.py -

from omegaconf import OmegaConf
OmegaConf.register_new_resolver(name, resolver, replace=True)

[noklam]

@merelcht, I've turned off "replace=True" after discussion with @noklam. The only use case for that would be overwriting the built-in resolvers (oc.create, oc.decode, oc.deprecate, oc.select, oc.dict) which probably not a lot of people would want to do. To switch on oc.env does not require it being replaced since we clear it entirely at initialisation of OmegaConfigLoader. OmegaConf also does not allow replacing resolvers by default. Either way, it doesn't make too much of a difference to the implementation. What do you think?

I think that's a fair point. If it turns out that users want to be able to replace resolvers we could potentially add another argument to CONFIG_LOADER_ARGS where they can overwrite it, e.g. replace_resolvers. Thanks for discussing this in more detail. I'm happy with this approach 🙂

Also updated the test to check if registering oc.env works properly.

👍

@merelcht We also discussed this point. Adding a new replace argument isn't hard, but it clutters the already long argument list.

Two reasons why I think we shouldn't do it now.

1. If we want to add replace argument, arguably we may need to add a few more since omegaconf allows few extra options. If so should we provide a omegaconf_kwargs instead of just one argument? It's not clear for now.
2. We can always add a new one, and it wouldn't be a breaking change. However if we decide to add replace as an arguments, but later we want to change it to kwargs, it will be a breaking change and we cannot do it easily.

So for now I think we have a good workaround for advance use case which won't be blocking. If there are more evidence coming in later we can then discuss more about 1. and have a hollistic view of what other arguments we may need to support.

[noklam]

I also open a new issue to fix the tests and signature in one go. Will refine the description later.

[noklam]

⭐️