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.

Sign up for GitHub

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

Jump to bottom

unsafe_merge crashes with nested structured config and union type #1087

Open
3 tasks done
function2-llx opened this issue Jun 11, 2023 · 1 comment · May be fixed by #1088
Open
3 tasks done

unsafe_merge crashes with nested structured config and union type #1087

function2-llx opened this issue Jun 11, 2023 · 1 comment · May be fixed by #1088
Labels
bug Something isn't working

Comments

@function2-llx
Copy link

Describe the bug
unsafe_merge crashes with nested structured config and union type.

To Reproduce

from dataclasses import dataclass

from omegaconf import OmegaConf

@dataclass
class A:
    x: int | str

@dataclass
class B:
    a: A

def main():
    x = OmegaConf.unsafe_merge(OmegaConf.structured(B), {'a': {'x': 1}})
    print(x)

if __name__ == '__main__':
    main()

Expected behavior
{'a': {'x': 1}} will be printed, just like OmegaConf.merge.

Actual result

Traceback (most recent call last):
  File "./test.py", line 18, in <module>
    main()
  File "./test.py", line 14, in main
    x = OmegaConf.unsafe_merge(OmegaConf.structured(B), {'a': {'x': 1}})
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File ".../omegaconf/omegaconf.py", line 308, in unsafe_merge
    target.merge_with(*configs[1:])
  File ".../omegaconf/basecontainer.py", line 492, in merge_with
    self._format_and_raise(key=None, value=None, cause=e)
  File ".../omegaconf/base.py", line 231, in _format_and_raise
    format_and_raise(
  File ".../omegaconf/basecontainer.py", line 490, in merge_with
    self._merge_with(*others)
  File ".../omegaconf/basecontainer.py", line 514, in _merge_with
    BaseContainer._map_merge(self, other)
  File ".../omegaconf/basecontainer.py", line 399, in _map_merge
    dest_node._merge_with(src_node)
  File ".../omegaconf/basecontainer.py", line 514, in _merge_with
    BaseContainer._map_merge(self, other)
  File ".../omegaconf/basecontainer.py", line 358, in _map_merge
    expand(dest)
  File ".../omegaconf/basecontainer.py", line 342, in expand
    node._set_value(val)
  File ".../omegaconf/dictconfig.py", line 647, in _set_value
    raise e
  File ".../omegaconf/dictconfig.py", line 644, in _set_value
    self._set_value_impl(value, flags)
  File ".../omegaconf/dictconfig.py", line 677, in _set_value_impl
    self.__setitem__(k, v)
  File ".../omegaconf/dictconfig.py", line 314, in __setitem__
    self._format_and_raise(key=key, value=value, cause=e)
  File ".../omegaconf/base.py", line 231, in _format_and_raise
    format_and_raise(
  File ".../omegaconf/dictconfig.py", line 308, in __setitem__
    self.__set_impl(key=key, value=value)
  File ".../omegaconf/dictconfig.py", line 318, in __set_impl
    self._set_item_impl(key, value)
  File ".../omegaconf/basecontainer.py", line 535, in _set_item_impl
    if self._get_root() is value._get_root():
                           ^^^^^^^^^^^^^^^^^
  File ".../omegaconf/base.py", line 289, in _get_root
    assert isinstance(self, Container)
AssertionError

Additional context

  • OmegaConf version: 2.3.0
  • Python version: 3.11.4
  • Operating system: Ubuntu 22.04
@function2-llx function2-llx added the bug Something isn't working label Jun 11, 2023
@Jasha10
Copy link
Collaborator

Jasha10 commented Jun 11, 2023

Thanks, @function2-llx. I can reproduce the error.

Jasha10 added a commit to Jasha10/omegaconf that referenced this issue Jun 11, 2023
@Jasha10
Closes omry#1087.
0667739 
This PR addresses an edge-case where `AssertionError` is raised during a
call to `OmegaConf.unsafe_merge`.

This PR is marked as a draft:
- I haven't written tests yet
- I'm not sure if this is the best approach to solving the issue.
@Jasha10 Jasha10 linked a pull request Jun 11, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

unsafe_merge: detect value with no root Jasha10/omegaconf
2 participants
@Jasha10 @function2-llx