Repository: mit-ll-responsible-ai/hydra-zen
Pull request #287 (2022-06-17 21:08:46)

Broaden overly-restrictive hydra_defaults validation
The following default lists are valid but were not permitted by hydra-zen:
- `[{"a": None}]`  (discovered in #284 )
- `[DictConfig({"a": "b"})]`
- `[make_config(a="b")]`
- `[{"a": ListConfig(["b"])]`

This PR broadens the validation performed by hydra-zen, plus updates the type-annotations and docs for `hydra_defaults`. 

The spec for Hydra's Defaults List can be found [here](https://hydra.cc/docs/next/advanced/defaults_list/)


Patch:
diff --git a/docs/source/changes.rst b/docs/source/changes.rst
index 500226b8..a6fdadc9 100644
--- a/docs/source/changes.rst
+++ b/docs/source/changes.rst
@@ -36,6 +36,20 @@ Bug Fixes
 - :func:`~hydra_zen.builds` would raise a ``TypeError`` if it encountered a target whose signature contained the annotations ``ParamSpecArgs`` or  ``ParamSpecKwargs``. It can now sanitize these annotations properly. (See :pull:`283`)
 
 
+.. _v0.7.1:
+
+---------------------
+0.7.1rc1 - 2022-05-10
+---------------------
+
+Bug Fixes
+---------
+
+The validation that hydra-zen performs on ``hydra_defaults`` was overly restrictive. E.g. it would flag ``[{"some_group": None}]`` as invalid, even though null is permitted in `Hydra's default list syntax <https://hydra.cc/docs/advanced/defaults_list/>`_.
+This patch fixes this validation and updates the docs & annotations for ``hydra_defaults`` in :func:`~hydra_zen.builds` and :func:`~hydra_zen.make_config`.
+See :pull:`287` for more details.
+
+
 .. _v0.7.0:
 
 ------------------
diff --git a/src/hydra_zen/structured_configs/_implementations.py b/src/hydra_zen/structured_configs/_implementations.py
index 192910c3..b792fdf6 100644
--- a/src/hydra_zen/structured_configs/_implementations.py
+++ b/src/hydra_zen/structured_configs/_implementations.py
@@ -765,7 +765,7 @@ def builds(
 
         If ``None``, the ``_convert_`` attribute is not set on the resulting config.
 
-    hydra_defaults : List['_self_' | Dict[str, str]] | None, optional (default = None)
+    hydra_defaults : None | list[str | dict[str, str | list[str] | None ]], optional (default = None)
         A list in an input config that instructs Hydra how to build the output config
         [7]_ [8]_. Each input config can have a Defaults List as a top level element. The
         Defaults List itself is not a part of output config.
@@ -1273,7 +1273,7 @@ def builds(
     if hydra_defaults is not None:
         if not _utils.valid_defaults_list(hydra_defaults):
             raise HydraZenValidationError(
-                f"`hydra_defaults` must be type `list[str | dict[str, str | list[str]]`"
+                f"`hydra_defaults` must be type `None | list[str | dict[str, str | list[str] | None ]]`"
                 f", Got: {repr(hydra_defaults)}"
             )
         hydra_defaults = sanitize_collection(hydra_defaults)
diff --git a/src/hydra_zen/structured_configs/_make_config.py b/src/hydra_zen/structured_configs/_make_config.py
index e7a1cd21..ac35d2b1 100644
--- a/src/hydra_zen/structured_configs/_make_config.py
+++ b/src/hydra_zen/structured_configs/_make_config.py
@@ -149,7 +149,7 @@ def make_config(
         If ``None``, the ``_convert_`` attribute is not set on the resulting config.
 
 
-    hydra_defaults : List['_self_' | Dict[str, str]] | None, optional (default = None)
+    hydra_defaults : None | list[str | dict[str, str | list[str] | None ]], optional (default = None)
         A list in an input config that instructs Hydra how to build the output config
         [7]_ [8]_. Each input config can have a Defaults List as a top level element. The
         Defaults List itself is not a part of output config.
diff --git a/src/hydra_zen/structured_configs/_utils.py b/src/hydra_zen/structured_configs/_utils.py
index 562cd0de..92565927 100644
--- a/src/hydra_zen/structured_configs/_utils.py
+++ b/src/hydra_zen/structured_configs/_utils.py
@@ -24,7 +24,8 @@ from typing import (
     overload,
 )
 
-from omegaconf import II
+
+from omegaconf import II, DictConfig, ListConfig
 from typing_extensions import (
     Annotated,
     Final,
@@ -509,7 +510,7 @@ def valid_defaults_list(hydra_defaults: Any) -> bool:
     Raises
     ------
     HydraZenValidationError: Duplicate _self_ entries"""
-    if not isinstance(hydra_defaults, list):
+    if not isinstance(hydra_defaults, (list, ListConfig)):
         return False
 
     has_self = False
@@ -523,13 +524,23 @@ def valid_defaults_list(hydra_defaults: Any) -> bool:
                 "`hydra_defaults` cannot have more than one '_self_' entry"
             )
 
-        if not isinstance(item, (dict, str)):
-            return False
-
-        if isinstance(item, dict) and any(
-            not isinstance(k, str) or not isinstance(v, (str, list))
-            for k, v in item.items()
-        ):
+        if isinstance(item, (dict, DictConfig)):
+            for k, v in item.items():
+                if not isinstance(k, str):
+                    return False
+
+                if (
+                    not isinstance(v, (str, list, ListConfig))
+                    and v is not None
+                    and v != MISSING
+                ):
+                    return False
+        elif isinstance(item, str):
+            continue
+        elif is_dataclass(item):
+            # no validation here
+            continue
+        else:
             return False
 
     if not has_self:
diff --git a/src/hydra_zen/typing/_implementations.py b/src/hydra_zen/typing/_implementations.py
index 857056cb..317a6a52 100644
--- a/src/hydra_zen/typing/_implementations.py
+++ b/src/hydra_zen/typing/_implementations.py
@@ -246,4 +246,6 @@ else:
         pass
 
 
-DefaultsList = List[Union[str, Mapping[str, Union[str, List[str]]]]]
+DefaultsList = List[
+    Union[str, DataClass_, Mapping[str, Union[None, str, Sequence[str]]]]
+]


Test patch:
diff --git a/tests/annotations/declarations.py b/tests/annotations/declarations.py
index 441ef413..359c7c8e 100644
--- a/tests/annotations/declarations.py
+++ b/tests/annotations/declarations.py
@@ -1041,5 +1041,21 @@ def check_hydra_defaults(
     make_config(hydra_defaults=1)  # type: ignore
 
     builds(int, hydra_defaults=["_self_", {"a": "b"}, {1: 1}])  # type: ignore
+    builds(int, hydra_defaults=["_self_", ["a"]])  # type: ignore
+    builds(int, hydra_defaults=["_self_", {("a",): "b"}])  # type: ignore
     builds(int, hydra_defaults={"a": "b"})  # type: ignore
     builds(int, hydra_defaults="_self_")  # type: ignore
+
+    builds(
+        int,
+        hydra_defaults=[
+            "_self_",
+            {"a": "b"},
+            {"a": None},
+            {"a": MISSING},
+            {"a": ["b"]},
+            DictConfig({"a": "b"}),
+            {"a": ListConfig(["a"])},
+            make_config(a="a"),
+        ],
+    )
diff --git a/tests/test_defaults_list.py b/tests/test_defaults_list.py
index 841712a4..fdd2244f 100644
--- a/tests/test_defaults_list.py
+++ b/tests/test_defaults_list.py
@@ -2,52 +2,82 @@
 # SPDX-License-Identifier: MIT
 
 import random
-from typing import Any
+from functools import partial
+from typing import Any, Callable, List
 
 import hypothesis.strategies as st
 import pytest
 from hydra.core.config_store import ConfigStore
 from hypothesis import given
+from omegaconf import DictConfig, ListConfig
 
-from hydra_zen import builds, instantiate, launch, make_config
+from hydra_zen import MISSING, builds, instantiate, launch, make_config
 from hydra_zen.errors import HydraZenValidationError
 
 
-def test_hydra_defaults_work_builds(version_base):
-    config_store = ConfigStore.instance()
-    config_store.store(group="x", name="a", node=builds(int, 10))
-    Conf = builds(dict, x=None, y="hi", hydra_defaults=["_self_", {"x": "a"}])
-    job = launch(Conf, instantiate, **version_base)
-    assert job.return_value == {"x": 10, "y": "hi"}
-
-
-def test_hydra_defaults_work_make_config(version_base):
+@pytest.mark.usefixtures("cleandir")
+@pytest.mark.parametrize("fn", [partial(builds, dict), make_config])
+@pytest.mark.parametrize(
+    "default,overrides",
+    [
+        ({"x": "a"}, []),
+        (DictConfig({"x": "a"}), []),
+        (make_config(x="a"), []),
+        ({"x": ["a"]}, []),
+        ({"x": ListConfig(["a"])}, []),
+        ({"x": None}, ["x=a"]),
+        ({"x": MISSING}, ["x=a"]),
+    ],
+)
+def test_hydra_defaults_work_as_expected(
+    fn: Callable, default: Any, overrides: List[str], version_base
+):
     config_store = ConfigStore.instance()
     config_store.store(group="x", name="a", node=builds(int, 10))
-    Conf = make_config(x=None, y="hi", hydra_defaults=["_self_", {"x": "a"}])
-    job = launch(Conf, instantiate, **version_base)
+    Conf = fn(x=None, y="hi", hydra_defaults=["_self_", default])
+    job = launch(Conf, instantiate, **version_base, overrides=overrides)
     assert job.return_value == {"x": 10, "y": "hi"}
 
 
 invalid_defaults = st.sampled_from(
     [1, [1], [{"a": 1}], {"a": 1}, {1: "a"}, {1: 1}, False, True]
 )
-valid_defaults = st.sampled_from(["a", {"a": "b"}, {"a": ["a", "b"]}])
+valid_defaults = st.sampled_from(
+    ["a", {"a": "b"}, {"a": None}, {"a": MISSING}, {"a": ["a", "b"]}]
+)
+
+
+@pytest.mark.filterwarnings("ignore::UserWarning")
+@given(
+    defaults=st.lists(valid_defaults, min_size=0),
+    include_self=st.sampled_from([None, "pre", "post"]),
+)
+def test_hydra_defaults_validation_passes_on_good_values(defaults: Any, include_self):
+    if include_self == "pre":
+        defaults = ["_self_"] + defaults
+    elif include_self == "post":
+        defaults.append("_self_")
+
+    builds(dict, hydra_defaults=defaults)
+    make_config(hydra_defaults=defaults)
 
 
+@pytest.mark.usefixtures("cleandir")
 @pytest.mark.filterwarnings("ignore::UserWarning")
 @given(
     invalids=invalid_defaults | st.lists(invalid_defaults, min_size=1),
     valids=st.lists(valid_defaults, min_size=0),
     include_self=st.sampled_from([None, "pre", "post"]),
 )
-def test_hydra_defaults_validation(invalids: Any, valids: list, include_self):
+def test_hydra_defaults_validation_catches_bad_values(
+    invalids: Any, valids: list, include_self
+):
     if isinstance(invalids, list):
         defaults = invalids + valids
         random.shuffle(defaults)
         if include_self == "pre":
             defaults = ["_self_"] + defaults
-        elif include_self == "pose":
+        elif include_self == "post":
             defaults.append("_self_")
     else:
         defaults = invalids
@@ -88,3 +118,16 @@ def test_no_self_in_defaults_warns():
 def test_redundant_defaults_in_make_config_raises():
     with pytest.raises(TypeError):
         make_config(defaults=["_self_"], hydra_defaults=["_self_"])
+
+
+def test_regression_284():
+    # https://github.com/mit-ll-responsible-ai/hydra-zen/issues/284
+    make_config(
+        hydra_defaults=[
+            "_self_",
+            {"model": "resnet"},
+            {"decorators_model": None},
+            {"decorators_model2": MISSING},
+            {"test_transform": "test_transformation_6ch"},
+        ]
+    )