Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shallow copy of OmegaConf containers does not work correctly #492

Closed
omry opened this issue Jan 24, 2021 · 0 comments · Fixed by #491
Closed

Shallow copy of OmegaConf containers does not work correctly #492

omry opened this issue Jan 24, 2021 · 0 comments · Fixed by #491
Milestone

Comments

@omry
Copy link
Owner

omry commented Jan 24, 2021

OmegaConf containers has a reference to the parent node.
This is to provide features like interpolations and the ability to provide the full key of a node (including the parent keys) in errors.

This means that there is a bi-birectional links between a container (DictConfig, ListConfig) and a node (DictConfig, ListConfig, AnyNode, IntNode etc).

Consider:

a:
  b:
    c: 10

The links are as follows:

a -> b, b -> a
b -> c, c -> b

If we copy a with a shallow copy, we end up with:

a -> b
b -> c, c -> b
# new links
a1 -> b,  b -> a1

Since b is the same object (shallow copy!), it can only have one parent. When we set the parent to a1, the parent is no longer a.
this is breaking the data model. if b (or one of it's children) is an interpolation, it's value might be changed now by changes to the a1 config tree.

One may think that we can solve it by copying b as well:

a -> b, b -> a
b -> c

a1 -> b1, b1 -> a1
b1 -> c, c -> b1

But now we have the same problem with c.

The only real solution is to perform a deepcopy instead of a shallow copy.

@omry omry added this to the OmegaConf 2.1 milestone Jan 24, 2021
@omry omry mentioned this issue Jan 24, 2021
@omry omry closed this as completed in #491 Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant