From 2610a53bc505804cccf9559b5f6d3a1b58609bb2 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Fri, 26 Feb 2021 07:04:43 -0600 Subject: [PATCH 1/7] rebased against master --- omegaconf/_utils.py | 44 ++++++++++++------- .../structured_conf/test_structured_basic.py | 7 +++ tests/test_errors.py | 27 +++++++----- 3 files changed, 51 insertions(+), 27 deletions(-) diff --git a/omegaconf/_utils.py b/omegaconf/_utils.py index a01051d5c..5bc3dacaa 100644 --- a/omegaconf/_utils.py +++ b/omegaconf/_utils.py @@ -15,6 +15,7 @@ ConfigTypeError, ConfigValueError, OmegaConfBaseException, + ValidationError, ) from .grammar_parser import parse @@ -195,13 +196,15 @@ def _resolve_forward(type_: Type[Any], module: str) -> Type[Any]: def get_attr_data(obj: Any, allow_objects: Optional[bool] = None) -> Dict[str, Any]: from omegaconf.omegaconf import OmegaConf, _maybe_wrap + obj_type = get_type_of(obj) + flags = {"allow_objects": allow_objects} if allow_objects is not None else {} dummy_parent = OmegaConf.create(flags=flags) + dummy_parent._metadata.object_type = obj_type from omegaconf import MISSING d = {} is_type = isinstance(obj, type) - obj_type = obj if is_type else type(obj) for name, attrib in attr.fields_dict(obj_type).items(): is_optional, type_ = _resolve_optional(attrib.type) type_ = _resolve_forward(type_, obj.__module__) @@ -217,13 +220,16 @@ def get_attr_data(obj: Any, allow_objects: Optional[bool] = None) -> Dict[str, A ) format_and_raise(node=None, key=None, value=value, cause=e, msg=str(e)) - d[name] = _maybe_wrap( - ref_type=type_, - is_optional=is_optional, - key=name, - value=value, - parent=dummy_parent, - ) + try: + d[name] = _maybe_wrap( + ref_type=type_, + is_optional=is_optional, + key=name, + value=value, + parent=dummy_parent, + ) + except ValidationError as ex: + dummy_parent._format_and_raise(key=name, value=value, cause=ex) d[name]._set_parent(None) return d @@ -233,10 +239,13 @@ def get_dataclass_data( ) -> Dict[str, Any]: from omegaconf.omegaconf import MISSING, OmegaConf, _maybe_wrap + obj_type = get_type_of(obj) + flags = {"allow_objects": allow_objects} if allow_objects is not None else {} dummy_parent = OmegaConf.create({}, flags=flags) + dummy_parent._metadata.object_type = obj_type d = {} - resolved_hints = get_type_hints(get_type_of(obj)) + resolved_hints = get_type_hints(obj_type) for field in dataclasses.fields(obj): name = field.name is_optional, type_ = _resolve_optional(resolved_hints[field.name]) @@ -257,13 +266,16 @@ def get_dataclass_data( f"Union types are not supported:\n{name}: {type_str(type_)}" ) format_and_raise(node=None, key=None, value=value, cause=e, msg=str(e)) - d[name] = _maybe_wrap( - ref_type=type_, - is_optional=is_optional, - key=name, - value=value, - parent=dummy_parent, - ) + try: + d[name] = _maybe_wrap( + ref_type=type_, + is_optional=is_optional, + key=name, + value=value, + parent=dummy_parent, + ) + except ValidationError as ex: + dummy_parent._format_and_raise(key=name, value=value, cause=ex) d[name]._set_parent(None) return d diff --git a/tests/structured_conf/test_structured_basic.py b/tests/structured_conf/test_structured_basic.py index a46e257fa..97b7f41bf 100644 --- a/tests/structured_conf/test_structured_basic.py +++ b/tests/structured_conf/test_structured_basic.py @@ -48,6 +48,13 @@ def test_error_on_non_structured_nested_config_class(self, module: Any) -> None: assert list(ret.keys()) == ["bar"] assert ret.bar == module.NotStructuredConfig() + def test_error_on_creation_with_bad_value_type(self, module: Any) -> None: + with raises( + ValidationError, + match=re.escape("Value 'seven' could not be converted to Integer"), + ): + OmegaConf.structured(module.User(age="seven")) + def test_assignment_of_subclass(self, module: Any) -> None: cfg = OmegaConf.create({"plugin": module.Plugin}) cfg.plugin = OmegaConf.structured(module.ConcretePlugin) diff --git a/tests/test_errors.py b/tests/test_errors.py index 1ccc811bd..ef24ae5be 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -532,30 +532,31 @@ def finalize(self, cfg: Any) -> None: pytest.param( Expected( create=lambda: None, - op=lambda cfg: OmegaConf.structured(NotOptionalInt), + op=lambda _: OmegaConf.structured(NotOptionalInt), exception_type=ValidationError, msg="Non optional field cannot be assigned None", - object_type_str=None, - ref_type_str=None, + key="foo", + object_type=NotOptionalInt, + parent_node=lambda _: {}, # dummy parent ), id="dict:create_none_optional_with_none", ), pytest.param( Expected( create=lambda: None, - op=lambda cfg: OmegaConf.structured(NotOptionalInt), + op=lambda _: OmegaConf.structured(NotOptionalInt), exception_type=ValidationError, - object_type=None, + object_type=NotOptionalInt, msg="Non optional field cannot be assigned None", - object_type_str="NotOptionalInt", - ref_type_str=None, + key="foo", + parent_node=lambda _: {}, # dummy parent ), id="dict:create:not_optional_int_field_with_none", ), pytest.param( Expected( create=lambda: None, - op=lambda cfg: OmegaConf.structured(NotOptionalA), + op=lambda _: OmegaConf.structured(NotOptionalA), exception_type=ValidationError, object_type=None, key=None, @@ -569,32 +570,35 @@ def finalize(self, cfg: Any) -> None: pytest.param( Expected( create=lambda: None, - op=lambda cfg: OmegaConf.structured(IllegalType), + op=lambda _: OmegaConf.structured(IllegalType), exception_type=ValidationError, msg="Input class 'IllegalType' is not a structured config. did you forget to decorate it as a dataclass?", object_type_str=None, ref_type_str=None, + parent_node=lambda _: None, ), id="dict_create_from_illegal_type", ), pytest.param( Expected( create=lambda: None, - op=lambda cfg: OmegaConf.structured(IllegalType()), + op=lambda _: OmegaConf.structured(IllegalType()), exception_type=ValidationError, msg="Object of unsupported type: 'IllegalType'", object_type_str=None, ref_type_str=None, + parent_node=lambda _: None, ), id="structured:create_from_unsupported_object", ), pytest.param( Expected( create=lambda: None, - op=lambda cfg: OmegaConf.structured(UnionError), + op=lambda _: OmegaConf.structured(UnionError), exception_type=ValueError, msg="Union types are not supported:\nx: Union[int, str]", num_lines=3, + parent_node=lambda _: None, ), id="structured:create_with_union_error", ), @@ -607,6 +611,7 @@ def finalize(self, cfg: Any) -> None: msg="Invalid type assigned : int is not a subclass of ConcretePlugin. value: 1", low_level=True, ref_type=Optional[ConcretePlugin], + parent_node=lambda _: {}, # dummy parent ), id="dict:set_value:reftype_mismatch", ), From cb24275cce4a02b805d1517162caa7472936f5e5 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Fri, 26 Feb 2021 07:20:14 -0600 Subject: [PATCH 2/7] test error message for deeper nesting --- tests/test_errors.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_errors.py b/tests/test_errors.py index ef24ae5be..be04c5fce 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -579,6 +579,20 @@ def finalize(self, cfg: Any) -> None: ), id="dict_create_from_illegal_type", ), + pytest.param( + Expected( + create=lambda: None, + op=lambda _: OmegaConf.structured( + ConcretePlugin(params=ConcretePlugin.FoobarParams(foo="x")) # type: ignore + ), + exception_type=ValidationError, + msg="Value 'x' could not be converted to Integer", + object_type=ConcretePlugin.FoobarParams, + key="foo", + parent_node=lambda _: {}, # dummy parent + ), + id="structured:create_with_invalid_value", + ), pytest.param( Expected( create=lambda: None, From a45e314d46cae856f1a020eada02cb5fe1a90cb2 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Wed, 24 Feb 2021 20:59:39 -0600 Subject: [PATCH 3/7] revert/remove some parent_node fields --- tests/test_errors.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/tests/test_errors.py b/tests/test_errors.py index be04c5fce..125a1bea9 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -575,7 +575,6 @@ def finalize(self, cfg: Any) -> None: msg="Input class 'IllegalType' is not a structured config. did you forget to decorate it as a dataclass?", object_type_str=None, ref_type_str=None, - parent_node=lambda _: None, ), id="dict_create_from_illegal_type", ), @@ -601,7 +600,6 @@ def finalize(self, cfg: Any) -> None: msg="Object of unsupported type: 'IllegalType'", object_type_str=None, ref_type_str=None, - parent_node=lambda _: None, ), id="structured:create_from_unsupported_object", ), @@ -612,7 +610,6 @@ def finalize(self, cfg: Any) -> None: exception_type=ValueError, msg="Union types are not supported:\nx: Union[int, str]", num_lines=3, - parent_node=lambda _: None, ), id="structured:create_with_union_error", ), @@ -625,7 +622,6 @@ def finalize(self, cfg: Any) -> None: msg="Invalid type assigned : int is not a subclass of ConcretePlugin. value: 1", low_level=True, ref_type=Optional[ConcretePlugin], - parent_node=lambda _: {}, # dummy parent ), id="dict:set_value:reftype_mismatch", ), From f2a1bfd2af08212cb2b7e294e24f63bdf8fb28a4 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Wed, 3 Mar 2021 01:48:05 -0600 Subject: [PATCH 4/7] use format_and_raise() directly --- omegaconf/_utils.py | 4 ++-- tests/test_errors.py | 9 +++------ 2 files changed, 5 insertions(+), 8 deletions(-) diff --git a/omegaconf/_utils.py b/omegaconf/_utils.py index 5bc3dacaa..90879296f 100644 --- a/omegaconf/_utils.py +++ b/omegaconf/_utils.py @@ -229,7 +229,7 @@ def get_attr_data(obj: Any, allow_objects: Optional[bool] = None) -> Dict[str, A parent=dummy_parent, ) except ValidationError as ex: - dummy_parent._format_and_raise(key=name, value=value, cause=ex) + format_and_raise(node=None, key=name, value=value, cause=ex, msg=str(ex)) d[name]._set_parent(None) return d @@ -275,7 +275,7 @@ def get_dataclass_data( parent=dummy_parent, ) except ValidationError as ex: - dummy_parent._format_and_raise(key=name, value=value, cause=ex) + format_and_raise(node=None, key=name, value=value, cause=ex, msg=str(ex)) d[name]._set_parent(None) return d diff --git a/tests/test_errors.py b/tests/test_errors.py index 125a1bea9..21088ef05 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -536,8 +536,7 @@ def finalize(self, cfg: Any) -> None: exception_type=ValidationError, msg="Non optional field cannot be assigned None", key="foo", - object_type=NotOptionalInt, - parent_node=lambda _: {}, # dummy parent + full_key="", ), id="dict:create_none_optional_with_none", ), @@ -546,10 +545,9 @@ def finalize(self, cfg: Any) -> None: create=lambda: None, op=lambda _: OmegaConf.structured(NotOptionalInt), exception_type=ValidationError, - object_type=NotOptionalInt, msg="Non optional field cannot be assigned None", key="foo", - parent_node=lambda _: {}, # dummy parent + full_key="", ), id="dict:create:not_optional_int_field_with_none", ), @@ -586,9 +584,8 @@ def finalize(self, cfg: Any) -> None: ), exception_type=ValidationError, msg="Value 'x' could not be converted to Integer", - object_type=ConcretePlugin.FoobarParams, key="foo", - parent_node=lambda _: {}, # dummy parent + full_key="", ), id="structured:create_with_invalid_value", ), From 703b6df88795df9b93e1dfc4c76bc135a594e6a0 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Thu, 4 Mar 2021 19:06:12 -0600 Subject: [PATCH 5/7] revert dummy_parent obj_type --- omegaconf/_utils.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/omegaconf/_utils.py b/omegaconf/_utils.py index 90879296f..770616e96 100644 --- a/omegaconf/_utils.py +++ b/omegaconf/_utils.py @@ -196,15 +196,13 @@ def _resolve_forward(type_: Type[Any], module: str) -> Type[Any]: def get_attr_data(obj: Any, allow_objects: Optional[bool] = None) -> Dict[str, Any]: from omegaconf.omegaconf import OmegaConf, _maybe_wrap - obj_type = get_type_of(obj) - flags = {"allow_objects": allow_objects} if allow_objects is not None else {} dummy_parent = OmegaConf.create(flags=flags) - dummy_parent._metadata.object_type = obj_type from omegaconf import MISSING d = {} is_type = isinstance(obj, type) + obj_type = get_type_of(obj) for name, attrib in attr.fields_dict(obj_type).items(): is_optional, type_ = _resolve_optional(attrib.type) type_ = _resolve_forward(type_, obj.__module__) @@ -239,13 +237,10 @@ def get_dataclass_data( ) -> Dict[str, Any]: from omegaconf.omegaconf import MISSING, OmegaConf, _maybe_wrap - obj_type = get_type_of(obj) - flags = {"allow_objects": allow_objects} if allow_objects is not None else {} dummy_parent = OmegaConf.create({}, flags=flags) - dummy_parent._metadata.object_type = obj_type d = {} - resolved_hints = get_type_hints(obj_type) + resolved_hints = get_type_hints(get_type_of(obj)) for field in dataclasses.fields(obj): name = field.name is_optional, type_ = _resolve_optional(resolved_hints[field.name]) From 35731c126e0f3e762d6b6b95cababc69ff2eeb28 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Thu, 4 Mar 2021 19:42:41 -0600 Subject: [PATCH 6/7] clean leftovers from previous attempts --- omegaconf/_utils.py | 2 +- tests/test_errors.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/omegaconf/_utils.py b/omegaconf/_utils.py index 770616e96..2dec6c1e9 100644 --- a/omegaconf/_utils.py +++ b/omegaconf/_utils.py @@ -202,7 +202,7 @@ def get_attr_data(obj: Any, allow_objects: Optional[bool] = None) -> Dict[str, A d = {} is_type = isinstance(obj, type) - obj_type = get_type_of(obj) + obj_type = obj if is_type else type(obj) for name, attrib in attr.fields_dict(obj_type).items(): is_optional, type_ = _resolve_optional(attrib.type) type_ = _resolve_forward(type_, obj.__module__) diff --git a/tests/test_errors.py b/tests/test_errors.py index 21088ef05..f68f29528 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -554,7 +554,7 @@ def finalize(self, cfg: Any) -> None: pytest.param( Expected( create=lambda: None, - op=lambda _: OmegaConf.structured(NotOptionalA), + op=lambda cfg: OmegaConf.structured(NotOptionalA), exception_type=ValidationError, object_type=None, key=None, @@ -568,7 +568,7 @@ def finalize(self, cfg: Any) -> None: pytest.param( Expected( create=lambda: None, - op=lambda _: OmegaConf.structured(IllegalType), + op=lambda cfg: OmegaConf.structured(IllegalType), exception_type=ValidationError, msg="Input class 'IllegalType' is not a structured config. did you forget to decorate it as a dataclass?", object_type_str=None, @@ -592,7 +592,7 @@ def finalize(self, cfg: Any) -> None: pytest.param( Expected( create=lambda: None, - op=lambda _: OmegaConf.structured(IllegalType()), + op=lambda cfg: OmegaConf.structured(IllegalType()), exception_type=ValidationError, msg="Object of unsupported type: 'IllegalType'", object_type_str=None, @@ -603,7 +603,7 @@ def finalize(self, cfg: Any) -> None: pytest.param( Expected( create=lambda: None, - op=lambda _: OmegaConf.structured(UnionError), + op=lambda cfg: OmegaConf.structured(UnionError), exception_type=ValueError, msg="Union types are not supported:\nx: Union[int, str]", num_lines=3, From 550861cfef606fcd50976fb2e4d23588b9e75f92 Mon Sep 17 00:00:00 2001 From: Jasha <8935917+Jasha10@users.noreply.github.com> Date: Thu, 4 Mar 2021 22:46:19 -0600 Subject: [PATCH 7/7] add 435.bugfix news fragment --- news/435.bugfix | 1 + 1 file changed, 1 insertion(+) create mode 100644 news/435.bugfix diff --git a/news/435.bugfix b/news/435.bugfix new file mode 100644 index 000000000..4065af67e --- /dev/null +++ b/news/435.bugfix @@ -0,0 +1 @@ +When initializing a Structured Config with an incorrectly-typed value, the resulting ValidationError now properly reports the offending value in its error message.