Repository: mit-ll-responsible-ai/hydra-zen (python)

Issue:
Bypassing omegaconf issue that leads to unexpected results/bug
See https://github.com/omry/omegaconf/issues/830 for the description of the bug.

In short, instantiation "ignores" child class' default-factory-field in favor of the parent's value (unless that value is specifically `MISSING`)

```python
>>> A = make_config(x=1)
>>> B = make_config(x=[1, 2], bases=(A,))  # must be a mutable value to trigger the bug
>>> instantiate(B)
{'x': 1}
```

Given that hydra-zen users are particularly heavy users of dataclasses who do not have to deal with the cumbersom nature of manually creating default-factories (since we create the default-factories for them), they are much more likely to be affected by this bug than the average Hydra user, I suspect. 

Thus it is pressing that we address this on the hydra-zen end either by: hard-pinning the minimum version of `omegaconf` to require the patched version, or, "detecting" this scenario ourselves and use a workaround. E.g., @jgbos pointed out that we can attempt to detect and remedy this issue for users by effectively doing the following "under the hood":

```python
B = make_config(x=builds(list, [1, 2]), bases=(A,))
```

this remedies the issue :

```python
>>> A = make_config(x=1)
>>> B = make_config(x=builds(list, [1, 2]), bases=(A,))  # <- make_config(x=[1, 2], bases=(A,)) will be replaced by this
>>> instantiate(B)
{'x': [1, 2]}
```

Pull request:
Patch upstream omegaconf issue #830
Closes #170 

For users using `omegaconf` 2.1.1 or earlier, we provide a patch of an upstream bug.

Before:

```python
>>> A = make_config(x=1)
>>> B = make_config(x=[1, 2], bases=(A,))  # must be a mutable value to trigger the bug
>>> instantiate(B)
{'x': 1}
```

After:

```python
>>> A = make_config(x=1)
>>> B = make_config(x=[1, 2], bases=(A,))  # must be a mutable value to trigger the bug
>>> instantiate(B)
{'x': [1, 2]}
```

We achieve this by detecting the particular conditions for which this bug occurs:
- The user is not using a newer version of `omegaconf`, where this bug is fixed
- and.. the config sets a mutable default value via default-value factory
- and.. the config inherits from one or more parent dataclasses
- and.. the parent also provides a field of the same name
- and.. the parent's field is *not* a default-value factory

*and only under these circumstances* will we automatically transform

```python
make_config(x=[1, 2], bases=(A,))
```

to

```python
make_config(x=builds(list, [1, 2]), bases=(A,))
```

which ensures that the resulting config will instantiate as-expected.

We provide this automatic support for all use-cases of `make_config` and `builds`. We cannot provide this automatic support for `hydrated_dataclass`, however we do still detect the conditions and raise a `HydraZenValidationError` in such circumstances. 

Patch:
diff --git a/setup.cfg b/setup.cfg
index 2d20792b..50758e0a 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -42,6 +42,7 @@ commands = pytest \
 
 [testenv:min-deps]  # test against minimum dependency versions
 deps = hydra-core==1.1.0
+       omegaconf==2.1.1
        typing-extensions==3.10.0.1
        {[testenv]deps}
 basepython = python3.7
@@ -60,6 +61,7 @@ setenv = NUMBA_DISABLE_JIT=1
 usedevelop = true
 basepython = python3.8
 deps = {[testenv]deps}
+       omegaconf==2.1.1  # ensure we cover paths that patch omegaconf 830
        coverage
        pytest-cov
        numpy
diff --git a/src/hydra_zen/errors.py b/src/hydra_zen/errors.py
index a97ea923..331d737c 100644
--- a/src/hydra_zen/errors.py
+++ b/src/hydra_zen/errors.py
@@ -16,5 +16,9 @@ class HydraZenDeprecationWarning(HydraZenException, FutureWarning):
     """
 
 
-class HydraZenUnsupportedPrimitiveError(HydraZenException, ValueError):
+class HydraZenValidationError(HydraZenException):
+    pass
+
+
+class HydraZenUnsupportedPrimitiveError(HydraZenValidationError, ValueError):
     pass
diff --git a/src/hydra_zen/structured_configs/_implementations.py b/src/hydra_zen/structured_configs/_implementations.py
index 6516fa7e..855783a9 100644
--- a/src/hydra_zen/structured_configs/_implementations.py
+++ b/src/hydra_zen/structured_configs/_implementations.py
@@ -7,6 +7,7 @@ from collections import Counter, defaultdict, deque
 from dataclasses import (
     MISSING,
     Field,
+    InitVar,
     dataclass,
     field,
     fields,
@@ -42,11 +43,17 @@ from typing_extensions import Final, Literal, TypeGuard
 from hydra_zen.errors import (
     HydraZenDeprecationWarning,
     HydraZenUnsupportedPrimitiveError,
+    HydraZenValidationError,
 )
 from hydra_zen.funcs import get_obj, partial, zen_processing
 from hydra_zen.structured_configs import _utils
 from hydra_zen.typing import Builds, Importable, Just, PartialBuilds, SupportedPrimitive
-from hydra_zen.typing._implementations import DataClass, HasPartialTarget, HasTarget
+from hydra_zen.typing._implementations import (
+    DataClass,
+    HasPartialTarget,
+    HasTarget,
+    _DataClass,
+)
 
 from ._value_conversion import ZEN_SUPPORTED_PRIMITIVES, ZEN_VALUE_CONVERSION
 
@@ -399,6 +406,26 @@ def hydrated_dataclass(
         decorated_obj = cast(Any, decorated_obj)
         decorated_obj = dataclass(frozen=frozen)(decorated_obj)
 
+        if _utils.PATCH_OMEGACONF_830 and 2 < len(decorated_obj.__mro__):
+            parents = decorated_obj.__mro__[1:-1]
+            # this class inherits from a parent
+            for field_ in fields(decorated_obj):
+                if field_.default_factory is not MISSING and any(
+                    hasattr(p, field_.name) for p in parents
+                ):
+                    # TODO: update error message with fixed omegaconf version
+                    _value = field_.default_factory()
+                    raise HydraZenValidationError(
+                        "This config will not instantiatie properly.\nThis is due to a "
+                        "known bug in omegaconf: The config specifies a "
+                        f"default-factory for field {field_.name}, and inherits from a "
+                        "parent that specifies the same field with a non-factory value "
+                        "-- the parent's value will take precedence.\nTo circumvent "
+                        f"this, specify {field_.name} using: "
+                        f"`builds({type(_value).__name__}, {_value})`\n\nFor more "
+                        "information, see: https://github.com/omry/omegaconf/issues/830"
+                    )
+
         return builds(
             target,
             *pos_args,
@@ -618,13 +645,25 @@ def sanitized_field(
     *,
     error_prefix: str = "",
     field_name: str = "",
+    _mutable_default_permitted: bool = True,
 ) -> Field:
     type_value = type(value)
     if (
         type_value in _utils.KNOWN_MUTABLE_TYPES
         and type_value in HYDRA_SUPPORTED_PRIMITIVES
     ):
-        return cast(Field, mutable_value(value))
+        if _mutable_default_permitted:
+            return cast(Field, mutable_value(value))
+        else:
+            return _utils.field(
+                default=sanitized_default_value(
+                    builds(type(value), value),
+                    allow_zen_conversion=allow_zen_conversion,
+                    error_prefix=error_prefix,
+                    field_name=field_name,
+                ),
+                init=init,
+            )
     return _utils.field(
         default=sanitized_default_value(
             value,
@@ -648,7 +687,7 @@ def builds(
     hydra_recursive: Optional[bool] = None,
     hydra_convert: Optional[Literal["none", "partial", "all"]] = None,
     dataclass_name: Optional[str] = None,
-    builds_bases: Tuple[Any, ...] = (),
+    builds_bases: Tuple[Type[_DataClass], ...] = (),
     frozen: bool = False,
     **kwargs_for_target: SupportedPrimitive,
 ) -> Type[Builds[Importable]]:  # pragma: no cover
@@ -667,7 +706,7 @@ def builds(
     hydra_recursive: Optional[bool] = None,
     hydra_convert: Optional[Literal["none", "partial", "all"]] = None,
     dataclass_name: Optional[str] = None,
-    builds_bases: Tuple[Any, ...] = (),
+    builds_bases: Tuple[Type[_DataClass], ...] = (),
     frozen: bool = False,
     **kwargs_for_target: SupportedPrimitive,
 ) -> Type[PartialBuilds[Importable]]:  # pragma: no cover
@@ -686,7 +725,7 @@ def builds(
     hydra_recursive: Optional[bool] = None,
     hydra_convert: Optional[Literal["none", "partial", "all"]] = None,
     dataclass_name: Optional[str] = None,
-    builds_bases: Tuple[Any, ...] = (),
+    builds_bases: Tuple[Type[_DataClass], ...] = (),
     frozen: bool = False,
     **kwargs_for_target: SupportedPrimitive,
 ) -> Union[
@@ -706,7 +745,7 @@ def builds(
     hydra_recursive: Optional[bool] = None,
     hydra_convert: Optional[Literal["none", "partial", "all"]] = None,
     frozen: bool = False,
-    builds_bases: Tuple[Any, ...] = (),
+    builds_bases: Tuple[Type[_DataClass], ...] = (),
     dataclass_name: Optional[str] = None,
     **kwargs_for_target: SupportedPrimitive,
 ) -> Union[Type[Builds[Importable]], Type[PartialBuilds[Importable]]]:
@@ -1554,12 +1593,34 @@ def builds(
         else:
             assert len(item) == 3, item
             value = item[-1]
+            name = item[0]
 
             if not isinstance(value, Field):
                 value = sanitized_field(
                     value,
                     error_prefix=BUILDS_ERROR_PREFIX,
                     field_name=item[0],
+                    _mutable_default_permitted=_utils.mutable_default_permitted(
+                        builds_bases, name
+                    ),
+                )
+            elif (
+                _utils.PATCH_OMEGACONF_830
+                and builds_bases
+                and value.default_factory is not MISSING
+            ):
+
+                # Addresses omegaconf #830 https://github.com/omry/omegaconf/issues/830
+                #
+                # Value was passed as a field-with-default-factory, we'll
+                # access the default from the factory and will reconstruct the field
+                value = sanitized_field(
+                    value.default_factory(),
+                    error_prefix=BUILDS_ERROR_PREFIX,
+                    field_name=item[0],
+                    _mutable_default_permitted=_utils.mutable_default_permitted(
+                        builds_bases, name
+                    ),
                 )
 
             sanitized_base_fields.append((item[0], item[1], value))
@@ -1941,8 +2002,9 @@ class ZenField:
     hint: type = Any
     default: Union[SupportedPrimitive, Field, Type[NOTHING]] = NOTHING
     name: Union[str, Type[NOTHING]] = NOTHING
+    _permit_default_factory: InitVar[bool] = True
 
-    def __post_init__(self):
+    def __post_init__(self, _permit_default_factory):
         if not isinstance(self.name, str):
             if self.name is not NOTHING:
                 raise TypeError(f"`ZenField.name` expects a string, got: {self.name}")
@@ -1950,7 +2012,9 @@ class ZenField:
         self.hint = _utils.sanitized_type(self.hint)
 
         if self.default is not NOTHING:
-            self.default = sanitized_field(self.default)
+            self.default = sanitized_field(
+                self.default, _mutable_default_permitted=_permit_default_factory
+            )
 
 
 def make_config(
@@ -1959,7 +2023,7 @@ def make_config(
     hydra_convert: Optional[Literal["none", "partial", "all"]] = None,
     config_name: str = "Config",
     frozen: bool = False,
-    bases: Tuple[Type[DataClass], ...] = (),
+    bases: Tuple[Type[_DataClass], ...] = (),
     **fields_as_kwargs: Union[SupportedPrimitive, ZenField],
 ) -> Type[DataClass]:
     """
@@ -2217,13 +2281,24 @@ def make_config(
             normalized_fields[_field] = ZenField(name=_field, hint=Any)
         else:
             assert isinstance(_field.name, str)
-            normalized_fields[_field.name] = _field
+            normalized_fields[_field.name] = _repack_zenfield(
+                _field, _field.name, bases
+            )
 
     for name, value in fields_as_kwargs.items():
         if not isinstance(value, ZenField):
-            normalized_fields[name] = ZenField(name=name, default=value)
+            default_factory_permitted = (
+                not bases or _utils.mutable_default_permitted(bases, field_name=name)
+                if _utils.PATCH_OMEGACONF_830
+                else True
+            )
+            normalized_fields[name] = ZenField(
+                name=name,
+                default=value,
+                _permit_default_factory=default_factory_permitted,
+            )
         else:
-            normalized_fields[name] = value
+            normalized_fields[name] = _repack_zenfield(value, name=name, bases=bases)
 
     # fields without defaults must come first
     config_fields: List[Union[Tuple[str, type], Tuple[str, type, Any]]] = [
@@ -2270,3 +2345,22 @@ def make_config(
             cls_name=config_name, fields=config_fields, frozen=frozen, bases=bases
         ),
     )
+
+
+def _repack_zenfield(value: ZenField, name: str, bases: Tuple[_DataClass, ...]):
+    default = value.default
+
+    if (
+        _utils.PATCH_OMEGACONF_830
+        and bases
+        and not _utils.mutable_default_permitted(bases, field_name=name)
+        and isinstance(default, Field)
+        and default.default_factory is not MISSING
+    ):
+        return ZenField(
+            hint=value.hint,
+            default=default.default_factory(),
+            name=value.name,
+            _permit_default_factory=False,
+        )
+    return value
diff --git a/src/hydra_zen/structured_configs/_utils.py b/src/hydra_zen/structured_configs/_utils.py
index f3242159..2f446ca4 100644
--- a/src/hydra_zen/structured_configs/_utils.py
+++ b/src/hydra_zen/structured_configs/_utils.py
@@ -9,6 +9,7 @@ from typing import (
     Any,
     Callable,
     Dict,
+    Iterable,
     List,
     Mapping,
     Optional,
@@ -23,7 +24,9 @@ from typing import (
 from omegaconf import II
 from typing_extensions import Final, TypeGuard
 
-from hydra_zen.typing._implementations import InterpStr
+from hydra_zen.typing._implementations import InterpStr, _DataClass
+
+PATCH_OMEGACONF_830: Final[bool] = True
 
 try:
     from typing import get_args, get_origin
@@ -407,3 +410,17 @@ def check_suspicious_interpolations(
                     f"change {_w} to {_expected}"
                 )
                 yield _expected
+
+
+def mutable_default_permitted(bases: Iterable[_DataClass], field_name: str) -> bool:
+    if not PATCH_OMEGACONF_830:  # pragma: no cover
+        return True
+
+    for base in bases:
+        if (
+            field_name in base.__dataclass_fields__
+            and base.__dataclass_fields__[field_name].default is not MISSING
+        ):
+            # see https://github.com/omry/omegaconf/issues/830
+            return False
+    return True


Test patch:
diff --git a/tests/test_hydra_behaviors.py b/tests/test_hydra_behaviors.py
index 56a99fc4..1bc225f6 100644
--- a/tests/test_hydra_behaviors.py
+++ b/tests/test_hydra_behaviors.py
@@ -1,17 +1,25 @@
 # Copyright (c) 2021 Massachusetts Institute of Technology
 # SPDX-License-Identifier: MIT
 
-from dataclasses import dataclass
+from dataclasses import dataclass, field, is_dataclass
 from typing import Any, List, Tuple
 
 import hypothesis.strategies as st
 import pytest
 from hypothesis import assume, given
-from omegaconf import DictConfig, ListConfig
+from omegaconf import MISSING, DictConfig, ListConfig, OmegaConf
 from omegaconf.errors import ValidationError
 
-from hydra_zen import builds, hydrated_dataclass, instantiate, mutable_value
-from hydra_zen.structured_configs._utils import get_obj_path
+from hydra_zen import (
+    ZenField,
+    builds,
+    hydrated_dataclass,
+    instantiate,
+    make_config,
+    mutable_value,
+)
+from hydra_zen.errors import HydraZenValidationError
+from hydra_zen.structured_configs._utils import PATCH_OMEGACONF_830, get_obj_path
 
 
 def f_three_vars(x, y, z):
@@ -202,3 +210,123 @@ def test_type_checking():
 
     # should be ok
     instantiate(builds(g2, x=[conf_C, conf_C]))
+
+
+def test_PATCH_OMEGACONF_830_is_set_properly():
+    # test that PATCH_OMEGACONF_830 is True only if local version
+    # of omegaconf has known bug
+    assert isinstance(PATCH_OMEGACONF_830, bool)
+
+    @dataclass
+    class BasicConf:
+        setup: Any = 1
+
+    @dataclass
+    class Config(BasicConf):
+        setup: Any = field(default_factory=lambda: list(["hi"]))
+
+    conf = OmegaConf.structured(Config)
+    if PATCH_OMEGACONF_830:
+        assert conf.setup == 1
+    else:
+        # local version of omegaconf should have correct behavior
+        assert conf.setup == ["hi"]
+
+
+@dataclass
+class A_inheritance:
+    x: Any
+    y: Any = 1
+
+
+valid_defaults = (
+    st.none()
+    | st.booleans()
+    | st.text(alphabet="abcde")
+    | st.integers(-3, 3)
+    | st.lists(st.integers(-2, 2))
+    | st.fixed_dictionaries({"a": st.integers(-2, 2)})
+)
+
+
+def via_hydrated(x, Parent):
+    z = x if not isinstance(x, (list, dict)) else mutable_value(x)
+
+    @hydrated_dataclass(A_inheritance)
+    class Conf(Parent):
+        x: Any = z
+
+    return Conf
+
+
+def mutable_if_needed(x):
+    if isinstance(x, (dict, list)):
+        return mutable_value(x)
+    return x
+
+
+@pytest.mark.parametrize(
+    "config_maker",
+    [
+        lambda x, Parent: make_config(x=x, bases=(Parent,)),
+        lambda x, Parent: make_config(x=ZenField(Any, x), bases=(Parent,)),
+        lambda x, Parent: make_config(ZenField(Any, x, "x"), bases=(Parent,)),
+        lambda x, Parent: builds(A_inheritance, x=x, builds_bases=(Parent,)),
+        # TODO: add case where x is populated via pop-full-sig
+        # we currently support specifing fields-as-args in builds
+        lambda x, Parent: builds(
+            A_inheritance, x=mutable_if_needed(x), builds_bases=(Parent,)
+        ),
+        via_hydrated,
+    ],
+)
+@given(
+    parent_field_name=st.sampled_from(["x", "y"]),
+    parent_default=valid_defaults,
+    child_default=valid_defaults,
+)
+def test_known_inheritance_issues_in_omegaconf_are_circumvented(
+    parent_field_name, parent_default, child_default, config_maker
+):
+    # Exercises omegaconf bug documented in https://github.com/omry/omegaconf/issues/830
+    # Should pass either because:
+    #  - the test env is running a new version of omegaconf, which has patched this
+    #  - hydra-zen is providing a workaround
+    if PATCH_OMEGACONF_830 and config_maker is via_hydrated:
+        pytest.skip("hydrated_dataclass cannot support patched workaround")
+
+    assume(parent_field_name != "y" or parent_default is not MISSING)
+
+    Parent = make_config(**{parent_field_name: parent_default})
+    Child = config_maker(x=child_default, Parent=Parent)
+
+    if (
+        PATCH_OMEGACONF_830
+        and isinstance(child_default, (list, dict))
+        and not isinstance(parent_default, (list, dict))
+        and parent_field_name == "x"  # parent field overlaps
+    ):
+        # ensure we only case to dataclass when necessary
+        assert is_dataclass(Child.x)
+    else:
+        assert Child().x == child_default
+
+    Obj = instantiate(Child)
+    assert Obj.x == child_default
+
+    if parent_field_name == "y":
+        assert Obj.y == parent_default
+
+
+@pytest.mark.skipif(
+    not PATCH_OMEGACONF_830, reason="issue has been patched by omegaconf"
+)
+def test_hydrated_dataclass_raises_on_omegaconf_inheritance_issue():
+
+    Parent = make_config(x=1)
+
+    with pytest.raises(HydraZenValidationError):
+
+        @hydrated_dataclass(A_inheritance)
+        class Conf(Parent):
+            x: Any = mutable_value([1, 2])