-
Notifications
You must be signed in to change notification settings - Fork 906
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 link to advanced runtime_params configuration #4341
Conversation
Signed-off-by: Dmitry Sorokin <[email protected]>
I'm wondering if we should update the above "How to specify parameters at runtime" section to use the |
@@ -169,3 +169,6 @@ kedro run --params="key1=value with spaces,key2=value" | |||
``` | |||
|
|||
Since key-value pairs are split on the first equals sign, values can contain equals signs, but keys cannot. | |||
|
|||
|
|||
> **Note:** If you want to override values of certain keys in your configuration with runtime parameters provided through the CLI option, you can use the [OmegaConfigLoader `runtime_params` resolver](advanced_configuration.md#how-to-override-configuration-with-runtime-parameters-with-the-omegaconfigloader). |
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.
You can use the {note}
notation instead, to maintain consistency. Eg -
kedro/docs/source/configuration/parameters.md
Lines 71 to 73 in 63d7516
```{note} | |
You can use `add_feed_dict()` to inject any other entries into your `DataCatalog` as per your use case. | |
``` |
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.
@ankatiyar, thanks! I agree, but I tried that syntax, and it doesn’t support links. Since I have one link there, my suggestion is to remove the word "Note" and keep the current syntax. What do you think?
Signed-off-by: Dmitry Sorokin <[email protected]>
Do you mean we should merge the two articles into the first one and remove the advanced article? |
I mean, keep the section in Advanced Configuration as it is but also mention the |
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 took me a while to understand the issue here. --params
is something that Kedro supports since the beginning. It supports parameters ONLY with arbitary nested level, so I find the explanation here does not explains why user need to use $runtime_params
.
$runtime_params
is added later with OmegaConfigLoader
to support overriding ANY config (i.e. catalog, a common use case is pointing to a different s3 bucket). So maybe the focus here should be, if user need to override configuration other than parameters
, they should use $runtime_params
?
+1 to what @noklam said 👍🏼 |
Signed-off-by: Dmitry Sorokin <[email protected]>
Thanks for the clarification, @noklam. I was also confused about why we added additional functionality for doing the same thing. I've made the updates and hope this works well. Requesting your re-reviews. |
Co-authored-by: Merel Theisen <[email protected]> Signed-off-by: Dmitry Sorokin <[email protected]>
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.
⭐
Description
Closes #3562
Developer Certificate of Origin
We need all contributions to comply with the Developer Certificate of Origin (DCO). All commits must be signed off by including a
Signed-off-by
line in the commit message. See our wiki for guidance.If your PR is blocked due to unsigned commits, then you must follow the instructions under "Rebase the branch" on the GitHub Checks page for your PR. This will retroactively add the sign-off to all unsigned commits and allow the DCO check to pass.
Checklist
RELEASE.md
file