From 3e802dcac347f25d472122b4a1d9fbbdc27dd577 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 11 Dec 2020 17:31:03 -0800 Subject: [PATCH] envorces that overrids are last in the defaults list --- hydra/_internal/new_defaults_list.py | 90 ++++++++++++------- hydra/core/new_default_element.py | 12 +++ tests/defaults_list/test_defaults_list.py | 8 +- tests/defaults_list/test_defaults_tree.py | 15 ++++ .../default_lists/override_wrong_order.yaml | 5 ++ 5 files changed, 94 insertions(+), 36 deletions(-) create mode 100644 tests/test_data/default_lists/override_wrong_order.yaml diff --git a/hydra/_internal/new_defaults_list.py b/hydra/_internal/new_defaults_list.py index 69ae116c268..13cff2c1906 100644 --- a/hydra/_internal/new_defaults_list.py +++ b/hydra/_internal/new_defaults_list.py @@ -195,7 +195,7 @@ def _validate_self(containing_node: InputDefault, defaults: List[InputDefault]) has_self = False has_none_override_items = False for d in defaults: - if isinstance(d, GroupDefault) and d.override: + if d.is_override(): continue has_none_override_items = True if d.is_self(): @@ -333,6 +333,60 @@ def _create_defaults_tree( return ret +def _update_overrides( + defaults_list: List[InputDefault], + overrides: Overrides, + parent: InputDefault, + interpolated_subtree: bool, +): + seen_override = False + last_override_seen = None + for d in defaults_list: + if d.is_self(): + continue + d.update_parent(parent.get_group_path(), parent.get_final_package()) + + if seen_override and not d.is_override(): + assert isinstance(last_override_seen, GroupDefault) + pcp = parent.get_config_path() + okey = last_override_seen.get_override_key() + oval = last_override_seen.get_name() + raise ConfigCompositionException( + dedent( + f"""\ + In {pcp}: Override '{okey} : {oval}' is defined before '{d.get_override_key()}: {d.get_name()}'. + Overrides must be at the end of the defaults list""" + ) + ) + + if isinstance(d, GroupDefault): + assert d.group is not None + legacy_hydra_override = not d.is_override() and d.group.startswith("hydra/") + if legacy_hydra_override: + d.override = True + url = "https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override" + msg = dedent( + f"""\ + In {parent.get_config_path()}: Invalid overriding of {d.group}: + Default list overrides requires 'override: true'. + See {url} for more information. + """ + ) + warnings.warn(msg, UserWarning) + + if d.override: + seen_override = True + last_override_seen = d + if interpolated_subtree: + # Since interpolations are deferred for until all the config groups are already set, + # Their subtree may not contain config group overrides + raise ConfigCompositionException( + f"{parent.get_config_path()}: Overrides are not allowed in the subtree" + f" of an in interpolated config group ({d.get_override_key()}={d.get_name()})" + ) + overrides.add_override(d) + + def _create_defaults_tree_impl( repo: IConfigRepository, root: DefaultsTreeNode, @@ -399,44 +453,14 @@ def _create_defaults_tree_impl( if len(defaults_list) > 0: self_added = _validate_self(containing_node=parent, defaults=defaults_list) - for d in defaults_list: - if d.is_self(): - continue - d.update_parent(parent.get_group_path(), parent.get_final_package()) - - if isinstance(d, GroupDefault): - assert d.group is not None - is_legacy_hydra_override = not d.override and d.group.startswith( - "hydra/" - ) - if is_legacy_hydra_override: - d.override = True - url = "https://hydra.cc/docs/next/upgrades/1.0_to_1.1/default_list_override" - msg = dedent( - f"""\ - In {parent.get_config_path()}: Invalid overriding of {d.group}: - Default list overrides requires 'override: true'. - See {url} for more information. - """ - ) - warnings.warn(msg, UserWarning) - - if d.override: - if interpolated_subtree: - # Since interpolations are deferred for until all the config groups are already set, - # Their subtree may not contain config group overrides - raise ConfigCompositionException( - f"{parent.get_config_path()}: Overrides are not allowed in the subtree" - f" of an in interpolated config group ({d.get_override_key()}={d.get_name()})" - ) - overrides.add_override(d) + _update_overrides(defaults_list, overrides, parent, interpolated_subtree) for d in reversed(defaults_list): if d.is_self(): d.update_parent(root.node.parent_base_dir, root.node.get_package()) children.append(d) else: - if isinstance(d, GroupDefault) and d.override: + if d.is_override(): continue new_root = DefaultsTreeNode(node=d, parent=root) d.update_parent(parent.get_group_path(), parent.get_final_package()) diff --git a/hydra/core/new_default_element.py b/hydra/core/new_default_element.py index 316e95e75db..75205710bb0 100644 --- a/hydra/core/new_default_element.py +++ b/hydra/core/new_default_element.py @@ -219,6 +219,9 @@ def get_override_key(self) -> str: key = f"{key}@{final_pkg}" return key + def is_override(self) -> bool: + raise NotImplementedError() + @dataclass class VirtualRoot(InputDefault): @@ -261,6 +264,9 @@ def __repr__(self) -> str: def resolve_interpolation(self, known_choices: DictConfig) -> None: raise NotImplementedError() + def is_override(self) -> bool: + return False + @dataclass(repr=False) class ConfigDefault(InputDefault): @@ -364,6 +370,9 @@ def resolve_interpolation(self, known_choices: DictConfig) -> None: def is_missing(self) -> bool: return self.get_name() == "???" + def is_override(self) -> bool: + return False + _legacy_interpolation_pattern: Pattern[str] = re.compile(r"\${defaults\.\d\.") @@ -392,6 +401,9 @@ def is_self(self) -> bool: def is_optional(self) -> bool: return self.optional + def is_override(self) -> bool: + return self.override + def get_group_path(self) -> str: assert self.parent_base_dir is not None assert self.group is not None diff --git a/tests/defaults_list/test_defaults_list.py b/tests/defaults_list/test_defaults_list.py index f0116872569..8f2cc8761fa 100644 --- a/tests/defaults_list/test_defaults_list.py +++ b/tests/defaults_list/test_defaults_list.py @@ -80,16 +80,18 @@ # - (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: Integration # - (Y) replace old defaults list computation # - (Y) enable --info=defaults output # - (Y) ensure all tests are passing -# - implement --info=defaults-tree output +# - (Y) implement --info=defaults-tree output # TODO: Followup changes # - Consider retaining the final choices in the hydra config node to allow interpolation with their values. -# - Consider enforcing that overrides are at the end of the defaults list +# - (Y) Enforce that overrides are at the end of the defaults list # (with the exception of _self_ that can be after them) +# - Consider overide style of: - override hydra/launcher: submitit +# - Fix error message when overriding a non-existing config group from the command line to not say "append with +" # TODO: (Y) rename support: diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py index f82128b588d..7abf2791454 100644 --- a/tests/defaults_list/test_defaults_tree.py +++ b/tests/defaults_list/test_defaults_tree.py @@ -581,6 +581,21 @@ def test_defaults_tree_with_package_overrides__group_override( ), id="override_nested_group_item:external_override", ), + param( + "override_wrong_order", + [], + raises( + ConfigCompositionException, + match=re.escape( + dedent( + """\ + In override_wrong_order: Override 'group1 : file2' is defined before 'group1: file1'. + Overrides must be at the end of the defaults list""" + ) + ), + ), + id="test_override_wrong_order_in_defaults_list", + ), ], ) def test_override_option_from_defaults_list( diff --git a/tests/test_data/default_lists/override_wrong_order.yaml b/tests/test_data/default_lists/override_wrong_order.yaml new file mode 100644 index 00000000000..da5d5e47db6 --- /dev/null +++ b/tests/test_data/default_lists/override_wrong_order.yaml @@ -0,0 +1,5 @@ +defaults: + # Error: overrides must come at the end of the defaults list + - group1: file2 + override: true + - group1: file1