_ensure_container does not set flags if target is already a DictConfig/ListConfig

[Jasha10]

Describe the bug The internal omegaconf._utils._ensure_container method has a flags argument.

• If we call _ensure_container(..., flags) on a python dict or a dataclass instance, then the returned DictConfig object has cfg._metadata.flags set appropriately. This is good.
• If we call _ensure_container(..., flags) on a DictConfig instance, then the flags argument is ignored, and the returned object's flags might not be set.

To Reproduce

$ cat tmp.py
from omegaconf import OmegaConf, DictConfig
from omegaconf._utils import _ensure_container
from dataclasses import dataclass


@dataclass
class DataclassUser:
    name: str = "bond"
    age: int = 7


cfg1 = _ensure_container(DataclassUser, flags={"my_flag": True})
assert cfg1._get_flag("my_flag") is True

dict_user = {"name": "bond", "age": 7}
cfg2 = _ensure_container(dict_user, flags={"my_flag": True})
assert cfg2._get_flag("my_flag") is True

dictconfig_user = DictConfig(content={"name": "bond", "age": 7})
cfg3 = _ensure_container(dictconfig_user, flags={"my_flag": True})
assert cfg3._get_flag("my_flag") is True

$ python tmp.py
Traceback (most recent call last):
  File "/Users/lappy486/omegaconf/tmp.py", line 21, in <module>
    assert cfg3._get_flag("my_flag") is True
AssertionError

Expected behavior I would expect that the output of _ensure_container agrees with the flags argument that is specified as input. This may not be a huge issue because _ensure_container is internal.

Additional context

• [x] OmegaConf version: 2.1.0dev20
• [x] Python version: 3.9.1
• [x] Operating system : MacOS

[Jasha10]

Relevance of this issue:

I searched the current codebase. There is only one place where the flags argument to _ensure_container is actually used; in BaseContainer._merge_with, we have a for loop which calls _ensure_container with the flags argument:

        for other in others:
            if other is None:
                raise ValueError("Cannot merge with a None config")

            my_flags = {}
            if self._get_flag("allow_objects") is True:
                my_flags = {"allow_objects": True}
            other = _ensure_container(other, flags=my_flags)

            if isinstance(self, DictConfig) and isinstance(other, DictConfig):
                BaseContainer._map_merge(self, other)
            elif isinstance(self, ListConfig) and isinstance(other, ListConfig):
                BaseContainer._list_merge(self, other)
            else:
                raise TypeError("Cannot merge DictConfig with ListConfig")

Now sure how this issue impacts the observable behavior of OmegaConf.merge...

[omry]

I think the flags argument is a later addition. Ensure container is not supposed to mutate the input so it makes sense that the flags is not set. We can probably remove the flags from it and be more explicit about it.

Now sure how this issue impacts the observable behavior of OmegaConf.merge...

The above is handling a merge with of a dict into a container that has allow_objects flag set to True. I think that without passing it we will fail to convert the dict to a DictConfig.

Given that _ensure_container is an internal method, I don't care too much. Does it cause any user visible issues?

[Jasha10]

I think that without passing it we will fail to convert the dict to a DictConfig.

Oh, I see. Handling the case where the dict might contain objects.

Does it cause any user visible issues?

I don't think so.

We can probably remove the flags from it and be more explicit about it.

That sounds good -- then _ensure_container will be more simple :) I'll wait with a PR until #568 gets merged, since it is also modifying _ensure_container.