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

Escaped resolver modifying `_root_` doesn't work #1120

Open MatteoVoges opened 12 months ago

MatteoVoges commented 12 months ago

Describe the bug I have a resolver, that modifies _root_. When the resolver is unescaped, it works fine, but if it's escaped, it doesn't behave as expected. This could be interesting for /related to #1113 @odelalleau

To Reproduce Please provide a minimal repro.

from omegaconf.omegaconf import OmegaConf as oc

def modify_root(key, value, _root_):
    _root_[key] = value
    return "done"

test = {
    "a": "a",
    "b": "b",
    "modify_a": "${modify_root:a,MODIFIED}",
    "modify_b": "\${modify_root:b,MODIFIED}"
}

oc.register_new_resolver("modify_root", modify_root)

config = oc.create(test)
oc.resolve(config)
obj = oc.to_object(config) # resolving second time
print(obj)
# >>> {'a': 'MODIFIED', 'b': 'b', 'modify_a': 'done', 'modify_b': 'done'}

We see, that both times the resolver got executed, but only the unescaped one could modify its key (a).

Expected behavior I would expect, that after resolving twice, the key b gets MODIFIED too.

Additional context

odelalleau commented 12 months ago

Yes it's related to #1113. I knew I had to look into the behavior of to_object() / to_container() but didn't have time for it yet.

In the meantime, the workaround is to manually call oc.resolve(config) before oc.to_object(config).

Edit: actually since to_object() is also trying to resolve, I think the workaround would rather be

oc.resolve(config)
obj = oc.to_container(
    cfg=config,
    resolve=False,
    throw_on_missing=True,
    enum_to_str=False,
    structured_config_mode=SCMode.INSTANTIATE,
)

(if you don't want to modify config, you'd need to deepcopy it first)

MatteoVoges commented 12 months ago

I don't see why the resolving behaves different in oc.resolve() and oc.to_object()

os.resolve() works in-place by replacing interpolations with their values. oc.to_object() creates a new config with new nodes and this is using a different code path where the new escape_interpolation_strings flag is currently lost.

and why the _root_ object is affected by this. Because #1113 is just handling the escaping of resolvers, I thought. And this time both resolvers executes...

I haven't looked at it too closely tbh. But I suspect it's related to the fact that #1113 helps avoid issues where the order of interpolation resolution matters when resolving the config. I might be able to provide more details when I look into it more closely.