envorces that overrids are last in the defaults list

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())

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

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:

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(

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