unsafe_merge: detect value with no root #1088
Conversation
| # 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(): |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Make sure that your proposed logic does not treat the nodes as being from the same config (see comment in 538).
There was a problem hiding this comment.
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.
|
Hi @omry , Thank you for creating this PR. I'm wondering if there is any blocker for merging this PR, that I could potentially help resolving? |
Closes #1087.
This PR addresses an edge-case where
AssertionErroris raised during acall to
OmegaConf.unsafe_merge.This PR is marked as a draft: