OmegaConf.to_object: Instantiate structured configs

[Jasha10]

Closes #472

The idea is to add an instantiate_structured_configs keyword argument to the OmegaConf.to_container method.
If OmegaConf.to_container(cfg, instantiate_structured_configs=True) is called with this new flag, the returned python object will include instantiated dataclass or attrs instances.

[omry]

Looking at the code, I believe this does not handle nested structured configs right now.
Also, this means to_container() can actually return an instance now, which is a bit weird:

1. The same is not great in such a case.
2. I did not see see the expected type changes demonstrating you are accounting to this at a glance.

We should probably introduce an alias to to_container(), maybe to_object() which will call to_container but will allow Any return type.

[Jasha10]

Looking at the code, I believe this does not handle nested structured configs right now.

I am having success with nested structured configs:

@dataclass
class Inner:
    cost: float

@dataclass
class Outer:
    i: Inner

cfg = OmegaConf.create(Outer(Inner(1.23)))
print(OmegaConf.to_container(cfg, instantiate_structured_configs=True))
# prints Outer(i=Inner(cost=1.23))

It is also working for e.g. structured config as a dict value type.

Also, this means to_container() can actually return an instance now, which is a bit weird:

1. The same is not great in such a case.

2. I did not see see the expected type changes demonstrating you are accounting to this at a glance.

I see what you mean. I'm surprised that mypy did not catch that.

We should probably introduce an alias to to_container(), maybe to_object() which will call to_container but will allow Any return type.

Ok, sounds good. So to_container will keep the old behavior (return python primitives) and to_object will instantiate structured configs?

[omry]

I am having success with nested structured configs:

Please add actual tests.
Can you explain why this works?

I also noticed that you are using get_structured_config_data() which is not designed for this (it's used to convert from an sc to a dictconfig) and is actually a rather weird function.
Can you explain what you are using it for exactly, it's not clear to me looking at the code what value it provides you. (it's returning a dictionary where values can be DictConfigs).

Ok, sounds good. So to_container will keep the old behavior (return python primitives) and to_object will instantiate structured configs?

Seems reasonable.
to_container() can still have the new parameter instantiate_structured_configs. (I think a better name for it is instantiate). Doc string should explain it in more detail.

[Jasha10]

Rebased onto master

[Jasha10]

I am having success with nested structured configs:

Please add actual tests.

I've added tests for nested structured config (f02261e).

Can you explain why this works?

It works because BaseContainer._to_content converts leaf nodes first before converting nodes that are higher up in heirarchy.

For example, if you have StructuredConfigOuter.foo == StructuredConfigInner, the _to_content container will first convert StructuredConfigInner to DataClassInner, and will then convert StructuredConfigOuter to DataClassOuter (using DataClassInner to populate the foo attribute of DataClassOuter).

[Jasha10]

I also noticed that you are using get_structured_config_data() which is not designed for this (it's used to convert from an sc to a dictconfig) and is actually a rather weird function.

The get_structured_config_data function is mostly useful in the case where a dataclass subclasses a Dict:

@dataclass
class Str2StrWithField(Dict[str, str]):
    foo: str = "bar"

When the _instantiate_structured_config_impl function instantiates a Str2StrWithField object, it first does

result = Str2StrWithField(foo=...)

to create the object, then it does

result.update(remaining keys that are not "foo")

I'm using the get_structured_config_data function to get a list of attributes for the dataclass, so that we will know which keys should be passed to Str2StrWithField(...) as keyword arguments in the first step, and which keys should be passed to result.update in the second step.

(it's returning a dictionary where values can be DictConfigs).

Hmm...
Do you think I should refactor the useful part of get_structured_config_data into another helper function?

[odelalleau]

No more comment from me at this point, thanks!

[omry]

The data passed in is derived from the content of self, which is why I am asking it.
It feels redundant.
You have custom code here that is handling things, it can resolve interpolations and deal with containers.

am I missing anything?

[omry]

I see a few things there that does not make sense when converting a DictConfig backed by a dataclass to an object:

1. Support for not resolving interpolations
2. Support for converting enums to string (doesn't make sense if we are creating an object).

What do you think about this high level approach:

# converts THIS dict config to an object if it's backed by a Structured Config (error otherwise):
DictConfig._to_object()

The implementation will use _to_content(scmode=Instantiate) for any nested containers (ListConfig or DictConfig).
_to_content() can call DictConfig._to_object() if the container it is called on is a DictConfig backed by a Structured Config (and the sc mode is instantiate)

[omry]

I like what I am seeing. A few nits inline.
@odelalleau, can you take a second look? there have been some significant changes.

[odelalleau]

Just some small comments.

Overall looks good to me. I'm not entirely sure we should ignore the resolve flag for structured configs (if someone wants an unresolved dataclass to later convert it back to a DictConfig, why not?), but I don't care much about it so let's go :)

[odelalleau]

@omry calling get_node() followed by _dereference_node() is a somewhat common operation and it's a bit annoying right now with the mypy asserts.
Would it make sense to add a get_dereferenced_node() function that would return a Node (not optional) to streamline such code?

[Jasha10]

That would make sense to me.
I'll do it in another PR :)

[Jasha10]

First step is here: #668

[omry]

shipit!