omry / omegaconf

Flexible Python configuration system. The last one you will ever need.
BSD 3-Clause "New" or "Revised" License
1.91k stars 104 forks source link

Merging nested readonly structured configs doesn't work. #1102

Open amatsukawa opened 1 year ago

amatsukawa commented 1 year ago

Describe the bug Merging nested readonly structured configs doesn't work.

There seems to be code to temporarily unset the read-only property of the outer-most container prior to merge to make this work in OmegaConf.merge. I believe the issue is that this logic needs to be recursive.

To Reproduce

from omegaconf import OmegaConf, MISSING
from dataclasses import dataclass

@dataclass(frozen=True)
class A:
    x: int = MISSING

@dataclass(frozen=True)
class B:
    a: A = MISSING
    b: int = 1

# This is the config
b = OmegaConf.structured(B(a=A()))
print(b)
# ==> {'a': {'x': '???'}, 'b': 1}

# Works
print(OmegaConf.merge(b, {"b": 3}))
# ==> {'a': {'x': '???'}, 'b': 3}

# Doesn't work
print(OmegaConf.merge(b, {"a": {"x": 3}}))
# ...
# ReadonlyConfigError: Cannot set value of read-only config node
#    full_key: a.x
#    reference_type=A
#    object_type=A

Expected behavior Merging nested structured readonly configs should work.

Jasha10 commented 1 year ago

Thanks for the report, @amatsukawa.

For now, you can work around this issue using the read_write context manager to temporarily turn off the frozen state of the config.

from omegaconf import read_write

...

with read_write(b.a):
    print(OmegaConf.merge(b, {"a": {"x": 3}}))
amatsukawa commented 1 year ago

Thanks for the response @Jasha10.

For those who come later: indeed that was the workaround I had come to, but to use merge with (1) an arbitrarily nested structured template config and (2) unknown (ahead of time) updates in the second argument of the merge, I had settled on recursively setting the readonly flag to false on every DictConfig and ListConfig object.