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

[BUG] Passing runtime nested params to kedro run --params overwrites params from config files in latest release #2357

Closed
tsanikgr opened this issue Feb 23, 2023 · 2 comments · Fixed by #2378
Assignees
Labels
Issue: Bug Report 🐞 Bug that needs to be fixed

Comments

@tsanikgr
Copy link
Contributor

tsanikgr commented Feb 23, 2023

Hello team - nice to be back in these discussions :)

Description

There was a regression in the latest release (0.18.5), which introduced OmegaConf for parsing parameters provided to the kedro run --params.

More specifically, in previous versions

kedro run --params foo.bar=a

was properly merged with an existing parameter {"foo": {"baz": 1}}.

Now, kedro.framework.cli.hooks.utils._split_params returns an omegaconf.DictConfig object (rather than a plain dictionary), and the second part of the if clause here returns false:

if isinstance(old_dict[key], dict) and isinstance(value, dict):
    _update_nested_dict(old_dict[key], value)

Which means that after merging we get

{"foo": {"bar": a}}

instead of the expected

{"foo": {"bar": a, "baz": 1}}

I could open a PR to fix but it ll take way longer that letting you fix it :)
In the meantime I 've got a local workaround by subclassing KedroContext and casting params to a dict!

@tsanikgr tsanikgr changed the title Passing runtime nested params to kedro run --params overwrites params from config files in latest release [BUG] Passing runtime nested params to kedro run --params overwrites params from config files in latest release Feb 23, 2023
@ankatiyar ankatiyar self-assigned this Feb 23, 2023
@tejaassolanki
Copy link

tejaassolanki commented Feb 24, 2023

This is also a problem at the line here
The run time params provided through
kedro run —params
is/are an instance of omegaconf.dictconfig.DictConfig even when the configloader is not set to OmegaConf.

Thus, the run time params are not added/merged with the catalog which subsequently fails to override the same from a parameters file and also causes failure of the pipeline because it is not able to get those parameters in the catalog.

This is my first ever comment on an open source project. Please feel free to correct me.

@merelcht
Copy link
Member

Hello @tsanikgr nice to see you on here! Even if it's to report a bug 😂 Thanks for reporting this and also thank you @tejaassolanki to report your findings. We will have a look at this asap!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Bug Report 🐞 Bug that needs to be fixed
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants