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

Behavior of OmegaConf.update changed in 2.0.2rc1 #363

Closed
JackEngelmann opened this issue Sep 9, 2020 · 4 comments · Fixed by #368
Closed

Behavior of OmegaConf.update changed in 2.0.2rc1 #363

JackEngelmann opened this issue Sep 9, 2020 · 4 comments · Fixed by #368

Comments

@JackEngelmann
Copy link

Hi,

the behavior of update seems to have changed. When replacing a dictionary, values aren't deleted anymore.

Minimal example in 2.0.1: (property "c" is removed)

>>> conf = omegaconf.OmegaConf.create({ "a": 2, "b": { "c": 3 }})
>>> omegaconf.OmegaConf.update(conf, "b", { "d": 4 })
>>> conf
{'a': 2, 'b': {'d': 4}}

The same example in 2.0.2rc1: (property "c" is not removed)

>>> conf = omegaconf.OmegaConf.create({ "a": 2, "b": { "c": 3 } })
>>> omegaconf.OmegaConf.update(conf, "b", { "d": 4 })
>>> conf
{'a': 2, 'b': {'c': 3, 'd': 4}}

Is this a bug or an intended change?

@omry
Copy link
Owner

omry commented Sep 9, 2020

Thanks for letting me know this bit you.

This is an intended change. the behavior of updating dictionaries changed from replace to merge.
The behavior of update for dictionaries and lists was not well defined and the current behavior prevented some very powerful interactions with Structured Configs.

Do you have a use case where you prefer assignment over merging?

@JackEngelmann
Copy link
Author

Ah ok, thank you for your reply!

This could be a breaking change for some people (like me :D ), so maybe a major release would be good.
My use-case is basically just deleting parts of the configuration. Is there another way to do this?

(Specifically, our configuration has { "remote_datasets": ..., "local_datasets": ... }. A script downloads the remote_datasets and adds them to the "local_datasets". The "remote_datasets" are deleted then.)

@omry
Copy link
Owner

omry commented Sep 9, 2020

Yeah, I agree.
The reason I felt this was okay was this was untested and undocumented behavior.
I can maintain backward compatibility by introducing a flag.

I do think the correct interpretation is to merge the dict and not to update it though, but the default behavior should not change without a warning.

@omry omry added this to the OmegaConf 2.0.1 milestone Sep 9, 2020
@omry
Copy link
Owner

omry commented Sep 9, 2020

see #367

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.

2 participants