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.

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

OmegaConfigLoader returns Dict instead of DictConfig, resolves runtime_params properly #2467

Merged
merged 25 commits into from
May 11, 2023
Merged
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
4565684
Merge branch 'main' of github.com:kedro-org/kedro
noklam Feb 10, 2023
67cff2d
Merge branch 'main' of github.com:kedro-org/kedro
noklam Feb 21, 2023
055124e
Merge branch 'main' of github.com:kedro-org/kedro
noklam Mar 22, 2023
842e83c
Merge branch 'main' of github.com:kedro-org/kedro
noklam Mar 23, 2023
d6d224a
Fix typehint
noklam Mar 24, 2023
78c1ae0
test push
noklam Mar 27, 2023
5b48003
Merge branch 'main' of github.com:kedro-org/kedro into fix-omegaconfig
noklam Mar 29, 2023
5b0bff8
POC of fix to solve the runtime param resolution problem
noklam Mar 29, 2023
c864783
Merge branch 'main' of github.com:kedro-org/kedro into fix-omegaconfig
noklam May 2, 2023
2c9da40
Fix OmegaConfigLoadaer - resolve runtime_params early
noklam May 2, 2023
f578c39
Delegate the intialization of runtime_params to AbstractConfigLoader
noklam May 2, 2023
de98e15
Add test for interpolated value and globals
noklam May 2, 2023
4c0571d
add more test and linting
noklam May 2, 2023
f1c1ec2
refactor
noklam May 2, 2023
acbde3e
update release note
noklam May 2, 2023
87a51b6
Apply comments and refactor the test
noklam May 4, 2023
a1b5f0c
Update RELEASE.md
noklam May 4, 2023
29f62ff
Remove unnecessary condition when len(config) == 1
noklam May 5, 2023
f610010
Update release note
noklam May 5, 2023
5799792
Merge branch 'main' into fix-omegaconfig
noklam May 5, 2023
78c2371
Merge branch 'main' into fix-omegaconfig
noklam May 10, 2023
e24fbfe
Merge branch 'fix-omegaconfig' of github.com:kedro-org/kedro into fix…
noklam May 10, 2023
0f7911f
Remove unused import
noklam May 10, 2023
e7d6f62
remove a line from coverage temporarily
noklam May 11, 2023
a4d9893
Merge branch 'main' into fix-omegaconfig
noklam May 11, 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
6 changes: 5 additions & 1 deletion kedro/config/omegaconf_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ def __init__(
env=env,
runtime_params=runtime_params,
)
if not self.runtime_params:
self.runtime_params = {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably nicer to do runtime_params=runtime_params or {} above instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even better AbstractConfigLoader should actually be the class responsible for this defaulting behaviour, i.e. it should do self.runtime_params = runtime_params or {}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

100% agree with this.


def __getitem__(self, key) -> Dict[str, Any]:
"""Get configuration files by key, load and merge them, and
Expand Down Expand Up @@ -275,7 +277,9 @@ 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, self.runtime_params), resolve=True
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this definitely work when self.runtime_params is a dictionary and not a DictConfig? i.e. is it ok to not do OmegaConf.create(runtime_params) here? Looking through omegaconf source I'm pretty sure this is fine (only the first argument of OmegaConf.merge needs to be DictConfig) but please do check this!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked with the source code, merge can take any primitive type I think it's fine here.

    def merge(
        *configs: Union[
            DictConfig,
            ListConfig,
            Dict[DictKeyType, Any],
            List[Any],
            Tuple[Any, ...],
            Any,
        ]

)

def _is_valid_config_path(self, path):
"""Check if given path is a file path and file type is yaml or json."""
Expand Down