From 37e9319ea4744e9c83cfd442356e236c455b6aac Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 11:36:20 -0800 Subject: [PATCH 1/9] test cleanup --- tests/test_struct.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/test_struct.py b/tests/test_struct.py index 66c87d580..d15e23406 100644 --- a/tests/test_struct.py +++ b/tests/test_struct.py @@ -1,7 +1,7 @@ import re from typing import Any, Dict -import pytest +from pytest import mark, raises from omegaconf import OmegaConf from omegaconf.errors import ConfigKeyError @@ -16,7 +16,7 @@ def test_struct_set_on_dict() -> None: c = OmegaConf.create({"a": {}}) OmegaConf.set_struct(c, True) # Throwing when it hits foo, so exception key is a.foo and not a.foo.bar - with pytest.raises(AttributeError, match=re.escape("a.foo")): + with raises(AttributeError, match=re.escape("a.foo")): # noinspection PyStatementEffect c.a.foo.bar @@ -24,13 +24,13 @@ def test_struct_set_on_dict() -> None: def test_struct_set_on_nested_dict() -> None: c = OmegaConf.create({"a": {"b": 10}}) OmegaConf.set_struct(c, True) - with pytest.raises(AttributeError): + with raises(AttributeError): # noinspection PyStatementEffect c.foo assert "a" in c assert c.a.b == 10 - with pytest.raises(AttributeError, match=re.escape("a.foo")): + with raises(AttributeError, match=re.escape("a.foo")): # noinspection PyStatementEffect c.a.foo @@ -38,18 +38,18 @@ def test_struct_set_on_nested_dict() -> None: def test_merge_dotlist_into_struct() -> None: c = OmegaConf.create({"a": {"b": 10}}) OmegaConf.set_struct(c, True) - with pytest.raises(AttributeError, match=re.escape("foo")): + with raises(AttributeError, match=re.escape("foo")): c.merge_with_dotlist(["foo=1"]) -@pytest.mark.parametrize("in_base, in_merged", [(dict(), dict(a=10))]) +@mark.parametrize("in_base, in_merged", [({}, {"a": 10})]) def test_merge_config_with_struct( in_base: Dict[str, Any], in_merged: Dict[str, Any] ) -> None: base = OmegaConf.create(in_base) merged = OmegaConf.create(in_merged) OmegaConf.set_struct(base, True) - with pytest.raises(ConfigKeyError): + with raises(ConfigKeyError): OmegaConf.merge(base, merged) @@ -59,6 +59,6 @@ def test_struct_contain_missing() -> None: assert "foo" not in c -@pytest.mark.parametrize("cfg", [{}, OmegaConf.create({}, flags={"struct": True})]) +@mark.parametrize("cfg", [{}, OmegaConf.create({}, flags={"struct": True})]) def test_struct_dict_get(cfg: Any) -> None: assert cfg.get("z") is None From 51cd4b4bcc029635ab67e6abe375a9346c16ed70 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 11:59:20 -0800 Subject: [PATCH 2/9] Assignment of a dict to an existing node in a parent in struct mode no longer raises ValidationError --- news/586.bugfix | 2 ++ omegaconf/omegaconf.py | 26 +++++++++++++++----------- tests/test_struct.py | 7 +++++++ 3 files changed, 24 insertions(+), 11 deletions(-) create mode 100644 news/586.bugfix diff --git a/news/586.bugfix b/news/586.bugfix new file mode 100644 index 000000000..ffdaf308b --- /dev/null +++ b/news/586.bugfix @@ -0,0 +1,2 @@ +Assignment of a dict to an existing node in a parent in struct mode no longer raises ValidationError + diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index d78e2605e..842ffabbc 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -898,6 +898,9 @@ def _node_wrap( ref_type: Any = Any, ) -> Node: node: Node + allow_objects = parent is not None and parent._get_flag("allow_objects") is True + dummy = OmegaConf.create(flags={"allow_objects": allow_objects}) + is_dict = is_primitive_dict(value) or is_dict_annotation(type_) is_list = ( type(value) in (list, tuple) @@ -909,7 +912,7 @@ def _node_wrap( node = DictConfig( content=value, key=key, - parent=parent, + parent=dummy, ref_type=type_, is_optional=is_optional, key_type=key_type, @@ -920,7 +923,7 @@ def _node_wrap( node = ListConfig( content=value, key=key, - parent=parent, + parent=dummy, is_optional=is_optional, element_type=element_type, ref_type=ref_type, @@ -932,33 +935,34 @@ def _node_wrap( is_optional=is_optional, content=value, key=key, - parent=parent, + parent=dummy, key_type=key_type, element_type=element_type, ) elif type_ == Any or type_ is None: - node = AnyNode(value=value, key=key, parent=parent, is_optional=is_optional) + node = AnyNode(value=value, key=key, parent=dummy, is_optional=is_optional) elif issubclass(type_, Enum): node = EnumNode( enum_type=type_, value=value, key=key, - parent=parent, + parent=dummy, is_optional=is_optional, ) elif type_ == int: - node = IntegerNode(value=value, key=key, parent=parent, is_optional=is_optional) + node = IntegerNode(value=value, key=key, parent=dummy, is_optional=is_optional) elif type_ == float: - node = FloatNode(value=value, key=key, parent=parent, is_optional=is_optional) + node = FloatNode(value=value, key=key, parent=dummy, is_optional=is_optional) elif type_ == bool: - node = BooleanNode(value=value, key=key, parent=parent, is_optional=is_optional) + node = BooleanNode(value=value, key=key, parent=dummy, is_optional=is_optional) elif type_ == str: - node = StringNode(value=value, key=key, parent=parent, is_optional=is_optional) + node = StringNode(value=value, key=key, parent=dummy, is_optional=is_optional) else: - if parent is not None and parent._get_flag("allow_objects") is True: - node = AnyNode(value=value, key=key, parent=parent, is_optional=is_optional) + if allow_objects: + node = AnyNode(value=value, key=key, parent=dummy, is_optional=is_optional) else: raise ValidationError(f"Unexpected object type : {type_str(type_)}") + node._set_parent(parent) return node diff --git a/tests/test_struct.py b/tests/test_struct.py index d15e23406..6bcc9f0e8 100644 --- a/tests/test_struct.py +++ b/tests/test_struct.py @@ -62,3 +62,10 @@ def test_struct_contain_missing() -> None: @mark.parametrize("cfg", [{}, OmegaConf.create({}, flags={"struct": True})]) def test_struct_dict_get(cfg: Any) -> None: assert cfg.get("z") is None + + +def test_struct_dict_assign() -> None: + cfg = OmegaConf.create({"a": {}}) + OmegaConf.set_struct(cfg, True) + cfg.a = {"b": 10} + assert cfg.a == {"b": 10} From 2d768fe584b30a47a11c1533076fe47e08fa0a44 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 14:23:43 -0800 Subject: [PATCH 3/9] minor: cleanr usage of flag_override in DictConfig and ListConfig --- omegaconf/dictconfig.py | 7 +++---- omegaconf/listconfig.py | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py index dd6a0c330..09a1d6833 100644 --- a/omegaconf/dictconfig.py +++ b/omegaconf/dictconfig.py @@ -641,10 +641,9 @@ def _set_value_impl( self._metadata.flags = copy.deepcopy(flags) # disable struct and readonly for the construction phase # retaining other flags like allow_objects. The real flags are restored at the end of this function - with flag_override(self, "struct", False): - with flag_override(self, "readonly", False): - for k, v in value.__dict__["_content"].items(): - self.__setitem__(k, v) + with flag_override(self, ["struct", "readonly"], False): + for k, v in value.__dict__["_content"].items(): + self.__setitem__(k, v) elif isinstance(value, dict): for k, v in value.items(): diff --git a/omegaconf/listconfig.py b/omegaconf/listconfig.py index 7829c6c5a..eca2c44bb 100644 --- a/omegaconf/listconfig.py +++ b/omegaconf/listconfig.py @@ -593,10 +593,9 @@ def _set_value_impl( self._metadata.flags = copy.deepcopy(flags) # disable struct and readonly for the construction phase # retaining other flags like allow_objects. The real flags are restored at the end of this function - with flag_override(self, "struct", False): - with flag_override(self, "readonly", False): - for item in value._iter_ex(resolve=False): - self.append(item) + with flag_override(self, ["struct", "readonly"], False): + for item in value._iter_ex(resolve=False): + self.append(item) elif is_primitive_list(value): for item in value: self.append(item) From f86f59063ab81d82b4e9df46dcace58e79994215 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 16:15:31 -0800 Subject: [PATCH 4/9] added a test for DictConfig creation with a parent in struct mode --- omegaconf/dictconfig.py | 5 +++-- omegaconf/omegaconf.py | 2 +- tests/test_basic_ops_dict.py | 8 ++++++++ 3 files changed, 12 insertions(+), 3 deletions(-) diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py index 09a1d6833..5d9bb523f 100644 --- a/omegaconf/dictconfig.py +++ b/omegaconf/dictconfig.py @@ -646,8 +646,9 @@ def _set_value_impl( self.__setitem__(k, v) elif isinstance(value, dict): - for k, v in value.items(): - self.__setitem__(k, v) + with flag_override(self, "struct", False): + for k, v in value.items(): + self.__setitem__(k, v) else: # pragma: no cover msg = f"Unsupported value type : {value}" raise ValidationError(msg) diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index 842ffabbc..b7fd076d0 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -899,7 +899,7 @@ def _node_wrap( ) -> Node: node: Node allow_objects = parent is not None and parent._get_flag("allow_objects") is True - dummy = OmegaConf.create(flags={"allow_objects": allow_objects}) + dummy = parent # OmegaConf.create(flags={"allow_objects": allow_objects}) is_dict = is_primitive_dict(value) or is_dict_annotation(type_) is_list = ( diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index c80929594..397c9bfb8 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -1023,3 +1023,11 @@ def test_dict_getitem_not_found() -> None: def test_dict_getitem_none_output() -> None: cfg = OmegaConf.create({"a": None}) assert cfg["a"] is None + + +def test_dictconfig_creation_with_struct_parent() -> None: + parent = OmegaConf.create({"a": 10}) + OmegaConf.set_struct(parent, True) + d = {"b": 0} + cfg = DictConfig(d, parent=parent) + assert cfg == d From bcd1d505fe6d2347e054bbacb5f3fc3b9085c18a Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 16:18:23 -0800 Subject: [PATCH 5/9] reverted changes to node_wrap --- omegaconf/omegaconf.py | 26 +++++++++++--------------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/omegaconf/omegaconf.py b/omegaconf/omegaconf.py index b7fd076d0..d78e2605e 100644 --- a/omegaconf/omegaconf.py +++ b/omegaconf/omegaconf.py @@ -898,9 +898,6 @@ def _node_wrap( ref_type: Any = Any, ) -> Node: node: Node - allow_objects = parent is not None and parent._get_flag("allow_objects") is True - dummy = parent # OmegaConf.create(flags={"allow_objects": allow_objects}) - is_dict = is_primitive_dict(value) or is_dict_annotation(type_) is_list = ( type(value) in (list, tuple) @@ -912,7 +909,7 @@ def _node_wrap( node = DictConfig( content=value, key=key, - parent=dummy, + parent=parent, ref_type=type_, is_optional=is_optional, key_type=key_type, @@ -923,7 +920,7 @@ def _node_wrap( node = ListConfig( content=value, key=key, - parent=dummy, + parent=parent, is_optional=is_optional, element_type=element_type, ref_type=ref_type, @@ -935,34 +932,33 @@ def _node_wrap( is_optional=is_optional, content=value, key=key, - parent=dummy, + parent=parent, key_type=key_type, element_type=element_type, ) elif type_ == Any or type_ is None: - node = AnyNode(value=value, key=key, parent=dummy, is_optional=is_optional) + node = AnyNode(value=value, key=key, parent=parent, is_optional=is_optional) elif issubclass(type_, Enum): node = EnumNode( enum_type=type_, value=value, key=key, - parent=dummy, + parent=parent, is_optional=is_optional, ) elif type_ == int: - node = IntegerNode(value=value, key=key, parent=dummy, is_optional=is_optional) + node = IntegerNode(value=value, key=key, parent=parent, is_optional=is_optional) elif type_ == float: - node = FloatNode(value=value, key=key, parent=dummy, is_optional=is_optional) + node = FloatNode(value=value, key=key, parent=parent, is_optional=is_optional) elif type_ == bool: - node = BooleanNode(value=value, key=key, parent=dummy, is_optional=is_optional) + node = BooleanNode(value=value, key=key, parent=parent, is_optional=is_optional) elif type_ == str: - node = StringNode(value=value, key=key, parent=dummy, is_optional=is_optional) + node = StringNode(value=value, key=key, parent=parent, is_optional=is_optional) else: - if allow_objects: - node = AnyNode(value=value, key=key, parent=dummy, is_optional=is_optional) + if parent is not None and parent._get_flag("allow_objects") is True: + node = AnyNode(value=value, key=key, parent=parent, is_optional=is_optional) else: raise ValidationError(f"Unexpected object type : {type_str(type_)}") - node._set_parent(parent) return node From f0156da85342b9c630a8c431e70a71c43a4bf80e Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 16:24:30 -0800 Subject: [PATCH 6/9] handlign readonly parent --- omegaconf/dictconfig.py | 2 +- tests/test_basic_ops_dict.py | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py index 5d9bb523f..3cb0dedf8 100644 --- a/omegaconf/dictconfig.py +++ b/omegaconf/dictconfig.py @@ -646,7 +646,7 @@ def _set_value_impl( self.__setitem__(k, v) elif isinstance(value, dict): - with flag_override(self, "struct", False): + with flag_override(self, ["struct", "readonly"], False): for k, v in value.items(): self.__setitem__(k, v) else: # pragma: no cover diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 397c9bfb8..954414f86 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -1025,9 +1025,10 @@ def test_dict_getitem_none_output() -> None: assert cfg["a"] is None -def test_dictconfig_creation_with_struct_parent() -> None: +@pytest.mark.parametrize("flag", ["struct", "readonly"]) +def test_dictconfig_creation_with_parent_flag(flag: str) -> None: parent = OmegaConf.create({"a": 10}) - OmegaConf.set_struct(parent, True) + parent._set_flag(flag, True) d = {"b": 0} cfg = DictConfig(d, parent=parent) assert cfg == d From 4839d3b64ea509fb27ca2958e5c3a34dd0dc62aa Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 16:31:12 -0800 Subject: [PATCH 7/9] fixed list case --- news/586.bugfix | 3 +-- omegaconf/listconfig.py | 5 +++-- tests/test_basic_ops_list.py | 9 +++++++++ 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/news/586.bugfix b/news/586.bugfix index ffdaf308b..ff20d1567 100644 --- a/news/586.bugfix +++ b/news/586.bugfix @@ -1,2 +1 @@ -Assignment of a dict to an existing node in a parent in struct mode no longer raises ValidationError - +Assignment of a dict/list to an existing node in a parent in struct mode no longer raises ValidationError diff --git a/omegaconf/listconfig.py b/omegaconf/listconfig.py index eca2c44bb..ec9d3a36a 100644 --- a/omegaconf/listconfig.py +++ b/omegaconf/listconfig.py @@ -597,8 +597,9 @@ def _set_value_impl( for item in value._iter_ex(resolve=False): self.append(item) elif is_primitive_list(value): - for item in value: - self.append(item) + with flag_override(self, ["struct", "readonly"], False): + for item in value: + self.append(item) @staticmethod def _list_eq(l1: Optional["ListConfig"], l2: Optional["ListConfig"]) -> bool: diff --git a/tests/test_basic_ops_list.py b/tests/test_basic_ops_list.py index 8c59fd805..0c292b686 100644 --- a/tests/test_basic_ops_list.py +++ b/tests/test_basic_ops_list.py @@ -712,3 +712,12 @@ def test_shallow_copy_none() -> None: c._set_value([1]) assert c[0] == 1 assert cfg._is_none() + + +@pytest.mark.parametrize("flag", ["struct", "readonly"]) +def test_listconfig_creation_with_parent_flag(flag: str) -> None: + parent = OmegaConf.create([]) + parent._set_flag(flag, True) + d = [1, 2, 3] + cfg = ListConfig(d, parent=parent) + assert cfg == d From ad9c5c39ebb8b3fb152eac3695de703c9b8f5b62 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 16:35:52 -0800 Subject: [PATCH 8/9] another test for readonly --- tests/test_readonly.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_readonly.py b/tests/test_readonly.py index 7a45c450a..08a68475e 100644 --- a/tests/test_readonly.py +++ b/tests/test_readonly.py @@ -16,6 +16,12 @@ raises(ReadonlyConfigError, match="a"), id="dict_setitem", ), + pytest.param( + {"a": None}, + lambda c: c.__setitem__("a", {"b": 10}), + raises(ReadonlyConfigError, match="a"), + id="dict_setitem", + ), pytest.param( {"a": {"b": {"c": 1}}}, lambda c: c.__getattr__("a").__getattr__("b").__setitem__("c", 1), From 1ef55217d827b6fa08d78902cfed33bb85999fe2 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Tue, 9 Mar 2021 18:28:38 -0800 Subject: [PATCH 9/9] fixed case for Structured Config --- omegaconf/dictconfig.py | 15 +++++++-------- tests/test_basic_ops_dict.py | 8 ++++---- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py index 3cb0dedf8..9af0564fc 100644 --- a/omegaconf/dictconfig.py +++ b/omegaconf/dictconfig.py @@ -629,18 +629,16 @@ def _set_value_impl( self.__dict__["_content"] = {} if is_structured_config(value): self._metadata.object_type = None - data = get_structured_config_data( - value, - allow_objects=self._get_flag("allow_objects"), - ) - for k, v in data.items(): - self.__setitem__(k, v) + ao = self._get_flag("allow_objects") + data = get_structured_config_data(value, allow_objects=ao) + with flag_override(self, ["struct", "readonly"], False): + for k, v in data.items(): + self.__setitem__(k, v) self._metadata.object_type = get_type_of(value) + elif isinstance(value, DictConfig): self.__dict__["_metadata"] = copy.deepcopy(value._metadata) self._metadata.flags = copy.deepcopy(flags) - # disable struct and readonly for the construction phase - # retaining other flags like allow_objects. The real flags are restored at the end of this function with flag_override(self, ["struct", "readonly"], False): for k, v in value.__dict__["_content"].items(): self.__setitem__(k, v) @@ -649,6 +647,7 @@ def _set_value_impl( with flag_override(self, ["struct", "readonly"], False): for k, v in value.items(): self.__setitem__(k, v) + else: # pragma: no cover msg = f"Unsupported value type : {value}" raise ValidationError(msg) diff --git a/tests/test_basic_ops_dict.py b/tests/test_basic_ops_dict.py index 954414f86..beab06e7f 100644 --- a/tests/test_basic_ops_dict.py +++ b/tests/test_basic_ops_dict.py @@ -1025,10 +1025,10 @@ def test_dict_getitem_none_output() -> None: assert cfg["a"] is None +@pytest.mark.parametrize("data", [{"b": 0}, User]) @pytest.mark.parametrize("flag", ["struct", "readonly"]) -def test_dictconfig_creation_with_parent_flag(flag: str) -> None: +def test_dictconfig_creation_with_parent_flag(flag: str, data: Any) -> None: parent = OmegaConf.create({"a": 10}) parent._set_flag(flag, True) - d = {"b": 0} - cfg = DictConfig(d, parent=parent) - assert cfg == d + cfg = DictConfig(data, parent=parent) + assert cfg == data