Fix bug where interpolations were unnecessarily resolved during merge. #432
Conversation
tests/__init__.py
Outdated
|
|
||
| @dataclass | ||
| class InterpolationList: | ||
| list: List[float] = "${optimization.lr}" # type: ignore |
There was a problem hiding this comment.
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.
omegaconf/basecontainer.py
Outdated
| self.__dict__["_content"] = [] | ||
| else: | ||
| self.__dict__["_content"].clear() |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
This block can be reduced to self.__dict__["_content"] = [] without the if-else and works the same
tests/test_merge.py
Outdated
| ), | ||
| ], | ||
| ) | ||
| def test_merge_with_interpolation(dst: Any, other: Any, expected: Any) -> None: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
This fails, in this case in validate_merge
There was a problem hiding this comment.
Okay, this needs to pass too. no reason for it to fail.
|
Also, can you tell me what is missing from this PR? :) |
news fragment? |
Yup :) |
pereman2
changed the title
omegaconf/basecontainer.py
Outdated
| @@ -575,9 +577,12 @@ def _item_eq( | |||
| def _is_none(self) -> bool: | |||
| return self.__dict__["_content"] is None | |||
|
|
|||
| def _is_missing(self) -> bool: | |||
| def _is_missing(self, throw_on_resolution_failure: bool = True) -> bool: | |||
There was a problem hiding this comment.
What if you change this function to not throw on resolution failure (without a parameter), does this cause any issues?
There was a problem hiding this comment.
It doesn't cause any issue, It could be changed to that.
tests/test_merge.py
Outdated
| ( | ||
| OmegaConf.structured(InterpolationDict), | ||
| OmegaConf.structured(InterpolationDict), | ||
| None, | ||
| "dict", | ||
| True, | ||
| ), | ||
| ( | ||
| OmegaConf.structured(InterpolationList), | ||
| OmegaConf.structured(InterpolationList), | ||
| None, | ||
| "list", | ||
| True, | ||
| ), |
There was a problem hiding this comment.
can you name tests?
pytest.param(..., id="test_name")
| if src._is_interpolation(): | ||
| dest._set_value(src._value()) | ||
| return |
There was a problem hiding this comment.
Merging an interpolation to a container is basically keeping the interpolation in dest therefore, dest's value should be src's value.
If I don't do this check here it will fail in for key, src_value in src.items_ex(resolve=False):(line 307).
To sum up, it's the same as line 289 where if src._is_missing(): dest._set_value("???")
There was a problem hiding this comment.
Ok. See that there is a comment explaining it in 288.
Add a similar comment.
omegaconf/dictconfig.py
Outdated
| @@ -445,7 +445,7 @@ def pop(self, key: Union[str, Enum], default: Any = DEFAULT_VALUE_MARKER) -> Any | |||
| self._format_and_raise(key=key, value=None, cause=e) | |||
|
|
|||
| def keys(self) -> Any: | |||
| if self._is_missing() or self._is_interpolation() or self._is_none(): | |||
| if self._is_interpolation() or self._is_missing() or self._is_none(): | |||
There was a problem hiding this comment.
not that _is_missing dies not throw, you can undo this change.
omegaconf/omegaconf.py
Outdated
| elif c._is_interpolation(): | ||
| return None |
There was a problem hiding this comment.
Now that is_missing doesn't throw on resolution failure it doesn't matter this line. I'll remove it.
tests/test_merge.py
Outdated
| def test_merge_with_interpolation( | ||
| dst: Any, other: Any, expected: Any, node: Any, check_missing: bool | ||
| ) -> None: | ||
| res = OmegaConf.merge(dst, other) | ||
| if check_missing: | ||
| OmegaConf.is_missing(res, node) | ||
| else: | ||
| assert res == expected |
There was a problem hiding this comment.
do one thing in each test.
you can add another test to test the behavior of _is_missing with interpolation.
There was a problem hiding this comment.
I'm changing it to is_interpolation
Sure |
Closes #431 .
DictConfig and ListConfig which were interpolations had to be "expanded" in order to fulfil the merge. If it wasn't, it detaches the node to find the interpolation value which in the example given in #431 doesn't exist and then fails. is_interpolation has to be checked before is_missing since is_missing detaches the node(which will resolve the interpolation) to find if that is missing.