Obey _recursive_
parameter on non-target nodes
#2577
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.
Motivation
When recursively instantiating a config, the recursive parameter is ignored unless the config is a target node (i.e. has a
_target_
parameter). It would be nice if the_recursive_
flag were considered for non-target nodes as well so that a recursive flow of instantiation can be "broken" (i.e. all nodes from a certain level in the config "tree" remain asDictConfig
objects). The reason why there is an upstream_recursive_=True
is because the parent node has other children who we do want to recursively instantiate. As a concrete example, consider the following set of configs and corresponding classes to be instantiated (I tried to keep them as minimal as possible)The output of the print statement above is
"Scheduler was instantiated: True"
, while the idea behind this PR is that it should be False (due to the setting of the_recursive_=False
parameter in theMyOptimizer
config).Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Added an additional test in
tests/instantiate/test_instantiate
and verified that no existing tests are broken. As far as I can tell, none of the failing CI tests are due to this PR.Related Issues and PRs
(Is this PR part of a group of changes? Link the other relevant PRs and Issues here. Use https://help.github.com/en/articles/closing-issues-using-keywords for help on GitHub syntax)