Hydra: [Bug] Impossible to instantiate nested dataclasses

[guillaumegenthial]

🐛 Bug

Description

I want to instantiate recursively nested dataclass instances.

Because OmegaConf considers dataclasses instances to be configs, when calling _get_kwargs during the instantiate call, the dataclass instance is converted back to a config because parameters are added to a OmegaConf list / dict.

See https://github.com/facebookresearch/hydra/blob/c1eb2435b6314412d7029acbf87efd63d84d384c/hydra/_internal/utils.py#L685

Checklist

• [x] I checked on the latest version of Hydra
• [x] I created a minimal repro

To reproduce

In a script my_app.py

"""Test hydra dataclass instantiation."""

from dataclasses import dataclass
from omegaconf import OmegaConf
from hydra.utils import instantiate

@dataclass
class A:
    x: int

@dataclass
class B:
    a: A

conf = {
    "_target_": "__main__.B",
    "a": {
        "_target_": "__main__.A",
        "x": 1
    }
}
parsed = OmegaConf.create(conf)
instance = instantiate(parsed)
print(instance)

run python my_app.py.

You get

B(a={'x': 1})

Expected Behavior

I expect the result to be B(a=A(x=1)).

It seems strange to me that the config schema interferes with the instantiation.

I work with a codebase where a bunch of classes are dataclasses and using hydra for configuration would be a plus.

System information

• Hydra Version : master
• Python version : 3.6
• Virtual environment type and version : instantiated with python -m venv
• Operating system : macOS 11.0.1

Additional context

Add any other context about the problem here.

[jieru-hu]

Thanks for the report!

I tried this locally and it seems like A was actually converted to the correct type however when it was added to B the typing info got lost. I'm wondering if this is something more related to omegaconf side.

cc @omry

[guillaumegenthial]

Thanks for the quick response.

I don't think it's an issue with omegaconf (it's the intended behavior from omegaconf when adding a dataclass instance to a OmegaConf object, as listed in the doc).

I am rather wondering why the _get_kwargs function uses OmegaConf objects to store instantiated objects (why final_kwargs has to be an OmegaConf object, instead of a regular python dict)?
https://github.com/facebookresearch/hydra/blob/c1eb2435b6314412d7029acbf87efd63d84d384c/hydra/_internal/utils.py#L653

[omry]

@guillaumegenthial, thanks for reporting.
This is indeed a limitation of the current implementation.

The reason is related to support of interpolation during instantiation.
Nested config objects can have interpolation. Here is an example in the tests.

As a temporary workaround (which may be the base of a permanent solution), can you try to wrap your dataclass in a regular object?

Something like this, although I am sure it has many mistakes:

@dataclass
class A:
    x: int

@dataclass
class B:
    a: A

class BoxedA:
  def __init__(x : int):
    self.a = A(a)

conf = {
    "_target_": "__main__.B",
    "a": {
        "_target_": "__main__.BoxedA",
        "x": 1
    }
}

If the approach works, we might use it automatically inside instantiate for instantiated objects that are dataclasses.

[guillaumegenthial]

Ok, thanks for the details.

I looked at the code, and from what I understand

1. it is indeed a desired behaviour that nested dataclasses instances are converted to DictConfig instances when _convert_ = ConvertMode.NONE
2. I suppose that one of hydra's usage (indicated by the test) is dynamic dependency injection: when the config is updated, instantiated objects whose attributes are DictConfig should have new values.

Do you confirm 1 and 2 ?

Then, the "bug" is actually to fix the behavior on dataclass instances when _convert_ = ConvertMode.ALL.
In other words, this line should be is_dataclass(ret.a) when _convert_ = ConvertMode.ALL.

[omry]

I need to take a second look at the code and the tests to see if this is desired or not.
As you have probably realized by now, this is a complex bit of code.
At a high level, I think instantiated dataclasses should be treated differently.
(normally dataclasses role is to be config objects, but here we are instantiating them, not getting them from the outside).

ConvertMode is definitely related here, but OmegaConf does not support converting back to dataclass instances.
Also, it's inefficient to instantiate the dataclass, convert it to DictConfig, and then instantiate it again from the DictConfig so I would rather avoid this path.

You probably saw "allow_objects": True in there.
This enables the placement of instantiated objects and of passthrough objects into final_kwargs and eventually into the function call.

What I am thinking is that can leverage that behavior and hide instantiated dataclasses (and maybe all instantiated objects) in a box that would make OmegaConf think they are regular objects (thus preventing their conversion to DictConfig).
When we actually call the target we can unbox any boxed objects and pass the real instantiated object to the target function.

The test I think you are referring to in 2, where the interpolation changes are effective after the object is instantiated, are relevant when the object is getting a DictConfig object as a parameter (in the roll of a config), and it has interpolation in it. And the value pointed to by the interpolation is changed later.
In that case, the test is ensuring that the config link to the parent config is retained and interpolation can take effect later.
This is a corner case and I don't mind changing this behavior - for example to somehow limit it to DictConfig and not to dataclass instances somehow.
But before going in that direction, let's see my suggestion above can solve the problem.

[guillaumegenthial]

All this makes a lot of sense, and indeed, the close relationship between omegaconf and hydra introduces complexity.

Without the live interpolation, the design would have been simpler. Hydra could

1. resolve the config with omegaconf (resolve interpolations etc.) and convert it to a simple dict container
2. instantiate objects from dictionaries.

But this seems like a design choice so we have to deal with it.

As you suggested, the way I see things, we have multiple options

1. Modify OmegaConf.to_container to return dataclasses instance when appropriate. The difficulty is in the "when appropriate". Also, converting back and forth is something we should avoid (efficiency, but mostly more prone to mistakes).
2. Support the distinction between a config dataclass and a regular dataclass in omegaconf
3. Use the box trick on the hydra side, to hide real dataclasses instances from omegaconf.
4. Change the kwargs logic on the hydra side to use omegaconf items more sparingly

For option 1., we want to return OmegaConf.get_type(conf)(**retdict) in place of this line in some situations. We have 3 situations (as dict, as DictConfig and as DataClass) which would require us to add a new parameter to the to_container method (today there is exclude_structured_configs which controls dict or DictConfig). Also, we have to treat all StructuredConfig objects in the same way (we probably don't want to). For those reasons, option 1 is a bit of a dead end.

One way to implement option 2, is to set an _IS_STRUCTURED_CONFIG = False attribute to dataclass instances that are to be treated as objects (assuming the allow_objects flag is activated).

Option 3 is closely related to option 2 but is not omegaconf native. I am a bit worried of the complexity of boxing / unboxing in the hydra side.

Option 4 is a bit fuzzier to me since I'm not too familiar with the code. But what I had in mind was to use regular lists and dictionaries when instantiating (for kwargs, etc.), except for places where we know for sure we want to preserve an omegaconf object. Or treat the _convert_ = ConvertMode.ALL mode differently and do what I suggested at the beginning if we didn't need the live interpolation: first resolve the config, then instantiate recursively.

[omry]

The idea with allow_objects: true is to make final_kwargs more like a standard dictionary (normally OmegaConf would reject unsupported types), while retaining interpolation support.
As I said, the live interpolation use case is not critical.

I think boxing and unboxing instantiated objects can be a pretty nice solution here.
If you want to give it a try I can make a few suggestions of how to tackle this.

By the way: did you look at the documentation for the new instantiate?
https://hydra.cc/docs/next/patterns/instantiate_objects/overview
It covers the conversion strategy as well.

[guillaumegenthial]

I missed the new documentation, I'll have a look, thank you.

I tried to do option 2, it's a sketch but seems to work.
The user's dataclass is preserved, and nested subdicts which are configs are also preserved (giving live interpolation).

See

• omegaconf change
• hydra change

There seems to be a few problems with side-cases (the to_container does not support the new attribute) but it gives a global idea.

And sure, I can also give the boxing option a try.

[omry]

I don't like 2. Feels like should be able to differentiate instantiated dataclasses versus dataclasses passed in as config.
For someone looking at OmegaConf alone (which is lower level than Hydra and does not know about it) - the change there makes no sense at all.

[guillaumegenthial]

Ok, that makes sense.

It would indeed introduce unnecessary confusion and complexity.

Let's try to do the boxing / unboxing trick on the hydra side.

[omry]

@guillaumegenthial, did you have a chance to try my proposed approach?

[omry]

@guillaumegenthial , please try #1223 and tell me if it covers your use case.

[omry]

or, scratch that. looks like it's failing some important tests. once I merge it.

[guillaumegenthial]

Hello @omry,

It's been a while and I did not push the WIP I had, which I just did.

See https://github.com/facebookresearch/hydra/pull/1224/files

It seems that #1223 does not exactly cover my use case (I tried with the snippet provided at the beginning of this issue).

My PR Draft seems to do the job, but tbh I'm not satisfied.

The main difference with your PR is that I unbox in instantiate, to control the unboxing using the top-level convert mode.

I'm thinking that instead of pursuing this path, we should simplify instantiate when convert is ConvertMode.ALL, after clarifying the expected behavior depending on the convert mode (what do we want for dataclasses, what about recursion, which node has priority over the convert mode, etc.)

[omry]

I think my solution is clean. the reason it's not working is that I only box structured configs instantiated in lists and dicts.
It should be pretty easy to add the missing case.

My gut feeling is that we should preserve instantiated dataclasses regardless of the convert mode.

[omry]

@guillaumegenthial , I just tested #1223 with your snippet and it works:

@dataclass
class A:
    x: int


@dataclass
class B:
    a: A


def test_wut():

    conf = {
        "_target_": "tests.test_utils.B",
        "a": {
            "_target_": "tests.test_utils.A",
            "x": 1,
        },
    }
    assert utils.instantiate(conf) == B(a=A(x=1))

What are you getting?

[omry]

Ah, not working. equal is being too smart. let me try a fix.

[guillaumegenthial]

Yes exactly!

The thing I noticed and the reason why I went for unboxing in instantiate and not in _get_kwargs is because if you unbox but add the result to a DictConfig, you did the boxing for nothing.

If you print stuff and run the test, it's clear what's happening.

def _get_kwargs(
    config: Union[DictConfig, ListConfig],
    root: bool = True,
    **kwargs: Any,
) -> Any:
    print("_get_kwargs", config, kwargs)
    ret = _get_kwargs_impl(config, root, **kwargs)
    print("_get_kwargs", config, kwargs, ret)
    ret = _unbox(ret)
    print("_get_kwargs", config, kwargs, ret)
    return ret

you get

_get_kwargs {'a': {'_target_': '__main__.A', 'x': 1}} {'_convert_': 'all'}
_get_kwargs {'x': 1} {}
_get_kwargs {'x': 1} {} {'x': 1}
_get_kwargs {'x': 1} {} {'x': 1}
_get_kwargs {'a': {'_target_': '__main__.A', 'x': 1}, '_convert_': 'all'} {'_convert_': 'all'} {'a': Boxed(A(x=1)), '_convert_': 'all'}
_get_kwargs {'a': {'_target_': '__main__.A', 'x': 1}, '_convert_': 'all'} {'_convert_': 'all'} {'a': {'x': 1}, '_convert_': 'all'}
B(a={'x': 1})

To resolve this, I use plain dictionaries if convert mode is ALL in instantiate to stored the unboxing result.

[guillaumegenthial]

Why don't we go for a simpler approach?

If convert mode is all, first convert all the config to plain dict / lists etc. and then do the recursive instantiation.

Something like this, at the beginning of instantiate

if kwargs.get("_convert_") == "all":
        config = OmegaConf.to_container(config, resolve=True)

        def _rec_instantiate(cfg):
            if isinstance(cfg, list):
                return [_rec_instantiate(it) for it in cfg]
            if isinstance(cfg, dict):
                target = _get_target_type(cfg, kwargs)
                return target(**{key: _rec_instantiate(value) for key, value in cfg.items()})
            return cfg

        return _rec_instantiate(config)

[omry]

I did not consider this approach and I can't tell you if it makes sense or not without spending some time implementing it.

1. I don't know that it's actually simpler.
2. I don't know that it really works (your PR is did not include any tests).
3. Most important: I don't think this should be conditioned on convert mode being all. Ideally instantiated dataclasses would not be converted (they are a part of the user app, not of the config).

[omry]

@guillaumegenthial , can you join the chat?

[guillaumegenthial]

I just joined the chat, good idea.

Also, have a look at https://github.com/facebookresearch/hydra/pull/1225, it's a draft of what I was proposing