Fix overwriting of nested parameters in config by runtime parameters

[ankatiyar]

Description

Resolves #2357

The bug

Nested params

If the config files already contained a nested parameter -

param1 : 5
foo : 
  bar : 1

And you do kedro run --params foo.baz=2, this would completely overwrite the foo parameter at runtime.
The parameters at runtime are then -

param1 : 5
foo: 
   baz : 2

Instead of

param1 : 5
foo: 
   bar : 1
   baz : 2

Kedro viz bug

The runtime params being passed as DictConfig instead of a python dictionary is also causing problems with Kedro Viz because it's not json-serialisable. This is breaking experiment tracking.

Development notes

• This is because omegaconf loads the runtime parameters as a DictConfig object instead of a dict. In case of nested dicts, it also loads the nested parameter as a DictConfig object which was causing the conditions in the changed lines to return False.
• Manually tested this solution works with both ConfigLoader as well as the OmegaConfigLoader.
• when the OmegaConfigLoader is used instead of ConfigLoader, it also loads the nested params as DictConfig. DictConfig key-value pairs can be accessed in a similar way to dict so I think this solution should suffice.

Update :

• We decided to convert runtime params from DictConfig to dict right when they're read. -> This solves the Kedro-viz issue
• Didn't change the way OmegaConfigLoader reads parameters from conf for now, while merging allow both DictConfig and dict types. -> Solves the nested params issue

Checklist

• Read the contributing guidelines
• 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

[tsanikgr]

Thank you for the PR :) A suggestion below!

[tsanikgr]

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

[antonymilne]

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

[ankatiyar]

This is creating trouble with mypy in _update_nested_dict. 😅

[rashidakanchwala]

I tested it and it works on viz. thanks

[antonymilne]

Suggested change

	@pytest.mark.parametrize("params", ["a=1,b.c=2", "a=1,b=2,c=3"])
	@pytest.mark.parametrize("params", ["a=1,b.c=2", "a=1,b=2,c=3", ""])

Would be good to test the case that no params are passed here also - does adding it as a test case work?

[ankatiyar]

Yeah this would work, added it to the test!

[antonymilne]

I think we should define this outside the test function 🙂

[ankatiyar]

Done!

[antonymilne]

Left a few comments but basically looks good, thank you for the quick fix Ankita! ⭐

Just to check that the tests are working as they should: do both test_no_DictConfig_in_store and the new cases for test_update_nested_dict fail on main currently?

[ankatiyar]

Just to check that the tests are working as they should: do both test_no_DictConfig_in_store and the new cases for test_update_nested_dict fail on main currently?

Thanks Antony! Yeah, I checked and both the tests are currently failing on main without this fix.