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 globals functionality to OmegaConfigLoader using custom "globals" resolver #2794

Closed
Tracked by #2919
ankatiyar opened this issue Jul 13, 2023 · 3 comments · Fixed by #2921
Closed
Tracked by #2919

Add globals functionality to OmegaConfigLoader using custom "globals" resolver #2794

ankatiyar opened this issue Jul 13, 2023 · 3 comments · Fixed by #2921

Comments

@ankatiyar
Copy link
Contributor

ankatiyar commented Jul 13, 2023

Description

After the introduction of variable interpolation in OmegaConfigLoader, two issues still stand -
* Sharing variables across different configs (catalog, parameters etc)
* Sharing variables across different envs (base, local etc)

This will be resolved by allowing global variables for configs.

Possible Implementation

Proposed by @marrrcin 🥇

To use globals, in your project -

  • Update settings.py
CONFIG_LOADER_ARGS = {
    "globals_pattern": "*globals.yml"
}
  • Add globals.yml to conf/base/ or conf/local
  • To use global values in parameters.yml or catalog.yml
    globals.yml
datasets: 
    csv: pandas.CSVDataSet

test_size: 0.3

catalog.yml

reviews:
  type: ${globals:"datasets.csv"}
  filepath: <path_to_file>

parameters.yml

model_options:
    test_size: ${globals:"test_size"}

Here "globals" is a custom resolver - https://omegaconf.readthedocs.io/en/latest/custom_resolvers.html#id9

Q: Configurable globals_pattern?
A: Allow users to pass globals_pattern through settings.py but have it be globals_pattern: {"*globals.yml"} by default.

Q: Where would globals.yml go?
A: The globals.yml files would stay within conf/base/ and conf/local/ instead of a top level conf/globals.yml. The common keys in local/globals.yml will overwrite the ones in base/globals.yml

conf/
  |_ base/
    |_ globals.yml
    |_ catalog.yml
  |_ local/
    |_ globals.yml
    |_ catalog.yml

Q: Soft or hard merge common keys?
A: Overwrite keys from base/globals.yml with keys from local/globals.yml for now but allow soft-merging in Kedro 0.19

Q: Allow globals_dict like TemplatedConfigLoader?`
A: No, this use case for dynamically providing global values can be solved using "custom resolvers"

Originally posted by @ankatiyar in #2175 (comment)

TBD

Q: Do global variables need to start with "_"?
Q: What should the name of the custom resolver be? "oc.global", "global" etc?

@marrrcin
Copy link
Contributor

The first point:

  • Update settings.py
CONFIG_LOADER_ARGS = {
    "globals_pattern": "*globals.yml"
}

should be optional, with *globals.yml being default, right?

@ankatiyar
Copy link
Contributor Author

That's right @marrrcin, thanks, I missed mentioning it!

@merelcht merelcht moved this to To Do in Kedro Framework Aug 4, 2023
@ankatiyar ankatiyar self-assigned this Aug 7, 2023
@ankatiyar ankatiyar moved this from To Do to In Progress in Kedro Framework Aug 7, 2023
@ankatiyar ankatiyar moved this from In Progress to To Do in Kedro Framework Aug 7, 2023
@ankatiyar ankatiyar moved this from To Do to In Progress in Kedro Framework Aug 7, 2023
@ankatiyar ankatiyar moved this from In Progress to In Review in Kedro Framework Aug 11, 2023
@noklam
Copy link
Contributor

noklam commented Aug 17, 2023

We found that _ is not support for globals.yml. It will now give an clear message that you shouldn't use _ for globals at all.

Noted this is not because of the implementation of the resolver, but rather OmegaConfigLoader filter out everything with _. If it turns out it is needed we can always add it and it would not be breaking. (if we enable it now and change later , it will be breaking).

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

Successfully merging a pull request may close this issue.

3 participants