Error handling

hydra/_internal/new_defaults_list.py

@@ -4,7 +4,7 @@
 import warnings
 from dataclasses import dataclass, field
 from textwrap import dedent
-from typing import Callable, Dict, List, Optional, Union
+from typing import Callable, Dict, List, Optional, Set, Union
 
 from omegaconf import DictConfig, OmegaConf
 
@@ -38,6 +38,7 @@ class Overrides:
     config_overrides: List[Override]
 
     known_choices: Dict[str, Optional[str]]
+    known_choices_per_group: Dict[str, Set[str]]
 
     deletions: Dict[str, Deletion]
 
@@ -49,6 +50,7 @@ def __init__(self, repo: IConfigRepository, overrides_list: List[Override]) -> N
         self.deletions = {}
 
         self.known_choices = {}
+        self.known_choices_per_group = {}
 
         for override in overrides_list:
             is_group = repo.group_exists(override.key_or_group)
@@ -92,7 +94,6 @@ def add_override(self, default: GroupDefault) -> None:
 
     def is_overridden(self, default: InputDefault) -> bool:
         if isinstance(default, GroupDefault):
-            # TODO: collect overridable keys for help/useful error purposes
             return default.get_override_key() in self.override_choices
 
         return False
@@ -107,11 +108,31 @@ def override_default_option(self, default: GroupDefault) -> None:
     def ensure_overrides_used(self) -> None:
         for key, used in self.override_used.items():
             if not used:
-                msg = dedent(
-                    f"""\
-                    Could not override '{key}'. No match in the defaults list.
-                    To append to your default list use +{key}={self.override_choices[key]}"""
+                group = key.split("@")[0]
+                choices = (
+                    self.known_choices_per_group[group]
+                    if group in self.known_choices_per_group
+                    else set()
                 )
+                if len(choices) > 1:
+                    msg = (
+                        f"Could not override '{key}'."
+                        f"\nDid you mean to override one of {', '.join(sorted(list(choices)))}?"
+                        f"\nTo append to your default list use +{key}={self.override_choices[key]}"
+                    )
+                elif len(choices) == 1:
+                    msg = (
+                        f"Could not override '{key}'."
+                        f"\nDid you mean to override {copy.copy(choices).pop()}?"
+                        f"\nTo append to your default list use +{key}={self.override_choices[key]}"
+                    )
+                elif len(choices) == 0:
+                    msg = (
+                        f"Could not override '{key}'. No match in the defaults list."
+                        f"\nTo append to your default list use +{key}={self.override_choices[key]}"
+                    )
+                else:
+                    assert False
                 raise ConfigCompositionException(msg)
 
     def ensure_deletions_used(self) -> None:
@@ -129,10 +150,16 @@ def set_known_choice(self, default: InputDefault) -> None:
             else:
                 prev = self.known_choices[key]
                 if default.get_name() != prev:
-                    raise ValueError(
-                        f"Internal error, value of {key} is being changed from {prev} to {default.get_name()}"
+                    raise ConfigCompositionException(
+                        f"Multiple values for {key} ({prev}, {default.get_name()})."
+                        " To override a value use 'override: true'"
                     )
 
+            group = default.get_group_path()
+            if group not in self.known_choices_per_group:
+                self.known_choices_per_group[group] = set()
+            self.known_choices_per_group[group].add(key)
+
     def is_deleted(self, default: InputDefault) -> bool:
         if not isinstance(default, GroupDefault):
             return False
@@ -344,7 +371,7 @@ def _create_defaults_tree_impl(
         loaded = repo.load_config(config_path=path, is_primary_config=is_primary_config)
 
         if loaded is None:
-            config_not_found_error(repo, root.node)
+            config_not_found_error(repo, root)
 
         assert loaded is not None
         defaults_list = copy.deepcopy(loaded.new_defaults_list)
@@ -373,7 +400,9 @@ def _create_defaults_tree_impl(
                     url = "https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override"
                     msg = dedent(
                         f"""\
-                        Default list overrides now requires 'override: true', see {url} for more information.
+                        Invalid overriding of {d.group}:
+                        Default list overrides requires 'override: true'.
+                        See {url} for more information.
                         """
                     )
                     warnings.warn(msg, UserWarning)
@@ -466,6 +495,7 @@ def _create_result_default(
         if tree is not None:
             res.parent = tree.node.get_config_path()
         res.package = node.get_final_package()
+        res.override_key = node.get_override_key()
     return res
 
 
@@ -520,6 +550,18 @@ def _create_root(config_name: str, with_hydra: bool) -> DefaultsTreeNode:
     return root
 
 
+def ensure_no_duplicates_in_list(result: List[ResultDefault]) -> None:
+    keys = set()
+    for item in result:
+        if not item.is_self:
+            key = item.override_key
+            if key in keys:
+                raise ConfigCompositionException(
+                    f"{key} appears more than once in the final defaults list"
+                )
+            keys.add(key)
+
+
 def _create_defaults_list(
     repo: IConfigRepository,
     config_name: str,
@@ -540,7 +582,7 @@ def _create_defaults_list(
     )
 
     output = _tree_to_list(tree=defaults_tree)
-    # TODO: fail if duplicate items exists
+    ensure_no_duplicates_in_list(output)
     return output
 
 
@@ -573,24 +615,28 @@ def create_defaults_list(
     return ret
 
 
-# TODO: show parent config name in the error (where is my error?)
-def config_not_found_error(repo: IConfigRepository, element: InputDefault) -> None:
+def config_not_found_error(repo: IConfigRepository, tree: DefaultsTreeNode) -> None:
+    element = tree.node
+    parent = tree.parent.node if tree.parent is not None else None
     options = None
     if isinstance(element, GroupDefault):
         group = element.get_group_path()
         options = repo.get_group_options(group, ObjectType.CONFIG)
-        opt_list = "\n".join(["\t" + x for x in options])
-        msg = (
-            f"Could not find '{element.name}' in the config group '{group}'"
-            f"\nAvailable options:\n{opt_list}\n"
-        )
+
+        msg = f"Could not find '{element.get_config_path()}'\n"
+        if len(options) > 0:
+            opt_list = "\n".join(["\t" + x for x in options])
+            msg = f"{msg}\nAvailable options in '{group}':\n" + opt_list
     else:
         msg = dedent(
             f"""\
         Could not load '{element.get_config_path()}'.
         """
         )
 
+    if parent is not None:
+        msg = f"In '{parent.get_config_path()}': {msg}"
+
     descs = []
     for src in repo.get_sources():
         descs.append(f"\t{repr(src)}")

hydra/core/new_default_element.py

@@ -15,6 +15,8 @@ class ResultDefault:
     package: Optional[str] = None
     is_self: bool = False
 
+    override_key: Optional[str] = field(default=None, compare=False)
+
     def __repr__(self) -> str:
         attrs = []
         attr_names = "config_path", "package", "parent"

tests/defaults_list/test_defaults_list.py

@@ -72,29 +72,31 @@
 #  - (Y) Support delete by group=value
 #  - (Y) Test deletion with a specific package
 #  - (Y) Deletion test with final defaults list
-# TODO: (Y) rename support:
-#  - Remove rename support form 1.1
-#  - Deprecate rename support in 1.0
-
-
 # TODO: Error handling:
-#  - Error handling for entries that failed to override anything
-#  - Error if delete override did not delete anything
-#  - Duplicate _self_ error
-#  - test handling missing configs mentioned in defaults list (with and without optional)
-#  - Ambiguous overrides should provide valid override keys for group
-#  - Test deprecation message when attempting to override hydra configs without override: true
-#  - Should duplicate entries in results list be an error? (same override key)
+#  - (Y) Error handling for overrides that did not match any config group in the defaults list
+#  - (Y) Error if delete override did not delete anything
+#  - (Y) Duplicate _self_ error
+#  - (Y) test handling missing configs mentioned in defaults list
+#  - (Y) Ambiguous overrides should provide valid override keys for group
+#  - (Y) Test deprecation message when attempting to override hydra configs without override: true
+#  - (Y) duplicate entries in final defaults list raises an error
 
 
 # TODO: Integrate with Hydra
 #  - replace old defaults list computation
 #  - enable --info=defaults output
 #  - ensure all tests are passing
 
+# TODO: (Y) rename support:
+#  - Remove rename support form 1.1
+#  - Deprecate rename support in 1.0
+
+
 # TODO: cleanup
+#  - Remove previous implementation of recursive defaults list and rename new_defaults_list to defaults_list etc.
 #  - Clean up package logic from config sources
 #  - Clean up Hydra 1.0 warnings related to package header
+#  - Delete tests and test data of old defaults list impl
 
 
 # TODO Documentation
@@ -598,7 +600,8 @@ def test_include_nested_group_pkg2(
                 match=re.escape(
                     dedent(
                         """\
-                        Could not override 'group1@wrong'. No match in the defaults list.
+                        Could not override 'group1@wrong'.
+                        Did you mean to override group1@pkg1?
                         To append to your default list use +group1@wrong=file2"""
                     )
                 ),
@@ -1116,7 +1119,7 @@ def test_overriding_package_header_from_defaults_list(
             id="just_hydra_config",
         ),
         param(
-            "override_hydra",
+            "legacy_override_hydra",
             [],
             [
                 ResultDefault(
@@ -1138,7 +1141,7 @@ def test_overriding_package_header_from_defaults_list(
                     is_self=False,
                 ),
                 ResultDefault(
-                    config_path="override_hydra",
+                    config_path="legacy_override_hydra",
                     parent="<root>",
                     package="",
                     is_self=True,
@@ -1409,3 +1412,40 @@ def test_deletion(
         overrides=overrides,
         expected=expected,
     )
+
+
+@mark.parametrize(  # type: ignore
+    "config_name,overrides,expected",
+    [
+        param(
+            "error_duplicate_group",
+            [],
+            raises(
+                ConfigCompositionException,
+                match=re.escape(
+                    "group1 appears more than once in the final defaults list"
+                ),
+            ),
+            id="error_duplicate_group",
+        ),
+        param(
+            "error_duplicate_group_nested",
+            [],
+            raises(
+                ConfigCompositionException,
+                match=re.escape(
+                    "group1/group2 appears more than once in the final defaults list"
+                ),
+            ),
+            id="error_duplicate_group_nested",
+        ),
+    ],
+)
+def test_duplicate_items(
+    config_name: str, overrides: List[str], expected: List[ResultDefault]
+) -> None:
+    _test_defaults_list_impl(
+        config_name=config_name,
+        overrides=overrides,
+        expected=expected,
+    )