Unexpected keys

[weiwei-ww]

Solve the issue I reported in #9 . When interpreting the !include tag, there is no need to pass the overrides to it. In fact, I am thinking if it is necessary at all for the _walk_tree_and_resolve function to have the overrides input, since all the overrides have been handled here, before _walk_tree_and_resolve is called:

HyperPyYAML/hyperpyyaml/core.py

Lines 302 to 306 in 9a36727

	if overrides is not None and overrides != "":
	if isinstance(overrides, str):
	overrides = ruamel_yaml.load(overrides)
	recursive_update(preview, overrides, must_match=overrides_must_match)
	_walk_tree_and_resolve("root", preview, preview, overrides, file_path)

[mravanelli]

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

[pplantinga]

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.

[pplantinga]

See comment

[weiwei-ww]

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.

HyperPyYAML/hyperpyyaml/core.py

Line 306 in 9a36727

_walk_tree_and_resolve("root", preview, preview, overrides, file_path)

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}}.

[pplantinga]

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

[weiwei-ww]

@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.

[pplantinga]

Added tests and removed overrides from _walk_tree_and_resolve