omegaconf icon indicating copy to clipboard operation
omegaconf copied to clipboard

Read-only flag not working during `OmegaConf.merge`

Open tarepan opened this issue 4 years ago • 3 comments

Summary

Even if we set Read-only flag, it do not prevent OmegaConf.merge (write operation).

To Reproduce

Work on value reassignment.

from omegaconf import OmegaConf

conf = OmegaConf.create({"field_a": "hello"})
OmegaConf.set_readonly(conf, True)

conf.field_a = "updated"
# ReadonlyConfigError: Cannot change read-only config container

Not work on config merge.

from omegaconf import OmegaConf

conf = OmegaConf.create({"field_a": "hello"})
OmegaConf.set_readonly(conf, True)

conf_update = OmegaConf.create({"field_a": "updated"})
conf_v2 = OmegaConf.merge(conf, conf_update) 
print(conf_v2.field_a)
# updated

Expected behavior

merge will be block when read-only field will be replaced.

merge is NOT literally in-place write operation, but it is practically write operation on single true config.
In my opinion, Read-only flag should be useful when I want to protect some field from accidental override by merge.

At least, this behavior should be explicitly documented.

Additional context

  • [x] OmegaConf version: 2.1.1
  • [x] Python version: Python 3.7.12
  • [x] Operating system: Ubuntu 18.04.5 LTS
  • [x] Please provide a minimal repro

tarepan avatar Oct 13 '21 15:10 tarepan

As you are pointing out, merge is NOT a write operation as it creates a new config. This is by design and makes perfect sense to me.

omry avatar Oct 14 '21 04:10 omry

Thanks for clarification!

Someone (like me) could misunderstand this intent/designDecision.
Explicit documentation in "Read-only flag" chapter will prevent this unhappy misunderstanding.

As feature request, I propose document improvements.

tarepan avatar Oct 14 '21 05:10 tarepan

By the way, there is an undocumented cfg.merge_with() function that does perform an inline write. This should fail if the dest config is read only.

omry avatar Oct 14 '21 19:10 omry