Clarify in docs how runtime parameter resolution works

[merelcht]

Description

Closes #3456

Development notes

The problem is that when runtime parameters are provided, they get merged with both the config loaded from the base environment as well as the run environment. So they are duplicated and because of the destructive merge happening by default this can end up removing values present in base config and not in the runtime config.

Solutions tried:

1. Only merge runtime_params into base config and not in all loaded configs: this meets the requirements for point 1 and 3 above, but not 2.
2. Move the merging of runtime_params to after both base and default_run_env config have been loaded. This meets requirements for 1 and 2, but not 3.

Proposed "solution"

not change anything. All the behaviours can be met with the current implementation in place, but to meet the expectation described in #3456 the user has to specify soft merging for parameters to ensure the default_run_env doesn't destructively overwrite (nested) parameters in base config.

Developer Certificate of Origin

We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a Signed-off-by line in the commit message. See our wiki for guidance.

If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.

Checklist

• Read the contributing guidelines
• Signed off each commit with a Developer Certificate of Origin (DCO)
• 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
• Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

[noklam]

I found it difficult to understand conceptually that we only include runtime_params for base_env but not the others. I think this may be buggy:

I try this with an example:

# base
model_options:
  test_size: 0.2
  random_state: 3
  features:
    - engines

# local
model_options:
  test_size: 0.2
  random_state: 3
  features:
    - engines
nok: 1

%reload_kedro --params nok=3
config = context.config_loader
config["parameters"]

The expexted parameters should give me nok=3, but instead I get nok=1. If I remove nok:1 from local then I get nok=3 again.

[merelcht]

@noklam you're right. Let me dig some more!

[merelcht]

This issue is more complicated than it seemed at first. The current behaviour is as follows:

• base config gets loaded and any runtime_params get merged into this (with soft-merge).
• Then default_run_env config gets loaded and any runtime_params get merged into this (with soft-merge).
• Then by default base and default_run_env get merged with a destructive merge, meaning any duplicate keys in base and default_run_env will be overwritten with the values in default_run_env.

I've added some tests to highlight what's going on and what the desired behaviour is:

1. If a (nested) parameter exists in the base config but not in the default_run_env config and needs to be overwritten at runtime through runtime_params only the specified parameter should be updated and not the full entry replaced, so don't destructively merge. (see test: test_runtime_params_resolution_with_env + more context Fix the overwriting of nested parameters with OmegaConfigLoader #3456 )
2. If a (nested) parameter exists in the default_run_env but not in the base config and needs to be overwritted at runtime through runtime_params, we expect to see the updated runtime value. (see test: test_runtime_params_resolution_with_env2 + Nok's comment)
3. Any interpolated values are overwritten by runtime_params when provided (see test: test_runtime_params_override_interpolated_value)

Solutions tried:

1. Only merge runtime_params into base config and not in all loaded configs: this meets the requirements for point 1 and 3 above, but not 2.
2. Move the merging of runtime_params to after both base and default_run_env config have been loaded. This meets requirements for 1 and 2, but not 3.

Proposed "solution": not change anything. All the behaviours can be met with the current implementation in place, but to meet the first expectation the user has to specify soft merging for parameters to ensure the default_run_env doesn't destructively overwrite (nested) parameters in base config.

[ElenaKhaustova]

What if we just merge base and default_run_env first and then merge runtime_params once? From what I got, the problem is that base and default_run_env have different priorities when merging, and merging runtime_params to each leads to losing some parameters from runtime_params. So, if we merge runtime_params on top user can always define how to do the last (runtime_params) merge, whether they override the result of the previous merge or not.

[DimedS]

Thanks for digging into that so thoroughly, @merelcht! From what I understand, the desired behavior is achievable, but we need to clearly document it for the users, right?

[merelcht]

What if we just merge base and default_run_env first and then merge runtime_params once? From what I got, the problem is that base and default_run_env have different priorities when merging, and merging runtime_params to each leads to losing some parameters from runtime_params. So, if we merge runtime_params on top user can always define how to do the last (runtime_params) merge, whether they override the result of the previous merge or not.

@ElenaKhaustova I thought the same and tried this, but then this part doesn't work anymore:

3. Any interpolated values are overwritten by runtime_params when provided (see test: test_runtime_params_override_interpolated_value)

[ElenaKhaustova]

What if we just merge base and default_run_env first and then merge runtime_params once? From what I got, the problem is that base and default_run_env have different priorities when merging, and merging runtime_params to each leads to losing some parameters from runtime_params. So, if we merge runtime_params on top user can always define how to do the last (runtime_params) merge, whether they override the result of the previous merge or not.

@ElenaKhaustova I thought the same and tried this, but then this part doesn't work anymore:

3. Any interpolated values are overwritten by runtime_params when provided (see test: test_runtime_params_override_interpolated_value)

Then, it looks like there's no easy way to fix that without refactoring the logic. So, just explaining how to achieve the desired behaviour looks good, though it's not easy cause the examples are quite niche and require an understanding of how resolution works from the user side. We can keep an eye on this case in if we decide to refactor this part to avoid corner cases because it looks quite complex now.

[DimedS]

Thank you, @merelcht! I'm not sure, but perhaps including an example to the docs similar to the one you provided in the tests would help to better illustrate the concept.

[merelcht]

Thank you, @merelcht! I'm not sure, but perhaps including an example to the docs similar to the one you provided in the tests would help to better illustrate the concept.

Done! Let me know if this is what you expected.

[astrojuanlu]

💯

[DimedS]

Thank you, @merelcht! I'm not sure, but perhaps including an example to the docs similar to the one you provided in the tests would help to better illustrate the concept.

Done! Let me know if this is what you expected.

super, thank you!