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: detect value with no root #1088

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

unsafe_merge: detect value with no root #1088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0667739
Closes #1087.
Jasha10 Jun 11, 2023
3cc1ba6
add test
Jasha10 Jun 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion omegaconf/basecontainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -531,8 +531,12 @@ def _set_item_impl(self, key: Any, value: Any) -> None:
if isinstance(value, Node):
do_deepcopy = not self._get_flag("no_deepcopy_set_nodes")
if not do_deepcopy and isinstance(value, Box):
# Detect the special case where value has no root:
value_has_root = (
isinstance(value, Container) or value._get_parent() is not None
)
# if value is from the same config, perform a deepcopy no matter what.
if self._get_root() is value._get_root():
if value_has_root and self._get_root() is value._get_root():
Comment on lines +534 to +539
Copy link
Collaborator Author

@Jasha10 Jasha10 Jun 11, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally, every node should have a root. The "root" of a node is a top-level Container that owns the node. In other words, the root is the top of the config tree. You can get the root of a node by calling node._get_root().

In example from issue #1087, the above call to value._get_root() raises an AssertionError. This happens because value is a temporary UnionNode value that has no parent (and thus has no root).

Here is a potential alternative approach to solving issue #1087:
Instead of detecting whether value_has_root as above, we could modify the Node._get_root method to return an Optional[Container] instead of a Container.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both approaches seems okay to me.
Also, please add a test for unsafe_merge similar in this case (similar to the repro in the bug report).

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure that your proposed logic does not treat the nodes as being from the same config (see comment in 538).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added the test.

Make sure that your proposed logic does not treat the nodes as being from the same config

Yup. If value_has_root is False then value is a temporary node and does not come from the same config as self.

do_deepcopy = True

if do_deepcopy:
Expand Down
21 changes: 21 additions & 0 deletions tests/test_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import copy
import re
import sys
from dataclasses import dataclass
from textwrap import dedent
from typing import (
Any,
Expand Down Expand Up @@ -1412,6 +1413,26 @@ def test_merge_with_other_as_interpolation(
assert OmegaConf.is_interpolation(res, node)


@mark.parametrize("merge", [OmegaConf.merge, OmegaConf.unsafe_merge])
def test_merge_with_nested_structured_config_and_union_type(
merge: Any,
) -> None:
@dataclass
class Inner:
x: Union[int, str]

@dataclass
class Outer:
inner: Inner

dst = OmegaConf.structured(Outer)
src = OmegaConf.create({"inner": {"x": 10}})
res = merge(dst, src)

expected = {"inner": {"x": 10}}
assert res == expected


@mark.parametrize(
("c1", "c2"),
[
Expand Down