Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix overwriting of nested parameters in config by runtime parameters #2378

Merged
merged 10 commits into from
Mar 2, 2023
Merged

Fix overwriting of nested parameters in config by runtime parameters #2378

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e45e1b8
Fix updating of nested params from CLI
ankatiyar Mar 1, 2023
f909b80
Merge branch 'main' into fix/update_nested_params
ankatiyar Mar 1, 2023
3cc7429
Convert DictConfig to dict for proper merging
ankatiyar Mar 2, 2023
20685c4
revert omegaconf
ankatiyar Mar 2, 2023
0230e66
Merge branch 'main' into fix/update_nested_params
ankatiyar Mar 2, 2023
9d65c97
revert utils indent
ankatiyar Mar 2, 2023
3b059d4
Merge branch 'fix/update_nested_params' of https://github.com/kedro-o…
ankatiyar Mar 2, 2023
74b7fa3
Test for nested params with omegaconf
ankatiyar Mar 2, 2023
99599ba
Add test for checking store does not contain DictConfig
ankatiyar Mar 2, 2023
4cc404e
docslinkcheck + move fn outside
ankatiyar Mar 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions kedro/framework/context/context.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a better way would be to check against the superclass of all dict-like objects to remove the strict dependency on omegaconf and make this future-proof.

from collections.abc import Mapping
from omegaconf import DictConfig

assert isinstance({}, Mapping) 
assert isinstance(DictConfig({1:1}), Mapping)

I would also double check for other occurrences of isinstance(x, dict) in the code

Copy link
Contributor

@antonymilne antonymilne Mar 2, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Definitely agree with this, thanks for the comment @tsanikgr. @ankatiyar, please could we do isinstance(..., Mapping) instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is creating trouble with mypy in _update_nested_dict. 😅

value, (dict, DictConfig)
):
_update_nested_dict(old_dict[key], value)
else:
old_dict[key] = value
Expand Down Expand Up @@ -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)

Expand Down