ListConfig append deepcopies input config nodes

[omry]

Closes #601

Not worried about perf:
This is something we are already doing for DictConfig, and ListConfig is not used nearly as much.

[Jasha10]

The BaseContainer._set_item_impl method uses a "no_deepcopy_set_nodes" flag to decide whether deepcopy shold be done.
I think that ListConfig.append should probably do the same...

[omry]

The BaseContainer._set_item_impl method uses a "no_deepcopy_set_nodes" flag to decide whether deepcopy shold be done.
I think that ListConfig.append should probably do the same...

This is an optimization, so not super critical because ListConfigs are not common.
I added it anyway. One thing I did not add was a logic that copies anyway if the assigned node is from the same config tree. I think this is handing some edge case but I would rather see it first before I enable the same logic in append.

[Jasha10]

One thing I did not add was a logic that copies anyway if the assigned node is from the same config tree. I think this is handing some edge case but I would rather see it first before I enable the same logic in append.

I commented out the edge-case logic to see what test would fail. The following case from test_merge.py would fail:

input1 = {"n": {"a": 10}, "i": "${n}"}
input2 = {"i": {"b": 20}}
expected = {"n": {"a": 10}, "i": {"a": 10, "b": 20}}
# the below succeeds
assert expected == OmegaConf.merge(input1, input2)
# the below fails: unsafe_merge returns {'n': {'a': 10, 'b': 20}, 'i': {'a': 10, 'b': 20}} 
assert expected == OmegaConf.unsafe_merge(input1, input2)

It seems that this edge case only happens when merging into an interpolation; the deepcopy prevents the merge from mutating other parts of the tree.
ListConfig.append does not mutate other parts of the tree, so I think we're safe.

[odelalleau]

I guess I'm a few minutes late on the review, sorry. Feel free to ignore my minor comment below.

At high level I'm fine with it but I wonder if append() wouldn't be better implemented as "append None to the underlying content list, then call set_item() on the last index". There's more stuff happening in _set_item_impl() that I don't fully grasp, which may -- or may not -- matter as well for append().

[odelalleau]

Easier to read (at least to me):

Suggested change

	ListConfig append now copies input config nodes
	ListConfig.append now copies input config nodes

[omry]

I guess I'm a few minutes late on the review, sorry. Feel free to ignore my minor comment below.

At high level I'm fine with it but I wonder if append() wouldn't be better implemented as "append None to the underlying content list, then call set_item() on the last index". There's more stuff happening in _set_item_impl() that I don't fully grasp, which may -- or may not -- matter as well for append().

Good call.
It did cross my mind briefly but I didn't try.
Got it to work in #740.