Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 migration steps for
TemplatedConfigLoader
toOmegaConfigLoader
#2904Add migration steps for
TemplatedConfigLoader
toOmegaConfigLoader
#2904Changes from 23 commits
28fa916
25f89f1
798f608
04bdf71
c2d3c8e
ffe59d7
bd624f9
1e8b76c
5a6fa50
47f22f5
44ec087
0925537
9b25d47
c359752
97827ff
da4f432
d04e444
1f15fef
9272efd
0b49d4c
6baa550
62510e6
40df8ff
17b594e
ee7785d
367539e
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
[Kedro.weaselwords] 'only' is a weasel word!
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, under this section, it would also be helpful to flag that the variable interpolation is only scoped to a particular configuration and the same environment. For eg., templated keys from
local
will not overwritebase
and the resolution of the keys happens within the environment which is different from howTemplatedConfigLoader
uses the 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.
Second on this. Do we already have the relevant docs in OmegaConfigLoader's page? Obviously just move everything to
global
would work(this is how template config loader works), but we may want to advise to use thecatalog_globals.yml
etc to replace the use ofglobals.py
for certain things.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.
📝 [vale] reported by reviewdog 🐶
[Kedro.sentencelength] Try to keep your sentence length to 30 words or fewer.
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.
[Kedro.toowordy] 'it was' is too wordy
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 may want to define this as it currently doesn't have context
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.
Should we add a link in case they name it something other than
globals.yml
?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.
What's the replacement for
globals_dict
?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.
There's no replacement. That's why I wrote "The
OmegaConfigLoader
requires global values to be provided in aglobals.yml
file.", but I can explicitly mention that theglobals_dict
is not supported with OCL for clarity.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.
"Should we add a link in case they name it something other than globals.yml?" @noklam what should we link to?
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.
@merelcht How about https://docs.kedro.org/en/latest/configuration/advanced_configuration.html#how-to-use-global-variables-with-the-omegaconfigloader?
Btw I think we should bubble up OmegaConfigLoader docs since we are trying to make it default. The hierarchy is also broken there is nothing inside OmegaConfigLoader. Cc @stichbury @ankatiyar
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 don't think it's broken, we don't have any sub-sections under
OmegaConfigLoader
because those are all under "Advanced Kedro configuration". We should probably clean this up when all docs onConfigLoader
andTemplatedConfigLoader
are removed as part of: #2692There 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.
@noklam We will be doing some of the moving around sections in the docs for #2900 after the "make starters use
OmegaConfigLoader
by default" PRs are merged. (@lrcouto)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 wonder if we want to provide some opinion here on why Jinja is a suboptimal solution here, not built for a whitespaced language etc.
Or point to articles like this one (last paragraph of "Pitfall 1")