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

Separate params to runtime_params and meta_params #2530

Open
noklam opened this issue Apr 19, 2023 · 2 comments
Open

Separate params to runtime_params and meta_params #2530

noklam opened this issue Apr 19, 2023 · 2 comments
Labels
Issue: Feature Request New feature or improvement to existing feature

Comments

@noklam
Copy link
Contributor

noklam commented Apr 19, 2023

Description

Is your feature request related to a problem? A clear and concise description of what the problem is: "I'm always frustrated when ..."

As part of the discussion of OmegaConfigLoader and a draft PR #2504, we want to separate the parameters into two arguments.

params is reserved for parameters only and it should override parameters.
meta_params , or alternatively meta_config

Context

Why is this change important to you? How would you use it? How can it benefit other users?

This should work for both TemplatedConfigLoader and OmegaConfigLoader

  1. For standard user: It will be clear that overriding an existing parameters versus any templated variable(TemplateConfigLoader) or interpolated value (OmegaConfigLoader)
  2. Internal / advance user: For people who need to extend their own ConfigLoader class, they should use the meta_params to compile the parameters.
  3. Additionally, this further decouple ConfigLoader from Kedro Project, runtime_params is a KedroSession concept. When config loader is used alone, it should only have access to meta_params. If we introduced this before 0.19 in a non-breaking way, it will have access to both runtime_params and meta_params, and only meta_params in 0.19.x.

Change 1: Resolves configs eagerly in ConfigLoader - only enable for parameters for now
config_loader[“parameters”] return Dict instead of DictConf
interpolation will works correctly
Change 2: Separate the runtime_params & meta_params - the name of “meta_params” need to be determined afterward. Some concerns around “runtime_params” works for “parameters” only, but “mete_params” works for everything and will confuse people

  1. Do Change 1 now - limited the scope to “parameters”
  2. Change 2 should be done, but requires some extra thinking - potentially interfering with the solution of enabling “globals” for OCL
  3. Change 2 should be done - would be great to do it in a non-breaking way. General agreement on the change, but unsure about what’s the correct name of this argument.
    Some altenative: “config_vars”, “meta_config”

Extended Discussion

How does “global” fit in the picture? It will be clearer when we finished Change 1
Antony thinks that with the validation change, it shouldn’t be matter even when the “residuals” are left in the config, Python config also don’t do anything special to extra configurations.
Ivan think this doesn’t solve the problem in a generic way, config_lodaer[“global”] is purposed, but this doesn’t solve all the problem because OmegaConfigLoader requires to merge dictionary before interpolating. This shouldn’t be a limitation, but will requires more work to implement a solution
Merel has some concern about the messaging of “meta_params”, will people really understand which argument to be used.
kedro run --params=base_path:s3_bucket will not work for catalog yet, but agree that it would be nice to support.

Possible Implementation

(Optional) Suggest an idea for implementing the addition or change.

Possible Alternatives

(Optional) Describe any alternative solutions or features you've considered.

@noklam noklam added the Issue: Feature Request New feature or improvement to existing feature label Apr 19, 2023
@merelcht
Copy link
Member

#2240 Should be done first.

@noklam
Copy link
Contributor Author

noklam commented Sep 4, 2023

Marked this in 0.19 release - we don't necessary finish this before 0.19, but the discussion should happened before this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Feature Request New feature or improvement to existing feature
Projects
Status: No status
Development

No branches or pull requests

2 participants