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 "runtime_params" resolver to allow overriding of config with CLI params #3036

Merged
merged 14 commits into from
Sep 27, 2023

Conversation

ankatiyar
Copy link
Contributor

@ankatiyar ankatiyar commented Sep 15, 2023

Description

Resolve #2531

Development notes

runtime_params resolver

  • similar to globals - registered using _registed_runtime_params_resolver()
  • Uses _get_runtime_value() to get the interpolated value, also similar to globals' _get_globals_value()

Changes to globals

  • Read globals and populate self._globals in the __init__ function.
  • When global values are read by _get_globals_value() it uses the self._globals dict.
  • This is so when globals are first read, the runtime_params resolver is cleared and then registered again in the __getitem__ fn
  • Also helps that we don't need to read self["globals"] each time to resolve wherever globals is used.

On the topic of nested resolvers

Need feedback from reviewers on this

  • I disable the use of runtime_params in globals.yml by disabling the runtime_params in __getitem__ if the key is "globals" and then registering it again. Throws an UnsupportedInterpolationType error when runtime_params: is used in globals.
  • Disabling globals within 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 -

my_param : "${runtime_params: y, ${globals: x, 34}}"

TODO

  • Test
  • Docs
  • Release Notes

Checklist

  • Read the contributing guidelines
  • Signed off each commit with a Developer Certificate of Origin (DCO)
  • 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
  • Checked if this change will affect Kedro-Viz, and if so, communicated that with the Viz team

Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
@ankatiyar ankatiyar marked this pull request as ready for review September 19, 2023 15:02
Copy link
Member

@merelcht merelcht left a 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?

Comment on lines 193 to 196
except UnsupportedInterpolationType:
raise UnsupportedInterpolationType(
"The `runtime_params:` resolver is not supported for globals."
)
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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>
Copy link
Member

@merelcht merelcht left a 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 👍

RELEASE.md Outdated Show resolved Hide resolved
Signed-off-by: Jo Stichbury <jo_stichbury@mckinsey.com>
Copy link
Contributor

@stichbury stichbury left a 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).

ankatiyar and others added 2 commits September 20, 2023 16:54
RELEASE.md Outdated Show resolved Hide resolved
Co-authored-by: Nok Lam Chan <nok.lam.chan@quantumblack.com>
kedro/config/omegaconf_config.py Show resolved Hide resolved
kedro/config/omegaconf_config.py Show resolved Hide resolved
tests/config/test_omegaconf_config.py Outdated Show resolved Hide resolved
else:
raise exc
# Re-register runtime params resolver incase it was deactivated
self._register_runtime_params_resolver()
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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>
Copy link
Member

@merelcht merelcht left a 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

ankatiyar and others added 2 commits September 27, 2023 16:00
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
Comment on lines 149 to 152
def __setitem__(self, key, value):
if key == "globals":
self._globals = value
super().__setitem__(key, value)
Copy link
Contributor

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.

Copy link
Contributor

@noklam noklam left a 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>
@ankatiyar ankatiyar enabled auto-merge (squash) September 27, 2023 15:15
@ankatiyar ankatiyar enabled auto-merge (squash) September 27, 2023 15:17
Signed-off-by: Ankita Katiyar <ankitakatiyar2401@gmail.com>
@ankatiyar ankatiyar merged commit 319c90f into main Sep 27, 2023
62 of 72 checks passed
@ankatiyar ankatiyar deleted the runtime-params-resolver branch September 27, 2023 16:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants