-
Notifications
You must be signed in to change notification settings - Fork 903
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 "runtime_params" resolver to allow overriding of config with CLI params #3036
Conversation
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.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 @ankatiyar ! I have also tried some simple use cases and it works perfectly 👍 I agree that it's should be allowed for users to use globals
as a default inside the runtime_params
resolver. But definitely the other way around doesn't make sense.
Are you adding docs as part of this PR or separately?
kedro/config/omegaconf_config.py
Outdated
except UnsupportedInterpolationType: | ||
raise UnsupportedInterpolationType( | ||
"The `runtime_params:` resolver is not supported for globals." | ||
) |
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 could potentially be raised because of something else right? So we should make sure that this specific message only shows when the user is trying to use runtime_params
with globals.
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.
I've updated the condition to only show this message if runtime_params
is used.
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.
I've also added docs to this PR itself since it was just one section.
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.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 is great! I think the docs are very clear too 👍
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.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 good to me, thanks @ankatiyar!
I made a commit to the branch to move the table of contents that links to the "How to " subsections to the top of the page (as there was some repetition with it in the place previously sited).
Co-authored-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Co-authored-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
kedro/config/omegaconf_config.py
Outdated
else: | ||
raise exc | ||
# Re-register runtime params resolver incase it was deactivated | ||
self._register_runtime_params_resolver() |
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.
I find this a bit hard to follow - I am not sure if this is registered or not. If I follow the logic, it assumes that it is always on and only turn off if key==global. Maybe this should move to the beginning instead? Then the __init__
register_runtime_params_resolvers can be removed optionally.
In fact, I found this is inconsistent and I think we should treat this same as the read_environment_variable
flag.
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.
Since replace=True
is set in _register_runtime_params_resolver()
, I didn't add an if condition here. It'll re-register it anyway.
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.
I've moved it to the beginning and removed the call from __init__
.
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.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.
Btw the docs failure for the databricks was fixed in develop
with https://docs.databricks.com/en/dev-tools/cli/authentication.html
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
kedro/config/omegaconf_config.py
Outdated
def __setitem__(self, key, value): | ||
if key == "globals": | ||
self._globals = value | ||
super().__setitem__(key, value) |
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.
I think we should have a comment explaining why we treat global differently.
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.
Approved with minor comments. It's a great feature to add!
Co-authored-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
Description
Resolve #2531
Development notes
runtime_params
resolver_registed_runtime_params_resolver()
_get_runtime_value()
to get the interpolated value, also similar to globals'_get_globals_value()
Changes to
globals
self._globals
in the__init__
function._get_globals_value()
it uses theself._globals
dict.runtime_params
resolver is cleared and then registered again in the__getitem__
fnself["globals"]
each time to resolve wherever globals is used.On the topic of nested resolvers
Need feedback from reviewers on this
runtime_params
inglobals.yml
by disabling theruntime_params
in__getitem__
if the key is "globals" and then registering it again. Throws anUnsupportedInterpolationType
error whenruntime_params:
is used in globals.runtime_params
is tricky, I haven't found a clean way to do it.However, I think allowing globals to be used as a default value for parameters with
runtime_params
resolver is valid and might be useful and we probably should allow this -TODO
Checklist
RELEASE.md
file