Fixed +experinent=foo causing unexpected changes to defaults list

[omry]

This is undoing a previous bug fix for a related problem that attempted to insert the external overrides in the right place.
The idea behind forcing the order is just to communicate to the user that overrides are special and applies to what comes before. In practice the implementation should work correctly even if they are not.

The solution introduced here is to exempt external overrides from the order check.

Closes #1706

[Jasha10]

The approach I took lets overrides come before non-overrides as long as they have a different override key:

$ git diff master
diff --git a/hydra/_internal/defaults_list.py b/hydra/_internal/defaults_list.py
index 0888952f7..e9026f8e5 100644
--- a/hydra/_internal/defaults_list.py
+++ b/hydra/_internal/defaults_list.py
@@ -352,8 +352,7 @@ def _update_overrides(
     parent: InputDefault,
     interpolated_subtree: bool,
 ) -> None:
-    seen_override = False
-    last_override_seen = None
+    last_overrides_seen: Dict[str, GroupDefault] = {}
     for d in defaults_list:
         if d.is_self():
             continue
@@ -364,16 +363,18 @@ def _update_overrides(
             assert d.group is not None
             legacy_hydra_override = not d.is_override() and d.group.startswith("hydra/")

-        if seen_override and not (d.is_override() or legacy_hydra_override):
-            assert isinstance(last_override_seen, GroupDefault)
+        if (
+            not (d.is_override() or legacy_hydra_override)
+            and d.get_override_key() in last_overrides_seen
+        ):
             pcp = parent.get_config_path()
-            okey = last_override_seen.get_override_key()
-            oval = last_override_seen.get_name()
+            okey = d.get_override_key()
+            oval = last_overrides_seen[okey].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"""
+                    In {pcp}: Override '{okey} : {oval}' is defined before '{okey} : {d.get_name()}'.
+                    Each override must come after the default it is overriding"""
                 )
             )

@@ -392,8 +393,7 @@ def _update_overrides(
                 warnings.warn(msg, UserWarning)

             if d.override:
-                seen_override = True
-                last_override_seen = d
+                last_overrides_seen[d.get_override_key()] = 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
@@ -467,14 +467,7 @@ def _create_defaults_tree_impl(
         self_added = _validate_self(containing_node=parent, defaults=defaults_list)

     if is_root_config:
-        # To ensure config overrides are last, insert the external overrides before the first config override.
-        insert_idx = len(defaults_list)
-        for idx, default in enumerate(defaults_list):
-            if default.is_override():
-                insert_idx = idx
-                break
-
-        defaults_list[insert_idx:insert_idx] = overrides.append_group_defaults
+        defaults_list.extend(overrides.append_group_defaults)

     _update_overrides(defaults_list, overrides, parent, interpolated_subtree)

diff --git a/tests/defaults_list/test_defaults_tree.py b/tests/defaults_list/test_defaults_tree.py
index 3a0bae03f..2dcb977f7 100644
--- a/tests/defaults_list/test_defaults_tree.py
+++ b/tests/defaults_list/test_defaults_tree.py
@@ -621,13 +621,26 @@ def test_defaults_tree_with_package_overrides__group_override(
                 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"""
+                        In override_wrong_order: Override 'group1 : file2' is defined before 'group1 : file1'.
+                        Each override must come after the default it is overriding"""
                     )
                 ),
             ),
             id="test_override_wrong_order_in_defaults_list",
         ),
+        param(
+            "override_same_level",
+            ["+group2=file1"],
+            DefaultsTreeNode(
+                node=ConfigDefault(path="override_same_level"),
+                children=[
+                    GroupDefault(group="group1", value="file2"),
+                    ConfigDefault(path="_self_"),
+                    GroupDefault(group="group2", value="file1"),
+                ],
+            ),
+            id="override_same_level_with_append",
+        ),
     ],
 )
 def test_override_option_from_defaults_list(

[omry]

The approach I took lets overrides come before non-overrides as long as they have a different override key

Nice.

Given the underlying motivation for requiring that overrides are end the end is just for clarity and does not serve a functional purpose, I think it's better to just exempt externally appended config groups from the check as I did.
The code becomes simpler instead of more complicated:
as an example, I am not sure I fully understand why your approach is really solving the problem and other similar problems because it's harder to understand the solution.

[Jasha10]

Given the underlying motivation for requiring that overrides are end the end is just for clarity and does not serve a functional purpose, I think it's better to just exempt externally appended config groups from the check as I did.

Fair enough.
Also, I realized my diff would be a breaking change. The following test would pass on master or on this PR branch, but it fails on my diff with the error message Override 'group1 : file1' is defined before 'group1 : file2':

$ cat tests/defaults_list/data/override_only.yaml
defaults:
  - override group1: file1

$ cat tests/defaults_list/test_defaults_tree.py | sed '2042,2052!d'
        param(
            "override_only",
            ["+group1=file2"],
            DefaultsTreeNode(
                node=ConfigDefault(path="override_only"),
                children=[
                    GroupDefault(group="group1", value="file1"),
                ],
            ),
            id="append_to_override_only",
        ),