From 5388f5fdb5bb567223ff965a56f5e318bd588f9f Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Thu, 3 Dec 2020 16:46:28 -0800 Subject: [PATCH] Error handling --- hydra/_internal/new_defaults_list.py | 82 +++-- hydra/core/new_default_element.py | 2 + tests/defaults_list/test_defaults_list.py | 70 ++++- tests/defaults_list/test_defaults_tree.py | 282 ++++++++++++++++-- .../default_lists/duplicate_self.yaml | 5 + .../default_lists/error_changing_group.yaml | 3 + .../default_lists/error_duplicate_group.yaml | 3 + .../error_duplicate_group_nested.yaml | 3 + .../default_lists/error_invalid_override.yaml | 3 + ..._hydra.yaml => legacy_override_hydra.yaml} | 0 10 files changed, 387 insertions(+), 66 deletions(-) create mode 100644 tests/test_data/default_lists/duplicate_self.yaml create mode 100644 tests/test_data/default_lists/error_changing_group.yaml create mode 100644 tests/test_data/default_lists/error_duplicate_group.yaml create mode 100644 tests/test_data/default_lists/error_duplicate_group_nested.yaml create mode 100644 tests/test_data/default_lists/error_invalid_override.yaml rename tests/test_data/default_lists/{override_hydra.yaml => legacy_override_hydra.yaml} (100%) diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index d49a7d9cb20..832ca06a956 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/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,17 +615,18 @@ 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"""\ @@ -591,6 +634,9 @@ def config_not_found_error(repo: IConfigRepository, element: InputDefault) -> No """ ) + 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)}") diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 1cff1cc0890..f4cf695b046 100644 --- a/hydra/core/new_default_element.py +++ b/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" diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index 311b7453b09..ed5d83d8c64 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -72,19 +72,14 @@ # - (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 @@ -92,9 +87,16 @@ # - 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="", 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, + ) diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index 623e2438953..795d96b7f13 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -416,7 +416,8 @@ def test_defaults_tree_with_package_overrides( 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""" ) ), @@ -449,7 +450,8 @@ def test_defaults_tree_with_package_overrides( match=re.escape( dedent( """\ - Could not override 'group1/group2'. No match in the defaults list. + Could not override 'group1/group2'. + Did you mean to override group1/group2@group1.pkg2? To append to your default list use +group1/group2=file2""" ) ), @@ -649,26 +651,7 @@ def test_two_group_defaults_different_pkgs( "config_name,overrides,expected", [ param( - "empty", - [], - DefaultsTreeNode( - node=VirtualRoot(), - children=[ - DefaultsTreeNode( - node=ConfigDefault(path="hydra/config"), - children=[ - ConfigDefault(path="_self_"), - GroupDefault(group="help", name="default"), - GroupDefault(group="output", name="default"), - ], - ), - ConfigDefault(path="empty"), - ], - ), - id="just_hydra", - ), - param( - "override_hydra", + "legacy_override_hydra", [], DefaultsTreeNode( node=VirtualRoot(), @@ -682,15 +665,15 @@ def test_two_group_defaults_different_pkgs( ], ), DefaultsTreeNode( - node=ConfigDefault(path="override_hydra"), + node=ConfigDefault(path="legacy_override_hydra"), children=[ConfigDefault(path="_self_")], ), ], ), - id="override_hydra", + id="legacy_override_hydra", ), param( - "override_hydra", + "legacy_override_hydra", ["hydra/help=custom2"], DefaultsTreeNode( node=VirtualRoot(), @@ -704,12 +687,12 @@ def test_two_group_defaults_different_pkgs( ], ), DefaultsTreeNode( - node=ConfigDefault(path="override_hydra"), + node=ConfigDefault(path="legacy_override_hydra"), children=[ConfigDefault(path="_self_")], ), ], ), - id="override_hydra+external", + id="legacy_override_hydra+external", ), ], ) @@ -717,14 +700,20 @@ def test_legacy_hydra_overrides_from_primary_config( config_name: str, overrides: List[str], expected: DefaultsTreeNode, - recwarn: Any, # Testing deprecated behavior ) -> None: - _test_defaults_tree_impl( - config_name=config_name, - input_overrides=overrides, - expected=expected, - prepend_hydra=True, + msg = dedent( + """\ + Invalid overriding of hydra/help: + Default list overrides requires 'override: true'. + See https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override for more information.""" ) + with warns(expected_warning=UserWarning, match=re.escape(msg)): + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + prepend_hydra=True, + ) @mark.parametrize( # type: ignore @@ -1686,3 +1675,230 @@ def test_deletion( input_overrides=overrides, expected=expected, ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "empty", + ["~group1"], + raises( + ConfigCompositionException, + match="Could not delete 'group1'. No match in the defaults list", + ), + id="override_non_existing", + ), + param( + "empty", + ["~group1=abc"], + raises( + ConfigCompositionException, + match="Could not delete 'group1=abc'. No match in the defaults list", + ), + id="override_non_existing", + ), + param( + "empty", + ["~group1@pkg1=abc"], + raises( + ConfigCompositionException, + match="Could not delete 'group1@pkg1=abc'. No match in the defaults list", + ), + id="override_non_existing", + ), + ], +) +def test_delete_non_existing( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "duplicate_self", + [], + raises( + ConfigCompositionException, + match="Duplicate _self_ defined in duplicate_self", + ), + id="duplicate_self", + ), + ], +) +def test_duplicate_self_error( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "not_found", + [], + raises( + ConfigCompositionException, + match="^Could not load 'not_found'.*", + ), + id="missing_primary", + ), + param( + "empty", + ["+group1=not_found"], + raises( + ConfigCompositionException, + match="^In 'empty': Could not find 'group1/not_found'\n\nAvailable options in 'group1':", + ), + id="missing_included_config", + ), + param( + "empty", + ["+group1=not_found"], + raises( + ConfigCompositionException, + match="^In 'empty': Could not find 'group1/not_found'\n\nAvailable options in 'group1':", + ), + id="missing_included_config", + ), + ], +) +def test_missing_config_errors( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "empty", + ["group1=file1"], + raises( + ConfigCompositionException, + match="Could not override 'group1'. No match in the defaults list.", + ), + id="override_non_existing", + ), + param( + "empty", + ["group1@pkg1=file1"], + raises( + ConfigCompositionException, + match="Could not override 'group1@pkg1'. No match in the defaults list.", + ), + id="override_non_existing", + ), + param( + "empty", + ["group1/group2=file1"], + raises( + ConfigCompositionException, + match="Could not override 'group1/group2'. No match in the defaults list.", + ), + id="override_non_existing", + ), + param( + "error_invalid_override", + [], + raises( + ConfigCompositionException, + match="Could not override 'group1'. No match in the defaults list.", + ), + id="error_invalid_override", + ), + param( + "group_default", + ["group1@foo=file1"], + raises( + ConfigCompositionException, + match=re.escape( + dedent( + """\ + Could not override 'group1@foo'. + Did you mean to override group1? + To append to your default list use +group1@foo=file1""" + ) + ), + ), + id="no_match_package_one_candidate", + ), + param( + "two_group_defaults_different_pkgs", + ["group1@foo=file1"], + raises( + ConfigCompositionException, + match=re.escape( + dedent( + """\ + Could not override 'group1@foo'. + Did you mean to override one of group1@pkg1, group1@pkg2? + To append to your default list use +group1@foo=file1""" + ) + ), + ), + id="no_match_package_multiple_candidates", + ), + ], +) +def test_override_errors( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) + + +@mark.parametrize( # type: ignore + "config_name,overrides,expected", + [ + param( + "error_changing_group", + [], + raises( + ConfigCompositionException, + match=re.escape( + "Multiple values for group1 (file2, file1). To override a value use 'override: true'" + ), + ), + id="error_changing_group", + ), + ], +) +def test_error_changing_group_choice( + config_name: str, + overrides: List[str], + expected: DefaultsTreeNode, +) -> None: + _test_defaults_tree_impl( + config_name=config_name, + input_overrides=overrides, + expected=expected, + ) diff --git a/tests/test_data/default_lists/duplicate_self.yaml b/tests/test_data/default_lists/duplicate_self.yaml new file mode 100644 index 00000000000..844137e9bfb --- /dev/null +++ b/tests/test_data/default_lists/duplicate_self.yaml @@ -0,0 +1,5 @@ +# @package _global_ +defaults: + - _self_ + - empty + - _self_ diff --git a/tests/test_data/default_lists/error_changing_group.yaml b/tests/test_data/default_lists/error_changing_group.yaml new file mode 100644 index 00000000000..692288eb248 --- /dev/null +++ b/tests/test_data/default_lists/error_changing_group.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: file1 + - group1: file2 \ No newline at end of file diff --git a/tests/test_data/default_lists/error_duplicate_group.yaml b/tests/test_data/default_lists/error_duplicate_group.yaml new file mode 100644 index 00000000000..2b20b0829e9 --- /dev/null +++ b/tests/test_data/default_lists/error_duplicate_group.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: file1 + - group1: file1 diff --git a/tests/test_data/default_lists/error_duplicate_group_nested.yaml b/tests/test_data/default_lists/error_duplicate_group_nested.yaml new file mode 100644 index 00000000000..ba9547549f2 --- /dev/null +++ b/tests/test_data/default_lists/error_duplicate_group_nested.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: group_item1 + - group1/group2: file1 diff --git a/tests/test_data/default_lists/error_invalid_override.yaml b/tests/test_data/default_lists/error_invalid_override.yaml new file mode 100644 index 00000000000..5518174f58d --- /dev/null +++ b/tests/test_data/default_lists/error_invalid_override.yaml @@ -0,0 +1,3 @@ +defaults: + - group1: file + override: true diff --git a/tests/test_data/default_lists/override_hydra.yaml b/tests/test_data/default_lists/legacy_override_hydra.yaml similarity index 100% rename from tests/test_data/default_lists/override_hydra.yaml rename to tests/test_data/default_lists/legacy_override_hydra.yaml