Moved the environment assumption from ConfigLoader to _ProjectSettings

[noklam]

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

Part of #2971. This enable the use of ConfigLoader as a standalone component while keeping existing behavior unchanged.

Development notes

• Update settings.py to declare the defaults explicitly
• Move the base and local assumption from ConfigLoader -> _ProjectSettings
• Add test to make sure standalone configloader work as expected.
• New ticket: update settings.py in starters - add default environment in settings kedro-starters#183
• New ticket: update docs - Update documentations of Configuration default environments #3321

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

• Read the contributing guidelines
• Signed off each commit with a Developer Certificate of Origin (DCO)
• 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
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[noklam]

The test was never updated after session is created. This is causing fail test because settings will not propagate correctly if we try to construct KedroContext directly.

[astrojuanlu]

I confirm that this works as expected from a usage point of view:

$ tree                                       (kedro38-dev) 
.
└── catalog.yml

1 directory, 1 file
$ ipython --no-banner

In [1]: from kedro.config import OmegaConfigLoader

In [2]: conf_loader = OmegaConfigLoader(conf_source=".")

In [3]: conf_loader["catalog"]
Out[3]: {'test_csv': {'type': 'pandas.CSVDataset', 'filepath': 'companies.csv'}}

Also did a quick check on an old spaceflights starter and nothing broke 👍🏽

[ankatiyar]

LGTM! Tried it locally. Do the docs need to be updated for this too?

[ankatiyar]

This needs to be reverted for code coverage I think

[noklam]

LGTM! Tried it locally. Do the docs need to be updated for this too?

@ankatiyar Good point, we can do that in a separate PR. I am not sure where would be the best place to do this, maybe https://docs.kedro.org/en/stable/notebooks_and_ipython/notebook-example/add_kedro_to_a_notebook.html?

Cc @stichbury What do you think? This related to the change of default arguments, thus make it easier to use ConfigLoader as a standalone component. Should we also update this in the Configuration page? The default of a Kedro project is unchanged (base and local), but it wouldn't be required if someone just import OmegaConfigLoader

I will fix the test coverage later since I want to wait for @merelcht review as well.

[stichbury]

Cc @stichbury What do you think? This related to the change of default arguments, thus make it easier to use ConfigLoader as a standalone component. Should we also update this in the Configuration page? The default of a Kedro project is unchanged (base and local), but it wouldn't be required if someone just import OmegaConfigLoader

Thanks for the catch @noklam -- yes, I think the best option is to update the configuration docs to highlight the change. Then illustrate its usage in the notebook example as @ankatiyar suggests.

[merelcht]

The implementation looks good to me! 👍 One thing I didn't immediately see was if we have test that explicitly tests different environments from base, local, and "". And if we have a test where we only set the default_run_env and not the base?

[noklam]

The implementation looks good to me! 👍 One thing I didn't immediately see was if we have test that explicitly tests different environments from base, local, and "". And if we have a test where we only set the default_run_env and not the base?

@merelcht I have added a test for the latter. One thing that I am not sure is the only "" case.

Assuming a structure like this and the parameters.yml has the same content.

--base/parameters.yml
--local/parameters.yml

You can either use both environments, or move everything to base. If you choose to keep this structure while setting no environment. You get "duplicate key error" because setting base_env="." is going to read both files due to this the default config pattern **/parameters* (match "local/parameters.yml" and "base/parameters.yml").

E ValueError: Duplicate keys found in /tmp/pytest-of-gitpod/pytest-5/test_load_config_only_default_0/local/parameters.yml and /tmp/pytest-of-gitpod/pytest-5/test_load_config_only_default_0/base/parameters.yml: dummy

We can avoid this by conditionally checking with something like

if base_env or default_run_env:
    self.config_patterns = {
                "catalog": ["catalog*", "catalog*/**", "**/catalog*"],
                "parameters": ["parameters*", "parameters*/**", "**/parameters*"],
                "credentials": ["credentials*", "credentials*/**", "**/credentials*"],
                "globals": ["globals.yml"],
            }
else:
    self.config_patterns = {
                "catalog": ["catalog*", "catalog*/**",],
                "parameters": ["parameters*", "parameters*/**"],
                "credentials": ["credentials*", "credentials*/**"],
                "globals": ["globals.yml"],
            }

It will avoid the problem but I don't like it because it is very special to this one case. Maybe what we can do is to enhance the error message, so it tells you which "env" is causing this error, additionally printing the related "config_pattern"?

[merelcht]

The implementation looks good to me! 👍 One thing I didn't immediately see was if we have test that explicitly tests different environments from base, local, and "". And if we have a test where we only set the default_run_env and not the base?

@merelcht I have added a test for the latter. One thing that I am not sure is the only "" case.

Aahh sorry reading my own comment again, I realise it's not very clear 😅 What I meant to say, is that I'm not sure if there's a test which checks if this all works when you have a random environment, so base_env="my_env" and default_run_env="my_other_env". I guess this isn't actually different from "base" and "local", but might be good to have an explicit test.

[noklam]

Aahh sorry reading my own comment again, I realise it's not very clear 😅 What I meant to say, is that I'm not sure if there's a test which checks if this all works when you have a random environment, so base_env="my_env" and default_run_env="my_other_env". I guess this isn't actually different from "base" and "local", but might be good to have an explicit test.

@merelcht Ah, I see what you mean now. I have updated the original tests test_load_config_only_default_run_environment and test_load_config_only_base_environment for this.

[merelcht]

Great work! 👍