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

Add a new configloader with OmegaConf as replacement for anyconf #1868

Closed
merelcht opened this issue Sep 23, 2022 · 3 comments · Fixed by #2085
Closed

Add a new configloader with OmegaConf as replacement for anyconf #1868

merelcht opened this issue Sep 23, 2022 · 3 comments · Fixed by #2085
Assignees

Comments

@merelcht
Copy link
Member

merelcht commented Sep 23, 2022

Introduction

As part of the work on improving configuration in Kedro we assessed alternatives for anyconfig. anyconfig was initially chosen because it supports reading different configuration formats. In practice, most users use yml files and so we want to use an alternative library that offers better functionality for yml configuration.

In #1657 it was decided to use OmegaConf as this replacement. More info about that conclusion can be found here: #1657 (comment)

We also did research to verify what config file formats users use in #1703

Implementation

In order to introduce this change in a non-breaking way we have to do the following:

  1. Add a new configloader
  2. Add OmegaConf as replacement for anyconf in this new configloader.
  3. Turn off all built-in resolvers in OmegaConf in ConfigLoader. In the first iteration we want to keep these advanced features turned off https://omegaconf.readthedocs.io/en/latest/custom_resolvers.html#clear-resolver. It's easier to introduce them step by step, but removing them would be a breaking change. In Allow users to pass credentials through environment variables #1909 we will introduce environment variables for credentials.
@szczeles
Copy link
Contributor

@MerelTheisenQB Do you know if OmegaConf implementation will keep the current configs merging behaviour? The challenge I've seen in the implementations is that top-level keys are overridden. For example having a conf/base/mlflow.yml like:

tracking:
  disable_tracking:
    pipelines:
    - on_exit_notification

  experiment:
    name: name-of-local-experiment

  params:
    long_params_strategy: tag

if I wanted to just change the experiment name, then this config located in conf/prod/mlflow.yml:

tracking:
  experiment:
    name: name-of-prod-experiment

overrides long_params_strategy and disable_tracking to the default values (in this case: fail and []) - inheritance doesn't work here. Basically, to inherit from base, I need to duplicate all the custom values.

In contrast, Helm package manager merges the configuration to the lowest level of the YAML tree, allowing to adjustment of only the necessary values. It would be great to be able to achieve this in Kedro too, but this change would be incompatible with the previous behavior.

@merelcht merelcht changed the title Add OmegaConf as replacement for anyconf in ConfigLoader Add a new configloader with OmegaConf as replacement for anyconf Oct 20, 2022
@merelcht
Copy link
Member Author

@MerelTheisenQB Do you know if OmegaConf implementation will keep the current configs merging behaviour? The challenge I've seen in the implementations is that top-level keys are overridden.

OmegaConf has a different merge strategy and will only replace the specified fields. So in your example if you merge the two files you'd end up with:

tracking:
  disable_tracking:
    pipelines:
    - on_exit_notification
  experiment:
    name: name-of-prod-experiment
  params:
    long_params_strategy: tag

See the OmegaConf docs for an example snippet you can run: https://omegaconf.readthedocs.io/en/2.2_branch/usage.html#merging-configurations

@szczeles
Copy link
Contributor

@MerelTheisenQB Fantastic! Thanks a lot for the confirmation

@merelcht merelcht self-assigned this Nov 29, 2022
@merelcht merelcht moved this from Delivery - Later 🔧 to Delivery - Now ⌛ in Roadmap Nov 29, 2022
@merelcht merelcht mentioned this issue Nov 30, 2022
5 tasks
@merelcht merelcht linked a pull request Dec 5, 2022 that will close this issue
5 tasks
@merelcht merelcht moved this to To Do in Kedro Framework Dec 5, 2022
@merelcht merelcht moved this from To Do to In Review in Kedro Framework Dec 5, 2022
@merelcht merelcht moved this from In Review to In Progress in Kedro Framework Dec 8, 2022
@merelcht merelcht moved this from In Progress to In Review in Kedro Framework Dec 12, 2022
Repository owner moved this from In Review to Done in Kedro Framework Dec 20, 2022
@yetudada yetudada moved this from Delivery - Now ⌛ to Shipped 🚀 in Roadmap Feb 23, 2023
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
Status: Shipped 🚀
Development

Successfully merging a pull request may close this issue.

2 participants