Add globals feature for OmegaConfigLoader using a globals resolver

[ankatiyar]

Description

Resolve #2794

Development notes

• Added globals patterns to config_patterns
  • Can be changed/added in your project's settings.py through CONFIG_LOADER_ARGS NOTE: this is already present in OmegaConfigLoader and this step is only needed if users want a different pattern

  CONFIG_LOADER_ARGS = {
      "config_patterns" = {
          "globals": ["globals*", <etc>]
       }
   }

  ❓ [QUESTION] In the attached issue, I've mentioned passing globals patterns as separate argument "globals_pattern" and not as a part of "config_patterns". Does this make more sense?

Try loading globals as a dict and assign to self._globals when OmegaConfigLoader is initialised.

SECTION BELOW NOT RELEVANT ANYMORE

• This needed me to do a some changes to the __getitem__() function since I couldn't use self["globals"] inside the __init__ function without causing weird issues. So I separated the actual loading logic into _get_config(key) which is called through __getitem__(key)
  • Did some refactoring of __getitem__() and load_and_merge_dir() functions while I was at it. ❓ [QUESTION] Should I keep this or can also revert this? (I was trying a different approach which is why I separated the _get_filtered_conf_paths() and _load_config_files() initially but it is not strictly required for this implementation)

• In the __init__ constructor -> self._register_globals_resolver() is called which registers a "globals" custom resolver to OmegaConf which calls the _get_globals_value() to resolve the value of the variable from config_loader["globals"] this happens when the config is loaded and merged by load_and_merge_dir_config()

To Test this PR

• settings.py - Only if you want to change the default patterns

from kedro.config import OmegaConfigLoader
CONFIG_LOADER_CLASS = OmegaConfigLoader
CONFIG_LOADER_ARGS = {
    "config_patterns": {
        "globals" : {"globals*"},
    },
}

• conf/base/parameters.yml (or any other conf file)

my_param : "${globals:my_global_var}"

• conf/base/globals.yml

my_global_var: 34

❓ [QUESTION] Need feedback on the overwriting behaviour of globals - currently this is the same as TemplatedConfigLoader

• if I have globals in local they will override any globals in base
• If I have a global in local that doesn’t exist in the globals.yml in base then it should still be used in any catalog.yml or parameters.yml in base
• If I run kedro run --env=base none of the globals.yml in local should be used because the base environment has been explicitly selected

[TODO]

Waiting for feedback on the general approach before doing the following tasks -

• Release notes
• More tests
• Docs

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

[merelcht]

I'm not quite sure I find the refactored code easier to follow. Especially extracting the loading code _load_config_files makes it harder for me to follow the code. @noklam might have a different view though. He started some refactoring a while back in #2591.

My main concern with the code though is around the "private" and "public" methods and the ease of customising OmegaConfigLoader for users.

I think it makes sense to have globals in config_patterns and not add a separate argument for globals_patterns.

[noklam]

I'm not quite sure I find the refactored code easier to follow. Especially extracting the loading code _load_config_files makes it harder for me to follow the code. @noklam might have a different view though. He started some refactoring a while back in #2591.

My main concern with the code though is around the "private" and "public" methods and the ease of customising OmegaConfigLoader for users.

I think it makes sense to have globals in config_patterns and not add a separate argument for globals_patterns.

In general I think some methods are too big, signified with the fact that we have a function called load_and_merge_dir_config. a xxx_and_yyy function usually is a hint that this is hard to test because it does multiple things at the same time. In https://github.com/kedro-org/kedro/pull/2591/files I try to break it into 3 steps

1. Build up the path to load config (aware of the pattern)
2. Load config (only aware of the path)
3. Merge config (aware of the config / dictionary)

• [Proposal] - Replace ConfigLoader with ConfigLoader and ConfigResolver #2481
  This issue further push the idea that ConfigLoader isn't really a "loader" because it does "load" and "resolve". We could then break it into two class so potentially we could take config anywhere (other than filebase). Conceptually I think this is the right thing to do but there isn't immediate use cases.

class FileConfigLoader(AbstractConfigLoader):
    """ConfigLoader implementation for loading configuration from a file."""
    # This will be the same as the current `ConfigLoader`
           
class DatabaseConfigLoader(AbstractConfigLoader):
    """ConfigLoader implementation for loading configuration from a database."""
    def load(self, connection_string):
          ...

class ConfigResolver:
    """Abstract base class for all ConfigResolver implementations."""

    def resolve(self) -> Dict[str, Any]:
        """Resolves the configuration data."""
        raise NotImplementedError

class JinjaConfigResolver(ConfigResolver):
    """ConfigResolver implementation for resolving Jinja templates."""

    def resolve(self) -> Dict[str, Any]:
        """Resolves the configuration data using Jinja templates."""
        # implementation here

Back to this PR specificially, I think I am not quite sure what's the constrains since @ankatiyar mention the refactor is necessary for the implementation.

Few comments:

• Moving logic from __getitem__ -> _get_config doesn't seem to be clearer to me.
• _get_conf_path looks good to me
• How different _get_filtered_conf_paths from _build_conf_path we used to have?

[ankatiyar]

@merelcht @noklam I've reverted the refactoring stuff, turns out it works fine without it. We can keep the scope of this PR just to adding the globals functionality.

[merelcht]

@merelcht @noklam I've reverted the refactoring stuff, turns out it works fine without it. We can keep the scope of this PR just to adding the globals functionality.

Okay cool, in that case let's indeed just keep this PR for adding the globals functionality and do refactoring separately. I think @noklam made a great point about our configuration loaders being more than just loaders and actually doing resolving as well. For the time being I wouldn't go as far as introducing the separation into two classes, but we can have another look at improving the methods and specifically the large load_and_merge_dir_config.

[ankatiyar]

Note to self: Change version depending on when this is merged.

[ankatiyar]

I've made some final changes + added docs and tests -

• Removed self.globals from __init__. Globals are loaded when the resolver _get_globals_value() is called by the "globals" resolver. This means that the globals are loaded multiple times but now if the users bypass configuration and set it in their settings.py, it will still work -

from kedro.config import ConfigLoader
conf_path = str(project_path / settings.CONF_SOURCE)
conf_loader = OmegaConfigLoader(conf_source=conf_path)
config_loader["globals"] = {"my_global" : 23 }

• Trying config_loader["globals"] should not return MissingConfigException but return an empty dict if there are no globals files.

• You can have nested global variables -
  globals.yml

nested:
  x: 23

Can be used in parameters/catalog etc with dot notation -

my_param: "${globals:nested.x}"

[merelcht]

This is looking so goooood! 👏 Great docs as well. I just left one comment about a test.

[merelcht]

Awesome work @ankatiyar ! 🤩 Don't forget to update the release notes!

[noklam]

        >>> # in settings.py
        >>> from kedro.config import TemplatedConfigLoader
        >>>
        >>> CONFIG_LOADER_CLASS = TemplatedConfigLoader
        >>> CONFIG_LOADER_ARGS = {
        >>>     "globals_pattern": "*globals.yml",
        >>> }

Suggested change

	"globals": ["globals*", "globals*/**", "**/globals*"],
	"globals": ["globals*"],

I suggest keep this pattern simple.

1. Align with the current TemplatedConfigLoader pattern, the above code block is copy from our template
2. Most people don't need nested globals, putting globals in folder should be minority, if so they should change the settings instead.
3. **/globals* is quite dangerous, as this could match parameters/globals_parameters.yml - although we usually suggest people using parameter_globals instead.

I may even suggest a more conservative default "globals": "globals.yml", forcing the default to be just globals.yml. If we change this after release it will be breaking change.

WDYT? @ankatiyar @merelcht , cc @stichbury because I really think we should separate the terminology for globals, it's being overloaded a lot now and this can lead to weird bugs.

[noklam]

Offline conversation - agree with using default with globals.yml. Cc @merelcht @ankatiyar

[noklam]

Excited to have this feature finally : ). Please include a release note to highlight this feature!

[ankatiyar]

Added a more helpful error message to address this problem - #2794 (comment)

We found that _ is not support for globals.yml. It will now give an clear message that you shouldn't use _ for globals at all.

Noted this is not because of the implementation of the resolver, but rather OmegaConfigLoader filter out everything with _. If it turns out it is needed we can always add it and it would not be breaking. (if we enable it now and change later , it will be breaking).

Essentially, if you have keys starting with '_' in globals.yml, it gets filtered out by the load_and_merge_dir_config() when loading the "globals". Then if you try to reference it in your parameters.yml or catalog.yml like - param1 : "${globals:_global_value}". It would just say "Globals key '_global_value' not found." Even if it does exist in the globals.yml.

[ankatiyar]

Also added the ability to provide a default value for globals resolver to fall back on if the key does not exist. + updated test and docs

[marrrcin]

Looks great, really clean implementation. Added 2 small questions.