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

Remove custom Kedro syntax for --params #1965

Closed
merelcht opened this issue Oct 24, 2022 · 3 comments
Closed

Remove custom Kedro syntax for --params #1965

merelcht opened this issue Oct 24, 2022 · 3 comments
Assignees

Comments

@merelcht
Copy link
Member

merelcht commented Oct 24, 2022

Description

Follow up to #1907

To-do:

  • Remove the support of Kedro custom syntax
    • kedro run
    • reload_kedro magic

kedro run --params=key:value (The Kedro syntax for params)

Context

In 0.19.0 we will remove the Kedro syntax for providing parameters and only use OmegaConf syntax.

@merelcht merelcht added the Issue: Feature Request New feature or improvement to existing feature label Oct 24, 2022
@merelcht merelcht added this to the Configuration overhaul milestone Oct 24, 2022
@merelcht merelcht removed the Issue: Feature Request New feature or improvement to existing feature label Oct 24, 2022
@ankatiyar
Copy link
Contributor

Following #2378 -

  • The output for _split_params function in kedro/framework/cli/utils.py has changed from omegaconf.DictConfig type to dict. This should ideally be changed back to DictConfig so omegaconf can be used to merge runtime params with parameters from config files.
  • We need to be careful with merging nested parameters (see [BUG] Passing runtime nested params to kedro run --params overwrites params from config files in latest release #2357) - this is happening right now in _update_nested_dict() in context.py
    • ConfigLoader loads the parameters and the nested params from config files as a dict type.
    • OmegaConfigLoader loads config params as a dict right now but the nested parameters are of the type DictConfig
  • Using DictConfig was causing issues with Kedro Viz because the DictConfig type is not json-serialisable. The runtime parameters still need to be converted to dict type before going in the session._store to not break experiment tracking.

@antonymilne
Copy link
Contributor

Just to add to the above, I think what we want is:

  • _split_params should be extremely simple, just doing OmegaConf.from_dotlist really
  • _update_nested_dict should disappear altogether and just use OmegaConf merging functionality, nothing custom

As part of a separate ticket, we should probably also be doing something about what the OmegaConfigLoader.load_and_merge_dir_config returns. This was changed in #2176 but I don't understand why we're using dict rather than just doing this:

OmegaConf.to_container(OmegaConf.merge(*aggregate_config), resolve=True)

If we changed this then we could also remove the special to_container behaviour we have on the logging config.

Ultimately, the config loader should probably be returning a pure Python dictionary and nothing OmegaConf specific, otherwise it's a leaky abstraction as it is now.

With that said, I don't know what the ultimate plan is about how we do the parameters merging in context.params. I assume we can use OmegaConf as a dependency here but don't actually want to rely on the fact that someone is using OmegaConfigLoader. So to keep it independent of config loader implementation I guess we'd do:

  • _split_params does OmegaConf.from_dotlist and then to_container to return a dictionary
  • OmegaConfigLoader.load_and_merge_dir_config does to_container to return a dictionary
  • in context.params, we turn these dictionaries back into OmegaConf objects in order to merge them together

It feels kind of circular doing this but I'm not sure whether there's a better way to do it since we need something that works independently of which configloader implementation you use - did you have a plan here @merelcht?

@SajidAlamQB
Copy link
Contributor

Completed in #3290

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

4 participants