Hydra 修復通過所有測試，卻被四行 YAML 證明有誤

So this summer I fixed a bug in Hydra. If you haven't used it, Hydra is the config framework that came out of Meta, and a lot of ML projects use it to manage their configs.

My fix passed every test. I was pretty happy with it. Then the maintainer replied with four lines of YAML that broke it.

Honestly, that review taught me more than the fix did, so here's the whole story.

Quick background: the Defaults List

In Hydra, a config can start with a defaults list. That's where you say which options to pull in, like "use file1 for group1." You can also write override group1: file3, which swaps in a different option for that group.

So what happens when two overrides fight over the same group? The docs have a rule for that:

If a Group Default is overridden more than once, the last one, in depth first order, wins.

Depth first just means Hydra walks the whole tree. If something in your list has its own defaults list, Hydra goes all the way into it before it moves on to the next entry.

The bug

The maintainer, omry, had filed this one as #3229. Say your main config overrides group1 to b. Then you add an experiment config from the command line with +experiment=..., and that experiment overrides group1 to c. The experiment comes later, so c should win. You got b.

Once I found it, the cause was pretty simple. Hydra keeps a record of overrides, and add_override only kept the first one it saw for each group. So the first override won, not the last.

My first fix

My idea was to register all of a config's overrides up front, before Hydra walks into that config's children. If a later override showed up for the same group, it could replace the earlier one, unless the earlier one had already been used.

It fixed the bug from the issue. All the existing tests passed. I opened the PR feeling good about it.

Four lines of YAML

A few hours later, omry replied with this:

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

The experiment config has its own override that sets group1 to file2. Then the last line of the main config overrides it to file3.

Walk it depth first. You go into experiment and hit file2. Then you come back out and hit file3. So file3 is last, and it should win.

My patch gave file2.

Here's why. I registered the main config's overrides before going into any of its children, so file3 got registered early. Then file2 showed up later, while Hydra was inside experiment, and my replace rule let it take over. I was going by when things got registered, and that is just not the same thing as depth-first order. The "already used" flag was standing in for the real rule, and it only happened to work for the case in the issue.

omry summed it up in one line: "The used flag reflects the current traversal state, not the overrides' depth-first ordering."

None of the existing tests had that ordering, so nothing caught it. Tests only cover the cases somebody already thought of.

The real fix was smaller

The review also explained how to fix it, and honestly it was really elegant. Hydra already walks each defaults list in reverse. So if you register each override at the moment you reach it in that reverse walk, the first one you register for a group is the last one in depth-first order. And add_override already kept the first one it saw.

So that first-wins behavior was right all along. The bug was where the call happened, not what it did.

I moved the registration into the reverse traversal and left add_override the way it was. Command-line overrides get registered before any of this happens, so they still beat everything in the defaults lists.

The edge case I almost buried

While I was testing the new version, I found one case that broke:

# primary config
defaults:
  - override db: sqlite

python my_app.py +db=mysql

On main, that gives you sqlite. With my new code it failed with Could not override 'db', because the reverse walk reaches the appended db entry before the override gets registered.

I could have quietly patched around it. Instead I handled it narrowly, so the behavior stayed exactly like main, and I called it out in the PR as a spot where I'd gone beyond what was asked.

omry's answer: "Your analysis of Hydra's current behavior is correct, but that behavior is a separate bug." That became its own issue, #3318, and I fixed it the next day in #3319.

The tests

I ended up with regression tests where each one fails on exactly one wrong version:

• the ordering from the original issue fails on main
• omry's four lines fail on my first patch
• the +db=mysql case fails without the special handling

That's my favorite part of the whole PR. If every test kills one specific wrong implementation, you know your tests are actually doing something.

The PR got opened, reviewed, reworked and merged all on the same day.

What I took away

• Passing tests isn't the same as being right. Tests only cover what someone already thought of. Now, for anything where order matters, I try to break my own fix by shuffling the order before I ask anyone to review it.
• Code the actual rule. I wrote code for a stand-in (when things got registered, plus a flag) instead of the rule itself (depth-first position). They agreed on the easy case and disagreed on the interesting one.
• Flag the weird stuff. The edge case I called out turned into its own issue and my next merged PR.
• Take the hard review. omry even mentioned that if this turned out to be more than I wanted to take on, there were easier issues in the 1.4 milestone. I'm really glad I stuck with it.

If you want to dig in:

• The PR: hydra#3315
• The original bug: hydra#3229
• The follow-up: hydra#3318, fixed in hydra#3319