Add _resolve_ flag to instantiate.

[kpoeppel]

Motivation

See issue #2268. Passing the whole configuration (e.g. via eval) to an instantiated object is impossible as the resolution breaks due to different top-level of the configuration.
With this addition, special objects are not resolved a second time (of course one resolve has to be called beforehand).

Have you read the Contributing Guidelines on pull #requests?

Yes

Test Plan

A simple test was already added. The default behaviour doesn't change.

Related Issues and PRs

Issue #2268

[facebook-github-bot]

Hi @kpoeppel!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

[Jasha10]

This PR does not address the use-case outlined in this comment. The assertion in the following script should pass:

from hydra.utils import instantiate
from omegaconf import DictConfig, OmegaConf


def train(cfg: DictConfig, num: int):
    return num


cfg = OmegaConf.create(
    """
func:
    _target_: __main__.train
    num: 123
"""
)
x = instantiate(cfg.func, cfg=cfg, _resolve_=False)
assert x == 124

The script instead fails with a TypeError.

[Jasha10]

This logic is tricky -- it would be good to have tests for some nested cases.

_target_: module.Outer
arg:
  _target_: module.Inner
  _resolve_: true
_resolve_: false

_target_: module.Outer
arg:
  _target_: module.Inner
  _resolve_: false
_resolve_: true

_target_: module.Outer
arg:
  _target_: module.Inner
  # _resolve_ true inherited
_resolve_: true

_target_: module.Outer
arg:
  _target_: module.Inner
  # _resolve_ false inherited
_resolve_: false

[kpoeppel]

Sorry for the late reply, as of the now pushed commit e586dc7 your code can work with _recursive_=False inside instantiate.

The problem is how to tackle recursion here, as things become self-referential potentially (-> infinite recursion). There are three options.

1. Demanding _recursive_=False in this code (current state).
2. Treat kwargs inside instantiate differently for the case _resolve_=False, such that these are excluded from the recursive instantiate. So here the implicit _recursive_=True would only apply to cfg.func and not to cfg.
3. Introducing another step for resolution (like in the Issue 2268)

What do you think works best?

[Jasha10]

I too should apologize for my late response.

The problem is how to tackle recursion here, as things become self-referential potentially (-> infinite recursion).

I am confused. Could you give me an example where infinite recursion would come up? Is this a new problem introduced by this PR, or would the infinite recursion happen even without this PR?

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

[Jasha10]

I've just realized that this PR is quite similar to what is discussed in issue #1758, specifically Omry's suggestion from this comment.

[kpoeppel]

Do you think this can be merged now from a software architecture point of view?
I still need to fix the failing tests, maybe also need some help because of their generic nested structure.

[Jasha10]

Do you think this can be merged now from a software architecture point of view?

Not sure. We might need to move the OmegaConf.resolve calls from the instantiate function into a more deeply nested function (such as instantiate_node).

This PR needs more tests before it can be merged: there's only one test for _resolve_ currently, which means edge-cases are not tested.

My advice is to use test-driven development:

• start with a clean slate of tests that pass
• add one failing test
• modify instantiate until all the tests (including the new test) pass
• add another failing test
• modify instantiate until all the tests (including the new test) pass
• ...