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

Fix bug where interpolations were unnecessarily resolved during merge. #432

Merged
merged 6 commits into from
Nov 11, 2020
Merged

Fix bug where interpolations were unnecessarily resolved during merge. #432

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b258b77
fix and tests
pereman2 Nov 6, 2020
9917717
test cases, interpolation as other fix
pereman2 Nov 6, 2020
026b385
news
pereman2 Nov 6, 2020
29b3304
name tests and no trow on resolution failure as default
pereman2 Nov 7, 2020
e047a26
split test and revert some changes
pereman2 Nov 8, 2020
a7b4765
add interpolation comment
pereman2 Nov 9, 2020
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
4 changes: 2 additions & 2 deletions omegaconf/basecontainer.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -298,7 +298,7 @@ def expand(node: Container) -> None:
else:
node._set_value(type_)

if dest._is_missing():
if dest._is_interpolation() or dest._is_missing():
omry marked this conversation as resolved.
Show resolved Hide resolved
expand(dest)

for key, src_value in src.items_ex(resolve=False):
Expand Down Expand Up @@ -387,7 +387,7 @@ def _merge_with(
if isinstance(self, DictConfig) and isinstance(other, DictConfig):
BaseContainer._map_merge(self, other)
elif isinstance(self, ListConfig) and isinstance(other, ListConfig):
if self._is_none() or self._is_missing() or self._is_interpolation():
if self._is_none() or self._is_interpolation() or self._is_missing():
self.__dict__["_content"] = []
else:
self.__dict__["_content"].clear()
Copy link
Owner

Choose a reason for hiding this comment

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

This bit puzzles me. the two branches are pretty similar.
Can you try to see if you can just use one of the branches always?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This block can be reduced to self.__dict__["_content"] = [] without the if-else and works the same

Copy link
Owner

Choose a reason for hiding this comment

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

Cool, please do it.

Expand Down
10 changes: 10 additions & 0 deletions tests/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,13 @@ class SubscriptedList:
@dataclass
class SubscriptedDict:
dict: Dict[str, int] = field(default_factory=lambda: {"foo": 4})


@dataclass
class InterpolationList:
list: List[float] = "${optimization.lr}" # type: ignore
Copy link
Owner

Choose a reason for hiding this comment

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

Check the docs for interpolation with Structured Configs.
you can do something like:

list: List[float] = II("optimization.lr")

And this would not need the type: ignore.



@dataclass
class InterpolationDict:
dict: Dict[str, int] = "${optimization.lr}" # type: ignore
22 changes: 22 additions & 0 deletions tests/test_merge.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
ConfWithMissingDict,
Group,
IllegalType,
InterpolationDict,
InterpolationList,
MissingDict,
MissingList,
Package,
Expand Down Expand Up @@ -484,3 +486,23 @@ def test_merge_allow_objects() -> None:
cfg._set_flag("allow_objects", True)
ret = OmegaConf.merge(cfg, {"foo": iv})
assert ret == {"a": 10, "foo": iv}


@pytest.mark.parametrize( # type:ignore
"dst, other, expected",
[
(
OmegaConf.structured(InterpolationList),
OmegaConf.create({"list": [0.1]}),
{"list": [0.1]},
),
(
OmegaConf.structured(InterpolationDict),
OmegaConf.create({"dict": {"a": 4}}),
{"dict": {"a": 4}},
),
],
)
def test_merge_with_interpolation(dst: Any, other: Any, expected: Any) -> None:
Copy link
Owner

Choose a reason for hiding this comment

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

Add a test for merging with itself.

Should be something like:

            OmegaConf.structured(InterpolationDict),
            OmegaConf.structured(InterpolationDict),
            {"dict": {"a": "${optimization.lr}"}},

I want to see that the interpolation is not resolved in the test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This fails, in this case in validate_merge

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Owner

Choose a reason for hiding this comment

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

Okay, this needs to pass too. no reason for it to fail.

res = OmegaConf.merge(dst, other)
assert res == expected