Repository: mit-ll-responsible-ai/hydra-zen
Pull request #174 (2021-12-03 20:10:24)

Change behavior of pop-full-sig to take priority over inherited fields
This PR changes the behavior of `builds` when `populate_full_signature=True` and one or more base-classes are specified for inheritance. **Notes that this a compatibility-breaking change**. Although the old behavior was likely not what one wanted/expected.

Previously, fields specified by the parent class would take priority over those that would be auto-populated:

```python
from hydra_zen import builds, make_config
    
def f(x=1):
    pass

Parent = make_config(x="hi")
```

**Before**

```python
>>> builds(f, populate_full_signature=True, builds_bases=(Parent,))  # x comes from parent
Builds_f(x='hi', _target_='__main__.f')

>>> builds(f, x=1, builds_bases=(Parent,))() 
Builds_f(x=1, _target_='__main__.f')
```

However, this behavior is unintuitive, as `populate_full_signature=True` should behave identically as the case where one manually-specifies the arguments from a target's signature. Thus we have have changed this behavior such that auto-populating and manually-populating fields from a signature always produces the same behavior:

**After**

```python
>>> builds(f, populate_full_signature=True, builds_bases=(Parent,))  # parent no longer takes priority
Builds_f(x=1, _target_='__main__.f')

>>> builds(f, x=1, builds_bases=(Parent,))() 
Builds_f(x=1, _target_='__main__.f')
```


Patch:
diff --git a/src/hydra_zen/structured_configs/_implementations.py b/src/hydra_zen/structured_configs/_implementations.py
index 855783a9..1f1f9c23 100644
--- a/src/hydra_zen/structured_configs/_implementations.py
+++ b/src/hydra_zen/structured_configs/_implementations.py
@@ -416,7 +416,7 @@ def hydrated_dataclass(
                     # 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 "
+                        "This config will not instantiate 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 "
@@ -426,9 +426,23 @@ def hydrated_dataclass(
                         "information, see: https://github.com/omry/omegaconf/issues/830"
                     )
 
+        if populate_full_signature:
+            # we need to ensure that the fields specified via the class definition
+            # take precedence over the fields that will be auto-populated by builds
+            kwargs = {
+                f.name: f.default if f.default is not MISSING else f.default_factory()
+                for f in fields(decorated_obj)
+                if not (f.default is MISSING and f.default_factory is MISSING)
+                and f.name not in _HYDRA_FIELD_NAMES
+                and not f.name.startswith("_zen_")
+            }
+        else:
+            kwargs = {}
+
         return builds(
             target,
             *pos_args,
+            **kwargs,
             populate_full_signature=populate_full_signature,
             hydra_recursive=hydra_recursive,
             hydra_convert=hydra_convert,
@@ -1504,9 +1518,6 @@ def builds(
                     user_specified_named_params[param.name]
                 )
                 _seen.add(param.name)
-            elif param.name in fields_set_by_bases:
-                # don't populate a parameter that can be derived from a base
-                continue
             else:
                 # any parameter whose default value is None is automatically
                 # annotated with `Optional[...]`. This improves flexibility with


Test patch:
diff --git a/tests/test_signature_parsing.py b/tests/test_signature_parsing.py
index 56c8b802..aff19ce6 100644
--- a/tests/test_signature_parsing.py
+++ b/tests/test_signature_parsing.py
@@ -9,10 +9,17 @@ from typing import Any, Callable, Dict, Mapping, Optional, Tuple, Type
 
 import hypothesis.strategies as st
 import pytest
-from hypothesis import given, settings
+from hypothesis import given, note, settings
 from omegaconf.errors import ValidationError
 
-from hydra_zen import builds, hydrated_dataclass, instantiate, mutable_value, to_yaml
+from hydra_zen import (
+    builds,
+    hydrated_dataclass,
+    instantiate,
+    make_config,
+    mutable_value,
+    to_yaml,
+)
 from hydra_zen.typing import Just
 from tests import valid_hydra_literals
 
@@ -319,3 +326,50 @@ def test_setting_default_with_Builds_widens_type(builds_as_default, hydra_recurs
     with pytest.raises(ValidationError):
         # ensure that type annotation is broadened only when hydra_recursive=False
         instantiate(builds(expects_int, x=builds_as_default, hydra_recursive=False))
+
+
+def func_with_various_defaults(x=1, y="a", z=[1, 2]):
+    pass
+
+
+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)})
+)
+
+
+@given(
+    passed_args=st.fixed_dictionaries(
+        {}, optional=dict(x=st.just(1), y=st.just("a"), z=st.just([1, 2]))
+    ),
+    parent=st.just(())
+    | st.dictionaries(st.sampled_from(["x", "y", "z"]), valid_defaults).map(
+        lambda kw: (make_config(**kw),)
+    ),
+)
+def test_pop_full_sig_is_always_identical_to_manually_specifying_sig_args(
+    passed_args: dict, parent: tuple
+):
+    Conf = builds(
+        func_with_various_defaults,
+        x=1,
+        y="a",
+        z=[1, 2],
+        populate_full_signature=False,
+        builds_bases=parent,
+    )
+    ConfWithPopSig = builds(
+        func_with_various_defaults,
+        **passed_args,
+        builds_bases=parent,
+        populate_full_signature=True,
+    )
+    if parent:
+        note(str(parent[0]()))
+    note(to_yaml(Conf))
+    note(to_yaml(ConfWithPopSig))
+    assert to_yaml(Conf) == to_yaml(ConfWithPopSig)