Inconsistencies in recursive behavior in set_readonly()

[MatteoVoges]

Describe the bug
There is unexpected behavior for different types of recursive flag passing when using OmegaConf.set_readonly()

To Reproduce

from omegaconf import OmegaConf

config = OmegaConf.create({"a": {"b": 1}})

# (1)
OmegaConf.set_readonly(config, True)
print(OmegaConf.is_readonly(config.a))
# >>> True (EXPECTED)

OmegaConf.set_readonly(config, False)
print(OmegaConf.is_readonly(config.a))
# >>> False (EXPECTED)

# (2)
OmegaConf.set_readonly(config.a, True)
print(OmegaConf.is_readonly(config))
# >>> False (EXPECTED)

OmegaConf.set_readonly(config, False)
print(OmegaConf.is_readonly(config.a))
# >>> True (WRONG)

Here we see in the first example, that when applying readonly to a config, that subconfigs get recursively updated with the correct state.
In the second example we apply readonly to a subconfig, and the parent node doesn't get updated, but when setting readonly to false in the parent node, the children don't get recursively updated, which is wrong.

Expected behavior
The child nodes should get correctly updated.

Additional context

• OmegaConf version: master (2.4.0.dev1)
• Python version: 3.10.12
• Operating system: linux
• Please provide a minimal repro

[odelalleau]

Although I agree this can be confusing, I'm not sure this is actually a bug.

The way flags work in OmegaConf is that a node inherits its parent's flags except for those that are explicitly set for this node. See code at:

omegaconf/omegaconf/base.py

Line 200 in 7dae67e

def _get_flag_no_cache(self, flag: str) -> Optional[bool]:

The order in which flags are set doesn't matter. Whether you do

OmegaConf.set_readonly(config, False)
OmegaConf.set_readonly(config.a, True)

or

OmegaConf.set_readonly(config.a, True)
OmegaConf.set_readonly(config, False)

you will always have is_readonly(config.a) because the flag is explicitly set for the subconfig node.

I think it'd make sense to add an option to override the child nodes' flags, but in the meantime, my suggestion would be that you write your own function that iterates over children to achieve this.

[MatteoVoges]

Thanks for the explanation. I just thought that the behavior should be the same, regardless if the value of the flag gets set to True or False.
Because I just need a "reset" of the flag for the whole config and some child nodes can be readonly, I just used oc.create(oc.to_container(config)) and create a fresh config without any flags.

[odelalleau]

Thanks for the explanation. I just thought that the behavior should be the same, regardless if the value of the flag gets set to True or False.

It should indeed be the same.

Because I just need a "reset" of the flag for the whole config and some child nodes can be readonly, I just used oc.create(oc.to_container(config)) and create a fresh config without any flags.

I agree it'd be good to also have a way to clear flags recursively.

[MatteoVoges]

It should indeed be the same.

Ahh you're right. I forgot for a moment, that directly set flags have higher priority than the parent flags 🤦‍♂️ .

[omry]

Because I just need a "reset" of the flag for the whole config and some child nodes can be readonly, I just used oc.create(oc.to_container(config)) and create a fresh config without any flags.

IIRC You can reset the value of a node by setting the flag value on that node to None.
If you want to do it recursively, maybe it's better to introduce a new reset_flag function instead of overloading set_flag.

[MatteoVoges]

I would be happy with both options. I just need a config with no flags, therefor its not important, if the value is None or False. A reset_flag function would be cool, too, but would do the same job as set_flag("reaonly", None).

[odelalleau]

Yeah I think a recursive set_flag() is more generic and powerful than a recursive reset_flag().

[omry]

To be honest, I think set_flag with a recursive flag is more confusing.
The semantics of set_flag are already recursive, so to a user reading the doc it would be unclear what recursive=True is doing differently.
In addition, setting the flag recursively will "freeze" the flag on the whole subtree and subsequent calls setting the flag on the parent will no longer be recursive as they are now (unless recursive=True) is passed.