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 configs unnecessarily tries to access base config interpolation #598

Closed
4 tasks done
1matthewli opened this issue Mar 12, 2021 · 10 comments
Closed
4 tasks done
Labels
bug Something isn't working

Comments

@1matthewli
Copy link

Describe the bug
When merging configs, OmegaConf seems to attempt to access interpolated values in the base config.

To Reproduce

base = OmegaConf.create({'a': '${a}'})
override = OmegaConf.create({'a': ''})
merged = OmegaConf.merge(base, override)

This produces the behavior:

RecursionError: maximum recursion depth exceeded while calling a Python object
	full_key: 
	reference_type=Optional[Dict[Union[str, Enum], Any]]
	object_type=dict

Expected behavior
The expected behavior is to produce the merged config {'a': ''}. I understand that the input config base may be considered as invalid, but it would be great if that did not stop the merging from working correctly.

Additional context

  • OmegaConf version: 2.0.6
  • Python version: 3.7.9
  • Operating system : Ubuntu 16.04.7 LTS
  • Please provide a minimal repro
@1matthewli 1matthewli added the bug Something isn't working label Mar 12, 2021
@omry
Copy link
Owner

omry commented Mar 12, 2021

This is a feature.
Merging a dict into an interpolation to a dict creates a new node that is the merger of both.

In [9]:  OmegaConf.merge({"a" :"${b}", "b": {"a": 1}}, {"a": {"c": 3}})
Out[9]: {'a': {'a': 1, 'c': 3}, 'b': {'a': 1}}

@omry
Copy link
Owner

omry commented Mar 12, 2021

Your config is also broken. a node should not interpolate to itself.

@1matthewli
Copy link
Author

I guess I just gave the most minimal example I could think of. Here is one that doesn't involve any broken configs:

base = OmegaConf.create({'a': '', 'b': '${a}'})
override = OmegaConf.create({'a': 'foo', 'nested': {'a': '${a}', 'b': '${a}'}})
OmegaConf.merge(base, override.nested)

@omry
Copy link
Owner

omry commented Mar 13, 2021

This kind of merge where you are merging things in different levels is a bad idea and is guaranteed to give you issues in various cases.

As I said, resolving the interpolation during merge is an intentional behavior.

Can you give some context on what you are trying to achieve here?

@omry
Copy link
Owner

omry commented Mar 13, 2021

Looking at your second example, you are merging

{'a': '${a}', 'b': '${a}'}

on

{'a': '', 'b': '${a}'}

and you are getting

{'a': '${a}', 'b': '${a}'}

This looks like a perfect interpretation to me.
Please explain what you are expecting to get and why.

Closing as invalid, feel free to follow up with more context.

@omry omry closed this as completed Mar 13, 2021
@omry
Copy link
Owner

omry commented Mar 13, 2021

I now noticed that the version you tested against is 2.0.6.
Try this on 2.1.0 dev release (pip install omegaconf --pre --upgrade).

$ ipython
Python 3.8.5 (default, Sep  4 2020, 07:30:14) 
Type 'copyright', 'credits' or 'license' for more information
IPython 7.19.0 -- An enhanced Interactive Python. Type '?' for help.

In [1]: from omegaconf import *

In [2]: __version__
Out[2]: '2.1.0dev21'

In [3]: base = OmegaConf.create({'a': '', 'b': '${a}'})
   ...: override = OmegaConf.create({'a': 'foo', 'nested': {'a': '${a}', 'b': '${a}'}})
   ...: OmegaConf.merge(base, override.nested)
Out[3]: {'a': '${a}', 'b': '${a}'}

@omry
Copy link
Owner

omry commented Mar 13, 2021

as a side note, your config is still broken because the result contains self interpolation.

@1matthewli
Copy link
Author

Thanks, the 2.1.0 dev release works!

@1matthewli
Copy link
Author

If you happen to know off the top of your head, just curious what changed that lets the merging succeed in the newer version despite the interpolation being invalid?

In [3]: base = OmegaConf.create({'a': '', 'b': '${a}'})
   ...: override = OmegaConf.create({'a': 'foo', 'nested': {'a': '${a}', 'b': '${a}'}})
   ...: OmegaConf.merge(base, override.nested)
Out[3]: {'a': '${a}', 'b': '${a}'}

@omry
Copy link
Owner

omry commented Mar 15, 2021

Many bugs have been fixed. Many of which are around the merge functionality.
This could be related : #486.

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