-
Notifications
You must be signed in to change notification settings - Fork 905
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 OmegaConfLoader
#2085
Add OmegaConfLoader
#2085
Conversation
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leave 1st round of comments - haven't fully understood the code but since this is a relatively large PR, it's nice to get some early comments.
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
…klabs/kedro into feature/add-omegaconf-loader
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great thank you for this awesome work! Just had a few comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! 🌟 I am happy for it to stay named OmegaConfLoader
temporarily until 0.19.0
when we can rename it to ConfigLoader
and remove the old TemplatedConfigLoader
and ConfigLoader
.
…loader instance Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is cleaner now with some of the methods moving into the class. I just re-iterate on one of the comment, generally look great!
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for explaining! Nice work🌟
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Signed-off-by: Merel Theisen <merel.theisen@quantumblack.com>
Description
Added a new config loader class based on
AbstractConfigLoader
that usesOmegaConf
to load and merge configuration.Background 🔍
OmegaConf
as replacement foranyconfig
#1657OmegaConf
as replacement foranyconf
#1868Development notes 🏗
Some differences between the
OmegaConfLoader
and the other config loader classes:OmegaConf
, because they offer variable interpolation. Hence I didn't copy over the testtest_ignore_hidden_keys
ScannerError
withParserError
to catch this.Other notes:
.yml
or.yaml
extension, I found out thatOmegaConf
can also handle.json
files. So I've added.json
as an acceptable extension for this loader as well.config/common.py
. The methods in there are deeply nested and hard to follow. Usage of these methods makes it pretty much impossible for users to customise any configloader implementations.Review notes ✍️
config/common.py
To Discuss 🗣
❓Which built-in resolvers should be turned off?
We decided to initially turn off all build-in resolvers. It's easier to introduce them step by step, but removing them would be a breaking change. In #1909 we will introduce environment variables for credentials. You can find a description of all built-in resolvers here: https://omegaconf.readthedocs.io/en/2.2_branch/custom_resolvers.html#built-in-resolvers
I feel like maybe it's okay to keep some resolvers turned on. For example
oc.dict
seems useful and not something we wouldn't want to add. I would like to hear what others think.❓ Is
OmegaConfLoader
the best name?I first wanted to call this config loader
YAMLConfigLoader
but then I found out thatOmegaConf
can also load.json
files. Any better name suggestions?❓ Will
OmegaConfLoader
replaceConfigLoader
?Follow up on the above question. I'm thinking that in
0.19.0
we should remove theConfigLoader
andTemplatedConfigLoader
and rename this newOmegaConfLoader
toConfigLoader
. If we do that, maybe the name of this new class doesn't matter so much for the0.18
series.Checklist ✅
RELEASE.md
file