Repository: mit-ll-responsible-ai/hydra-zen
Pull request #422 (2023-03-04 16:44:52)

Emit warning when `store(node=<config>)` is used. 
Hydra's config store API uses the `node` kwarg to specify the to-be-stored config. hydra-zen's store relied on a positional argument for indicating the config (so that our self-partialing mechanic can type-check). This discrepancy can lead to subtle, elusive user errors. See #421 

Thus this PR will emit a warning when the pattern `store(node=<config>, ...)`.


Patch:
diff --git a/src/hydra_zen/wrapper/_implementations.py b/src/hydra_zen/wrapper/_implementations.py
index 5dd43776..113df7c7 100644
--- a/src/hydra_zen/wrapper/_implementations.py
+++ b/src/hydra_zen/wrapper/_implementations.py
@@ -1134,6 +1134,7 @@ class ZenStore:
         "_deferred_to_config",
         "_deferred_store",
         "_overwrite_ok",
+        "_warn_node_kwarg",
     )
 
     def __init__(
@@ -1143,6 +1144,7 @@ class ZenStore:
         deferred_to_config: bool = True,
         deferred_hydra_store: bool = True,
         overwrite_ok: bool = False,
+        warn_node_kwarg: bool = True,
     ) -> None:
         """
         Parameters
@@ -1150,18 +1152,26 @@ class ZenStore:
         name : Optional[str]
             The name for this store.
 
-        deferred_to_config : bool, optional (default=True)
+        deferred_to_config : bool, default=True
             If `True` (default), this store will a not apply `to_config` to the
             target until that specific entry is accessed by the store.
 
-        deferred_hydra_store : bool, optional (default=True)
+        deferred_hydra_store : bool, default=True
             If `True` (default), this store will not add entries to Hydra's global
             config store until `store.add_to_hydra_store` is called explicitly.
 
-        overwrite_ok : bool, optional (default=False)
+        overwrite_ok : bool, default=False
             If `False` (default), attempting to overwrite entries in this store and
             trying to use this store to overwrite entries in Hydra's global store
             will raise a `ValueError`.
+
+        warn_node_kwarg: bool, default=True
+            If `True` specifying a `node` kwarg in `ZenStore.__call__` will emit a
+            warning.
+
+            This helps to protect users from mistakenly self-partializing a store
+            with `store(node=Config)` instead of actually storing the node with
+            `store(Config)`.
         """
         if not isinstance(deferred_to_config, bool):  # type: ignore
             raise TypeError(
@@ -1190,6 +1200,8 @@ class ZenStore:
         # Contains the current default arguments for `self.__call__`
         self._defaults: _StoreCallSig = defaults.copy()
 
+        self._warn_node_kwarg = warn_node_kwarg
+
     def __repr__(self) -> str:
         # TODO: nicer repr?
         groups_contents: DefaultDict[Optional[str], List[str]] = defaultdict(list)
@@ -1301,11 +1313,20 @@ class ZenStore:
             has updated default arguments.
         """
         if __target is None:
+            if self._warn_node_kwarg and "node" in kw:
+                warnings.warn(
+                    "hydra-zen's store API does not use the `node` keyword. To store a config, specify it as a positional argument: `store(<config>)`.\n\nIf the use of `node` was intentional, you can suppress this warning by using a store that is initialized via `ZenStore(warn_node_kwarg=False)."
+                )
+                # warnings.warn(
+                #     f"\nCalling `store(node=<config>, ...)` does not store a config. The appropriate syntax is to use a positional argument:\n\t`store(node=<config>)` -> `store(<config>)`.\nIf this was intentional, you can suppress this warning by using a store initialized via `ZenStore(warn_node_kwarg=False)."
+                # )
+
             _s = type(self)(
                 self.name,
                 deferred_to_config=self._deferred_to_config,
                 deferred_hydra_store=self._deferred_store,
                 overwrite_ok=self._overwrite_ok,
+                warn_node_kwarg=self._warn_node_kwarg,
             )
             _s._defaults = self._defaults.copy()
 


Test patch:
diff --git a/tests/test_launch/test_dict_overrides.py b/tests/test_launch/test_dict_overrides.py
index bc222411..8548813d 100644
--- a/tests/test_launch/test_dict_overrides.py
+++ b/tests/test_launch/test_dict_overrides.py
@@ -17,6 +17,7 @@ from hydra_zen import (
     make_config,
     multirun,
 )
+from hydra_zen._compatibility import HYDRA_VERSION, Version
 from hydra_zen._launch import value_check
 
 any_types = st.from_type(type)
@@ -84,7 +85,9 @@ def test_overrides_roundtrip(
         "+list_": hydra_list(list_),
         "+mrun": multirun(mrun),
     }
-    (jobs,) = launch(make_config(), instantiate, overrides, multirun=True)
+    (jobs,) = launch(
+        make_config(), instantiate, overrides, multirun=True, version_base="1.2"
+    )
 
     assert len(jobs) == len(mrun)
     for i, job in enumerate(jobs):
@@ -132,8 +135,22 @@ def test_overrides_kwargs_roundtrip(
         "+list_": hydra_list(list_),
         "+mrun": multirun(mrun),
     }
-    jobs = launch(make_config(), instantiate, ["+foo=1"], multirun=True, **overrides)
-    jobs1 = launch(make_config(), instantiate, {"+foo": 2}, multirun=True, **overrides)
+    jobs = launch(
+        make_config(),
+        instantiate,
+        ["+foo=1"],
+        multirun=True,
+        **overrides,
+        version_base="1.2",
+    )
+    jobs1 = launch(
+        make_config(),
+        instantiate,
+        {"+foo": 2},
+        multirun=True,
+        **overrides,
+        version_base="1.2",
+    )
 
     assert len(jobs[0]) == len(mrun)
     assert len(jobs1[0]) == len(mrun)
@@ -180,7 +197,12 @@ def test_overrides_yaml(
         "+list_": hydra_list(list_),
         "+mrun": multirun(mrun),
     }
-    (jobs,) = launch(make_config(), instantiate, overrides, multirun=True)
+    if HYDRA_VERSION >= Version(1, 2, 0):
+        overrides["hydra.job.chdir"] = True
+
+    (jobs,) = launch(
+        make_config(), instantiate, overrides, multirun=True, version_base="1.2"
+    )
 
     assert len(jobs) == len(mrun)
     for i, job in enumerate(jobs):
diff --git a/tests/test_store.py b/tests/test_store.py
index 699788a4..fd3853c2 100644
--- a/tests/test_store.py
+++ b/tests/test_store.py
@@ -789,3 +789,9 @@ def test_configure_hydra_chdir(inp: str):
     if inp:
         cli.append(inp)
     subprocess.run(cli).check_returncode()
+
+
+def test_node_warns():
+    store = ZenStore(warn_node_kwarg=True)
+    with pytest.warns(UserWarning):
+        store(node=builds(int))