From e45e1b85c441b7e9ceef1cc56c50b54ba5429a9a Mon Sep 17 00:00:00 2001 From: Ankita Katiyar Date: Wed, 1 Mar 2023 16:07:06 +0000 Subject: [PATCH 1/7] Fix updating of nested params from CLI Signed-off-by: Ankita Katiyar --- kedro/framework/context/context.py | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/kedro/framework/context/context.py b/kedro/framework/context/context.py index c76ed2ed87..75a782b48b 100644 --- a/kedro/framework/context/context.py +++ b/kedro/framework/context/context.py @@ -7,6 +7,7 @@ from urllib.parse import urlparse from warnings import warn +from omegaconf import DictConfig from pluggy import PluginManager from kedro.config import ConfigLoader, MissingConfigException @@ -154,7 +155,9 @@ def _update_nested_dict(old_dict: Dict[Any, Any], new_dict: Dict[Any, Any]) -> N if key not in old_dict: old_dict[key] = value else: - if isinstance(old_dict[key], dict) and isinstance(value, dict): + if isinstance(old_dict[key], (dict, DictConfig)) and isinstance( + value, (dict, DictConfig) + ): _update_nested_dict(old_dict[key], value) else: old_dict[key] = value @@ -322,8 +325,7 @@ def _add_param_to_feed_dict(param_name, param_value): """ key = f"params:{param_name}" feed_dict[key] = param_value - - if isinstance(param_value, dict): + if isinstance(param_value, (dict, DictConfig)): for key, val in param_value.items(): _add_param_to_feed_dict(f"{param_name}.{key}", val) From 3cc74290c3dc997e3c7bc06fdbd0446ca0425f68 Mon Sep 17 00:00:00 2001 From: Ankita Katiyar Date: Thu, 2 Mar 2023 12:19:53 +0000 Subject: [PATCH 2/7] Convert DictConfig to dict for proper merging Signed-off-by: Ankita Katiyar --- kedro/config/omegaconf_config.py | 2 +- kedro/framework/cli/utils.py | 4 ++-- kedro/framework/context/context.py | 7 ++----- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/kedro/config/omegaconf_config.py b/kedro/config/omegaconf_config.py index ebd09ad23b..cbb4e0596f 100644 --- a/kedro/config/omegaconf_config.py +++ b/kedro/config/omegaconf_config.py @@ -250,7 +250,7 @@ def load_and_merge_dir_config( return {} if len(aggregate_config) == 1: return list(aggregate_config)[0] - return dict(OmegaConf.merge(*aggregate_config)) + return OmegaConf.to_container(OmegaConf.merge(*aggregate_config)) @staticmethod def _is_valid_config_path(path): diff --git a/kedro/framework/cli/utils.py b/kedro/framework/cli/utils.py index 4105a1e9f0..5218dec4ac 100644 --- a/kedro/framework/cli/utils.py +++ b/kedro/framework/cli/utils.py @@ -468,8 +468,8 @@ def _split_params(ctx, param, value): f"cannot be an empty string." ) dot_list.append(item) - conf = OmegaConf.from_dotlist(dot_list) - return conf + conf = OmegaConf.from_dotlist(dot_list) + return OmegaConf.to_container(conf) def _split_load_versions(ctx, param, value): diff --git a/kedro/framework/context/context.py b/kedro/framework/context/context.py index 75a782b48b..81bb51a887 100644 --- a/kedro/framework/context/context.py +++ b/kedro/framework/context/context.py @@ -7,7 +7,6 @@ from urllib.parse import urlparse from warnings import warn -from omegaconf import DictConfig from pluggy import PluginManager from kedro.config import ConfigLoader, MissingConfigException @@ -155,9 +154,7 @@ def _update_nested_dict(old_dict: Dict[Any, Any], new_dict: Dict[Any, Any]) -> N if key not in old_dict: old_dict[key] = value else: - if isinstance(old_dict[key], (dict, DictConfig)) and isinstance( - value, (dict, DictConfig) - ): + if isinstance(old_dict[key], dict) and isinstance(value, dict): _update_nested_dict(old_dict[key], value) else: old_dict[key] = value @@ -325,7 +322,7 @@ def _add_param_to_feed_dict(param_name, param_value): """ key = f"params:{param_name}" feed_dict[key] = param_value - if isinstance(param_value, (dict, DictConfig)): + if isinstance(param_value, dict): for key, val in param_value.items(): _add_param_to_feed_dict(f"{param_name}.{key}", val) From 20685c488abb4fc74bf620bef0b19b7a0d0fe23d Mon Sep 17 00:00:00 2001 From: Ankita Katiyar Date: Thu, 2 Mar 2023 13:46:02 +0000 Subject: [PATCH 3/7] revert omegaconf Signed-off-by: Ankita Katiyar --- kedro/config/omegaconf_config.py | 2 +- kedro/framework/context/context.py | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/kedro/config/omegaconf_config.py b/kedro/config/omegaconf_config.py index cbb4e0596f..ebd09ad23b 100644 --- a/kedro/config/omegaconf_config.py +++ b/kedro/config/omegaconf_config.py @@ -250,7 +250,7 @@ def load_and_merge_dir_config( return {} if len(aggregate_config) == 1: return list(aggregate_config)[0] - return OmegaConf.to_container(OmegaConf.merge(*aggregate_config)) + return dict(OmegaConf.merge(*aggregate_config)) @staticmethod def _is_valid_config_path(path): diff --git a/kedro/framework/context/context.py b/kedro/framework/context/context.py index 81bb51a887..75a782b48b 100644 --- a/kedro/framework/context/context.py +++ b/kedro/framework/context/context.py @@ -7,6 +7,7 @@ from urllib.parse import urlparse from warnings import warn +from omegaconf import DictConfig from pluggy import PluginManager from kedro.config import ConfigLoader, MissingConfigException @@ -154,7 +155,9 @@ def _update_nested_dict(old_dict: Dict[Any, Any], new_dict: Dict[Any, Any]) -> N if key not in old_dict: old_dict[key] = value else: - if isinstance(old_dict[key], dict) and isinstance(value, dict): + if isinstance(old_dict[key], (dict, DictConfig)) and isinstance( + value, (dict, DictConfig) + ): _update_nested_dict(old_dict[key], value) else: old_dict[key] = value @@ -322,7 +325,7 @@ def _add_param_to_feed_dict(param_name, param_value): """ key = f"params:{param_name}" feed_dict[key] = param_value - if isinstance(param_value, dict): + if isinstance(param_value, (dict, DictConfig)): for key, val in param_value.items(): _add_param_to_feed_dict(f"{param_name}.{key}", val) From 9d65c97fc9f87d8044d118903310e08bfd8f92f1 Mon Sep 17 00:00:00 2001 From: Ankita Katiyar Date: Thu, 2 Mar 2023 13:49:28 +0000 Subject: [PATCH 4/7] revert utils indent Signed-off-by: Ankita Katiyar --- kedro/framework/cli/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/kedro/framework/cli/utils.py b/kedro/framework/cli/utils.py index 5218dec4ac..ab24378870 100644 --- a/kedro/framework/cli/utils.py +++ b/kedro/framework/cli/utils.py @@ -468,7 +468,7 @@ def _split_params(ctx, param, value): f"cannot be an empty string." ) dot_list.append(item) - conf = OmegaConf.from_dotlist(dot_list) + conf = OmegaConf.from_dotlist(dot_list) return OmegaConf.to_container(conf) From 74b7fa3a0dcca08b748c78f0ae9c4dbd807ac2cb Mon Sep 17 00:00:00 2001 From: Ankita Katiyar Date: Thu, 2 Mar 2023 15:01:00 +0000 Subject: [PATCH 5/7] Test for nested params with omegaconf Signed-off-by: Ankita Katiyar --- tests/framework/context/test_context.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/framework/context/test_context.py b/tests/framework/context/test_context.py index 08bf0f2014..b9df3dce22 100644 --- a/tests/framework/context/test_context.py +++ b/tests/framework/context/test_context.py @@ -10,6 +10,7 @@ import pytest import toml import yaml +from omegaconf import OmegaConf from pandas.util.testing import assert_frame_equal from kedro import __version__ as kedro_version @@ -163,7 +164,6 @@ def dummy_dataframe(): ' --from-nodes "nodes3"' ) - expected_message_head = ( "There are 4 nodes that have not run.\n" "You can resume the pipeline run by adding the following " @@ -484,6 +484,11 @@ def test_validate_layers_error(layers, conflicting_datasets, mocker): {"a": {"a.c": {"a.c.b": 4}}}, {"a": {"a.a": 1, "a.b": 2, "a.c": {"a.c.a": 3, "a.c.b": 4}}}, ), + ( + {"a": OmegaConf.create({"b": 1}), "x": 3}, + {"a": {"c": 2}}, + {"a": {"b": 1, "c": 2}, "x": 3}, + ), ], ) def test_update_nested_dict(old_dict: Dict, new_dict: Dict, expected: Dict): From 99599ba41a83c0023d79c77db1196b79a288c784 Mon Sep 17 00:00:00 2001 From: Ankita Katiyar Date: Thu, 2 Mar 2023 15:36:13 +0000 Subject: [PATCH 6/7] Add test for checking store does not contain DictConfig Signed-off-by: Ankita Katiyar --- tests/framework/session/test_session.py | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/tests/framework/session/test_session.py b/tests/framework/session/test_session.py index 1cccce0426..e5a2d9cf6f 100644 --- a/tests/framework/session/test_session.py +++ b/tests/framework/session/test_session.py @@ -2,14 +2,17 @@ import re import subprocess import textwrap +from collections.abc import Mapping from pathlib import Path import pytest import toml import yaml +from omegaconf import OmegaConf from kedro import __version__ as kedro_version from kedro.config import AbstractConfigLoader, ConfigLoader, OmegaConfigLoader +from kedro.framework.cli.utils import _split_params from kedro.framework.context import KedroContext from kedro.framework.project import ( ValidationError, @@ -920,3 +923,25 @@ def test_setup_logging_using_omega_config_loader_class( ).as_posix() actual_log_filepath = call_args["handlers"]["info_file_handler"]["filename"] assert actual_log_filepath == expected_log_filepath + + +@pytest.mark.parametrize("params", ["a=1,b.c=2", "a=1,b=2,c=3"]) +def test_no_DictConfig_in_store( + params, + mock_package_name, + fake_project, +): + extra_params = _split_params(None, None, params) + session = KedroSession.create( + mock_package_name, fake_project, extra_params=extra_params + ) + + def get_all_values(mapping: Mapping): + for value in mapping.values(): + yield value + if isinstance(value, Mapping): + yield from get_all_values(value) + + assert not any( + OmegaConf.is_config(value) for value in get_all_values(session._store) + ) From 4cc404e3c64f3503cb74cf5228e9499d99f151ca Mon Sep 17 00:00:00 2001 From: Ankita Katiyar Date: Thu, 2 Mar 2023 17:13:56 +0000 Subject: [PATCH 7/7] docslinkcheck + move fn outside Signed-off-by: Ankita Katiyar --- docs/source/development/commands_reference.md | 2 +- docs/source/development/linting.md | 2 +- tests/framework/session/test_session.py | 15 ++++++++------- 3 files changed, 10 insertions(+), 9 deletions(-) diff --git a/docs/source/development/commands_reference.md b/docs/source/development/commands_reference.md index 9c80fc0ed0..64792e4a1e 100644 --- a/docs/source/development/commands_reference.md +++ b/docs/source/development/commands_reference.md @@ -408,7 +408,7 @@ _This command will be deprecated from Kedro version 0.19.0._ kedro lint ``` -Your project is linted with [`black`](https://github.com/psf/black), [`flake8`](https://gitlab.com/pycqa/flake8) and [`isort`](https://github.com/PyCQA/isort). +Your project is linted with [`black`](https://github.com/psf/black), [`flake8`](https://github.com/PyCQA/flake8) and [`isort`](https://github.com/PyCQA/isort). #### Test your project diff --git a/docs/source/development/linting.md b/docs/source/development/linting.md index 545cf679e2..240f3d30c1 100644 --- a/docs/source/development/linting.md +++ b/docs/source/development/linting.md @@ -8,7 +8,7 @@ consistent. ## Set up linting tools There are a variety of linting tools available to use with your Kedro projects. This guide shows you how to use -[`black`](https://github.com/psf/black), [`flake8`](https://gitlab.com/pycqa/flake8), and +[`black`](https://github.com/psf/black), [`flake8`](https://github.com/PyCQA/flake8), and [`isort`](https://github.com/PyCQA/isort) to lint your Kedro projects. - **`black`** is a [PEP 8](https://peps.python.org/pep-0008/) compliant opinionated Python code formatter. `black` can check for styling inconsistencies and reformat your files in place. diff --git a/tests/framework/session/test_session.py b/tests/framework/session/test_session.py index e5a2d9cf6f..bb014baafa 100644 --- a/tests/framework/session/test_session.py +++ b/tests/framework/session/test_session.py @@ -925,7 +925,14 @@ def test_setup_logging_using_omega_config_loader_class( assert actual_log_filepath == expected_log_filepath -@pytest.mark.parametrize("params", ["a=1,b.c=2", "a=1,b=2,c=3"]) +def get_all_values(mapping: Mapping): + for value in mapping.values(): + yield value + if isinstance(value, Mapping): + yield from get_all_values(value) + + +@pytest.mark.parametrize("params", ["a=1,b.c=2", "a=1,b=2,c=3", ""]) def test_no_DictConfig_in_store( params, mock_package_name, @@ -936,12 +943,6 @@ def test_no_DictConfig_in_store( mock_package_name, fake_project, extra_params=extra_params ) - def get_all_values(mapping: Mapping): - for value in mapping.values(): - yield value - if isinstance(value, Mapping): - yield from get_all_values(value) - assert not any( OmegaConf.is_config(value) for value in get_all_values(session._store) )