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 not working correctly for DictConfig #450

Closed
louismartin opened this issue Dec 9, 2020 · 13 comments · Fixed by #452
Closed

Shallow copy not working correctly for DictConfig #450

louismartin opened this issue Dec 9, 2020 · 13 comments · Fixed by #452
Labels
bug Something isn't working

Comments

@louismartin
Copy link

louismartin commented Dec 9, 2020

Describe the bug
It seems that DictConfig objects cannot be copied in python. I am not sure if this is expected behaviour or not.

To Reproduce

from omegaconf import OmegaConf
a = OmegaConf.create({"a": 1, "b": 2})
b = a.copy()
b["a"] = 42
print(b)
print(a)

Outputs:

{'a': 42, 'b': 2}
{'a': 42, 'b': 2}

Simple fix

A simple fix would be to first cast the object to a dict:

from omegaconf import OmegaConf
a = OmegaConf.create({"a": 1, "b": 2})
b = dict(a).copy()
b["a"] = 42
print(b)
print(a)

Outputs:

{'a': 42, 'b': 2}
{'a': 1, 'b': 2}

This however only works for first level attributes, nested DictConfigs won't be casted to dict and will therefore be modified inplace.
For instance:

from omegaconf import OmegaConf
a = OmegaConf.create({"a": {"aa": 1}, "b": 2})
b = dict(a).copy()
b["a"]["aa"] = 42
print(b)
print(a)

Outputs:

{'a': {'aa': 42}, 'b': 2}
{'a': {'aa': 42}, 'b': 2}

This caused a downstream bug in fairseq: facebookresearch/fairseq#3011

@louismartin louismartin added the bug Something isn't working label Dec 9, 2020
@omry
Copy link
Owner

omry commented Dec 9, 2020

Hi, this looks identical to the semantics of copy on the standard dict:

In [3]: a = {"a": {"aa": 1}, "b": 2}

In [4]: b = a.copy()

In [5]: b["a"]["aa"] = 42

In [6]: a
Out[6]: {'a': {'aa': 42}, 'b': 2}

In [7]: b
Out[7]: {'a': {'aa': 42}, 'b': 2}

You should probably use copy.deepcopy().

@omry
Copy link
Owner

omry commented Dec 9, 2020

Closing as I think it's not a bug, let me know if you disagree.

@omry omry closed this as completed Dec 9, 2020
@louismartin
Copy link
Author

Oh yes good point, I totally agree that for the nested DictConfigs it is the same behaviour as standard dictionaries.
Is this wanted, however, that non-nested DictConfigs cannot be copied using the copy operation? (unlike standard dictionaries this time)

@omry
Copy link
Owner

omry commented Dec 10, 2020

Can you create a minimal example where the behavior of the DictConfig deviates from that of the standard config?

@louismartin
Copy link
Author

I meant that DictConfig does not behave like dictionaries of the standard python library.

Standard dictionaries:

a = {"a": 1, "b": 2}
b = a.copy()
b["a"] = 42
print(b)
print(a)
----- Output -----
{'a': 42, 'b': 2}
{'a': 1, 'b': 2}

DictConfig:

from omegaconf import OmegaConf
a = OmegaConf.create({"a": 1, "b": 2})
b = a.copy()
b["a"] = 42
print(b)
print(a)
----- Output -----
{'a': 42, 'b': 2}
{'a': 42, 'b': 2}

@omry
Copy link
Owner

omry commented Dec 10, 2020

Gotcha. definitely looks like a bug.
For now use deepcopy.

@omry omry added this to the OmegaConf 2.1 milestone Dec 10, 2020
@omry omry changed the title Copying omegaconf.dictconfig.DictConfig objects does not work Shallow copy not working correctly for DictConfig Dec 10, 2020
@omry
Copy link
Owner

omry commented Dec 10, 2020

Once this is fixed, we should consider backporting to the 2.0_branch.

@louismartin
Copy link
Author

Perfect thanks!

@omry omry reopened this Dec 10, 2020
@omry
Copy link
Owner

omry commented Dec 10, 2020

@odelalleau, @pereman2 can one if you take a look?

@pereman2
Copy link
Contributor

@odelalleau, @pereman2 can one if you take a look?

Sure

@omry
Copy link
Owner

omry commented Dec 13, 2020

@pereman2, can you backport to 2.0?

@omry omry reopened this Dec 13, 2020
pereman2 added a commit to pereman2/omegaconf that referenced this issue Dec 13, 2020
@omry omry modified the milestones: OmegaConf 2.1, OmegaConf 2.0.6 Dec 13, 2020
@omry
Copy link
Owner

omry commented Dec 13, 2020

This is merged to the 2.0 branch. We will probably wait a bit before releasing 2.0.6 to see if there are any other small bugfixes it should include.

@omry omry closed this as completed Dec 13, 2020
@omry
Copy link
Owner

omry commented Dec 13, 2020

@louismartin, I am guessing that you also want this in fbcode. Ping me on Workplace to discuss how to get this done (should be easy).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants