Make later Defaults List overrides take precedence

[TheSaiEaranti]

Fixes #3229

Problem

A Defaults List override inside a config appended with +group=option did not override an earlier override of the same config group. Overrides.add_override recorded only the first override for a key, so the first override won instead of the last one in depth-first order, contradicting the documented rule.

Fix

Overrides were registered per-config in _update_overrides, before descending into that config's children — which encodes list position rather than depth-first position. Registration now happens in the reverse traversal in _create_defaults_tree_impl, at the point each override entry is encountered:

• Because the traversal is reversed, the first override registered for a key is the last one in depth-first order, and add_override's existing first-wins behavior keeps it. add_override itself is unchanged from main apart from a comment.
• Command-line overrides are registered in Overrides.__init__, before any traversal, so they continue to take priority over Defaults List overrides.
• _update_overrides still validates ordering ("Overrides must be at the end of the defaults list") and rejects overrides in interpolated subtrees; it just no longer registers.
• One special case: an externally appended group default (+db=mysql) is the one legal way a group default can follow an override of the same group in a single defaults list, and the reverse traversal visits the appended entry before that override registers. Resolving an external append therefore first registers the current list's own override for its key, preserving main's behavior for defaults: [override db: sqlite] composed with +db=mysql.

Override resolution remains part of defaults-tree construction, so configs selected during traversal can still introduce additional defaults and overrides.

(An earlier version of this PR pre-registered a config's overrides before traversing its children, with a replace-unless-used rule. The review pointed out an ordering it gets wrong — two competing Defaults List overrides where the deeper one is registered later — and that case is now part of the regression tests below.)

Tests

• group_default_with_override + +experiment=override_config_group — the appended config's override wins (the Defaults-list overrides in appended configs do not override earlier overrides #3229 ordering). Fails on main.
• the same, plus group1=file1 on the command line — the command line still wins.
• group_default_with_explicit_experiment_and_override — group1: file1, experiment: override_config_group, override group1: file3: both competing overrides come from Defaults Lists and the later (file3) wins. Fails on the previous version of this patch.
• the same, plus a command-line group1=file1 — CLI priority.
• group_default_after_explicit_experiment — a group resolved before a nested override for it is registered resolves per its own list; no spurious "Could not override" error.
• group_override_only + +group1=file1 — a config whose defaults list only overrides a group, composed with an append of that group: the override applies. Fails without the external-append handling.

pytest tests/defaults_list tests/test_compose.py → 422 passed. Full suite excluding test_completion → 2055 passed (test_completion fails identically on an unmodified checkout in my environment).

[omry]

This patch pre-registers all overrides from a config before traversing its children. The used flag reflects the current traversal state, not the overrides' depth-first ordering.

This fails for:

defaults:
  - group1: file1
  - experiment: override_config_group  # overrides group1 with file2
  - override group1: file3

There is no command-line override in this example. Both competing overrides come from Defaults Lists.

file3 is last in depth-first order and should win, but this patch produces file2.

Override resolution must remain part of defaults-tree construction because selecting a config can introduce additional defaults and overrides. Please register each Defaults List override when it is encountered during the existing reverse traversal instead of registering all overrides from a config in advance. _update_overrides() can continue validating and preparing the entries, but add_override() should be called from the reverse traversal.

With the original first-wins behavior, reverse traversal will retain the last override in depth-first order. Command-line overrides are registered before traversal and will continue to take priority.

Please add regression coverage for both this ordering and the ordering reported in issue #3229.

Defaults List composition is one of the more challenging areas of Hydra. If this turns out to be more involved than you want to tackle, that is completely fine. There are other, easier issues in the Hydra 1.4 milestone that would also be valuable contributions.

[TheSaiEaranti]

Thank you for the review — you're right. I reproduced your example against my patch and it produces file2; pre-registering a config's overrides before descending into its children encodes list position rather than depth-first position, and the used flag was a proxy for traversal state that happened to cover #3229 but not this ordering.

Reworked as you described:

• add_override() is back to first-wins, unchanged from main except for a comment.
• _update_overrides() no longer registers anything; it still validates ordering and prepares the entries.
• Registration happens in the reverse traversal in _create_defaults_tree_impl, at the point each override entry is encountered. The first registration for a key is therefore the last override in depth-first order, and first-wins keeps it. Command-line overrides are registered in Overrides.__init__ before traversal and retain priority.

One case surfaced while testing compositions against this that I want to flag explicitly, since it's a deviation from the literal prescription. An externally appended group default is the one legal way a group default can follow an override of the same group within a single defaults list, and the reverse traversal visits the appended entry before that override registers:

# primary config
defaults:
  - override db: sqlite

$ python my_app.py +db=mysql

On main this composes db=sqlite; with in-traversal registration alone it raises Could not override 'db', because the appended entry resolves first and the override goes unused. I handled it narrowly: when the reversed loop resolves an externally appended group default, it first registers the current list's own override for that key, if one is pending. This only fires for external appends, so it cannot affect your example (no appends) or #3229 (the appended entry's key is experiment, not group1). Happy to handle it differently if you'd prefer.

Regression coverage for all three orderings, each failing on exactly one version of this change:

• group_default_with_override + +experiment=override_config_group — the Defaults-list overrides in appended configs do not override earlier overrides #3229 ordering; fails on main.
• group_default_with_explicit_experiment_and_override — your example (group1: file1, experiment: override_config_group, override group1: file3 → file3), plus a command-line-priority variant; fails on the previous version of this patch.
• group_override_only + +group1=file1 — the appended-group case above; fails without the append handling.

tests/defaults_list + tests/test_compose.py: 422 passed. Full suite excluding test_completion (fails identically on a clean checkout in my environment): 2055 passed. Also exercised a broader set of compositions locally — overrides nested two and three configs deep, sibling subtrees overriding the same group, hydra-subtree overrides from the primary config and from an experiment, package-qualified overrides, and CLI/deletion interactions — with results consistent with last-in-depth-first-order semantics, and no behavior changes relative to main other than the two intended ones.

[omry]

Thanks for calling this out. Your analysis of Hydra's current behavior is correct, but that behavior is a separate bug. A dangling Defaults List override should fail before Hydra processes command-line overrides, so +db=mysql should not have an opportunity to make an invalid Defaults List appear valid.

I filed issue #3318 to track that problem. It is separate from the override-precedence bug in issue #3229, so it should be fixed separately from this PR.