@pplantinga, could you please take a look. I'm wondering what are the implications on the existing recipes that use !include (e.g, voicebank)

Thanks for the contribution! However there's still some things we should take a look at and fix before its good to go.

First, extra keys still get added if there are any items being passed. For example:

f1.yaml

seed: 23
config: !include:f2.yaml
  lr: 0.02

f2.yaml

batch_size: 32
lr: 0.01

Calling:

with open("f1.yaml") as f:
    load_hyperpyyaml(f, overrides={"seed": 8})

results in:

{seed: 8, config: {batch_size: 32, lr: 0.02, seed: 8}}

The other thing that I'm worried about is potentially missing cases where the overrides should be passed, though I suppose some would argue that this is intended behavior.

Same yaml files, but different override call:

with open("f1.yaml") as f:
    load_hyperpyyaml(f, overrides={"batch_size": 24}, overrides_must_match=False)

main branch result:

{seed: 23, config: {batch_size: 24, lr: 0.02}, batch_size: 24}

this PR result:

{seed: 23, config: {batch_size: 32, lr: 0.02}, batch_size: 24}

I designed it to work the first way because I thought that was more intuitive, but I can see an argument that the opposite is true, and that all overrides must be passed explicitly. If that's the case, we should rewrite this to work that way instead.

First, extra keys still get added if there are any items being passed.

To address this case, when calling _walk_tree_and_resolve, giving it an empty overrides (e.g. {} or None) can solve it. As I mentioned, I think the input overrides is not necessary here, since all the overrides have been properly handled before this line.

The other thing that I'm worried about is potentially missing cases where the overrides should be passed, though I suppose some would argue that this is intended behavior.

As for this, indeed we have different outputs from the main branch and my PR.

Main output

{seed: 23, config: {batch_size: 24, lr: 0.02}, batch_size: 24}

PR output:

{seed: 23, config: {batch_size: 32, lr: 0.02}, batch_size: 24}

I can see an argument that the opposite is true, and that all overrides must be passed explicitly.

That is also my point of view. I think the PR output makes more sense, since the value of overrides is overrides={"batch_size": 24}, therefore, the config[batch_size] is not overridden. Meanwhile, if you wish to override the config[batch_size], the value of overrides should be overrides={'config': {'batch_size': 24}}.

I think you're right that its better to be explicit in this case. I also think you're probably right that _walk_tree_and_resolve should get an empty overrides, or rather, that argument should be removed from the function altogether. However, since this is a change to functionality (even if its only a minor one), I would like it to be pretty well tested. Would you be willing to make this change as well as adding a few tests along the lines of the examples in the comments here? In addition, we'd have to run some recipes to make sure it doesn't break them (it shouldn't...)

@pplantinga I agree that it has to be fully tested before the changes are applied. I will try to come up with some test cases and post them here.

Added tests and removed overrides from _walk_tree_and_resolve