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

Draft - Resolves configuration earlier & separate runtimes_params and meta_params #2504

Conversation

noklam
Copy link
Contributor

@noklam noklam commented Apr 11, 2023

Signed-off-by: Nok Chan nok.lam.chan@quantumblack.com

NOTE: Kedro datasets are moving from kedro.extras.datasets to a separate kedro-datasets package in
kedro-plugins repository. Any changes to the dataset implementations
should be done by opening a pull request in that repository.

Description

  1. Resolve runtime_params earlier
  2. Fix problem when len(aggregate_config) == 1
ds_name='example_iris_data' ds_config={'type': 'pandas.CSVDataSet', 'filepath': '/Users/Nok_Lam_Chan/dev/test/omega-nest-param/data/01_raw/iris.csv'}

│ /Users/Nok_Lam_Chan/GitHub/kedro/kedro/io/data_catalog.py:272 in from_config                     │
│                                                                                                  │
│   269 │   │                                                                                      │
│   270 │   │   layers = defaultdict(set)  # type: Dict[str, Set[str]]                             │271 │   │   for ds_name, ds_config in catalog.items():                                         │
│ ❱ 272 │   │   │   ds_layer = ds_config.pop("layer", None)                                        │
│   273 │   │   │   if ds_layer is not None:                                                       │
│   274 │   │   │   │   layers[ds_layer].add(ds_name)  
  1. Separate runtime_params & meta_params
  2. Bonus: It may also simplify the syntax - since meta_params will not be pass to catalog and avoid the validation problem.

Development notes

Challenge 1 - Parameters resolved too late
Once we merge parameters in ConfigLoader, all runtime_params are merged. This is fine for parameters, but when it's done for catalog -

Challenge 2 - Works for parameters but not for other configs
We get invalid entries and we don't really want it. Same goes for logging. This wasn't an issue in #2467 because of an accident.

Currently this PR takes a simple solution - it is only enable for parameters and nothing else. This is actually the same problem
of #2175, but was hidden because of the above logic.

if len(aggregate_config) == 1:
return list(aggregate_config)[0]
return dict(OmegaConf.merge(*aggregate_config))

Checklist

  • Read the contributing guidelines
  • 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

Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam noklam linked an issue Apr 11, 2023 that may be closed by this pull request
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
@noklam noklam changed the title Draft - Push OmegaConfigLoader resolve config earlier and separate runtime_params and meta_params Draft - Resolves configuration earlier & separate runtimes_params and meta_params Apr 12, 2023
Signed-off-by: Nok Chan <nok.lam.chan@quantumblack.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OmegaConfigLoader parameters does not propogate properly
1 participant