Added a warning if primary config is missing _self_ #1756
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
the
CLA Signed
jieru-hu
reviewed
Aug 4, 2021
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
jieru-hu
approved these changes
Aug 5, 2021
Jasha10
reviewed
Aug 5, 2021
|
The primary feedback I am looking for is about the clarity of the doc changes. |
|
You can read them in context at the website preview: |
|
taking another pass of the doc... |
jieru-hu
reviewed
Aug 5, 2021
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Show resolved
Hide resolved
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
jieru-hu
reviewed
Aug 5, 2021
jieru-hu
reviewed
Aug 5, 2021
Jasha10
reviewed
Aug 5, 2021
Jasha10
reviewed
Aug 6, 2021
Comment on lines
+71
to
+76
| ### Composition order of primary config | ||
| Your primary config can contain both config values and a Defaults List. | ||
| In such cases, you should add the `_self_` keyword to your defaults list to specify the composition order of the config file relative to the items in the defaults list. | ||
|
|
||
| * If you want your primary config to override the values of configs from the Defaults List, append `_self_` to the end of the Defaults List. | ||
| * If you want the configs from the Defaults List to override the values in your primary config, insert `_self_` as the first item in your Defaults List. |
There was a problem hiding this comment.
These guidelines apply to nested configs too, not just the primary config, right?
There was a problem hiding this comment.
No. For nested configs, in all cases I have seen so far trailing _self_ makes sense so requiring users to specify it is not something I want to do.
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/docs/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/versioned_docs/version-1.1/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/versioned_docs/version-1.1/upgrades/1.0_to_1.1/changes_to_default_composition_order.md
Outdated
Show resolved
Hide resolved
website/versioned_docs/version-1.1/tutorials/structured_config/5_schema.md
Outdated
Show resolved
Hide resolved
kandluis
reviewed
Aug 9, 2021
| #### Requiring users to specify a default list value | ||
| ### A Note about composition order | ||
| The default composition order in Hydra is that values defined in a config are merged into values introduced from configs in the Defaults List - or in other words - overriding them. | ||
| This behavior can be unintuitive when your primary config is a Structured Config, like in the example above. |
There was a problem hiding this comment.
This is definitely quite unintuitive, though seeing the _self_ makes it a bit clearer.
There was a problem hiding this comment.
I agree. Unfortunately this behavior makes a ton of sense in all other use cases and making the behavior special in this unique cases will likely add even more confusion.
|
Left a few minor nits, but overall, the updated docs will be helpful! Thanks :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #1755.
See #1755 and documentation changes in this diff for context.