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

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

Open
amatsukawa opened this issue Jul 14, 2023 · 2 comments
Open

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

amatsukawa opened this issue Jul 14, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@amatsukawa
Copy link

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.

@amatsukawa amatsukawa added the bug Something isn't working label Jul 14, 2023
@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 20, 2023

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
Copy link
Author

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.

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

No branches or pull requests

2 participants