From 3b04bf9e1a9fa6ca9779bf42baa95faa24b3edc8 Mon Sep 17 00:00:00 2001 From: Omry Yadan Date: Fri, 30 Apr 2021 10:52:24 -0700 Subject: [PATCH] improve error message when creating a DictConfig from a bad dataclass (#701) * improve error message when creating a DictConfig from a bad Structured Config * in DictConfig.__init__ and ListConfig.__init__: pass key to format_and_raise * news fragment Co-authored-by: Jasha <8935917+Jasha10@users.noreply.github.com> --- news/697.bugfix | 1 + omegaconf/_utils.py | 23 ++++++++++++++++------- omegaconf/dictconfig.py | 2 +- omegaconf/listconfig.py | 2 +- tests/__init__.py | 10 ++++++++++ tests/test_errors.py | 36 ++++++++++++++++++++++++++++++++---- 6 files changed, 61 insertions(+), 13 deletions(-) create mode 100644 news/697.bugfix diff --git a/news/697.bugfix b/news/697.bugfix new file mode 100644 index 000000000..a88460156 --- /dev/null +++ b/news/697.bugfix @@ -0,0 +1 @@ +Improve error message when creating a config from a Structured Config that fails type validation \ No newline at end of file diff --git a/omegaconf/_utils.py b/omegaconf/_utils.py index 82294d660..d9177bd75 100644 --- a/omegaconf/_utils.py +++ b/omegaconf/_utils.py @@ -24,6 +24,7 @@ ConfigIndexError, ConfigTypeError, ConfigValueError, + GrammarParseError, OmegaConfBaseException, ValidationError, ) @@ -265,12 +266,15 @@ def get_attr_data(obj: Any, allow_objects: Optional[bool] = None) -> Dict[str, A from omegaconf.omegaconf import OmegaConf, _maybe_wrap flags = {"allow_objects": allow_objects} if allow_objects is not None else {} - dummy_parent = OmegaConf.create(flags=flags) + from omegaconf import MISSING d = {} is_type = isinstance(obj, type) obj_type = obj if is_type else type(obj) + dummy_parent = OmegaConf.create({}, flags=flags) + dummy_parent._metadata.object_type = obj_type + for name, attrib in attr.fields_dict(obj_type).items(): is_optional, type_ = _resolve_optional(attrib.type) type_ = _resolve_forward(type_, obj.__module__) @@ -294,8 +298,10 @@ def get_attr_data(obj: Any, allow_objects: Optional[bool] = None) -> Dict[str, A value=value, parent=dummy_parent, ) - except ValidationError as ex: - format_and_raise(node=None, key=name, value=value, cause=ex, msg=str(ex)) + except (ValidationError, GrammarParseError) as ex: + format_and_raise( + node=dummy_parent, key=name, value=value, cause=ex, msg=str(ex) + ) d[name]._set_parent(None) dict_subclass_data = extract_dict_subclass_data(obj=obj, parent=dummy_parent) if dict_subclass_data is not None: @@ -313,9 +319,10 @@ def get_dataclass_data( from omegaconf.omegaconf import MISSING, OmegaConf, _maybe_wrap flags = {"allow_objects": allow_objects} if allow_objects is not None else {} - dummy_parent = OmegaConf.create({}, flags=flags) d = {} obj_type = get_type_of(obj) + dummy_parent = OmegaConf.create({}, flags=flags) + dummy_parent._metadata.object_type = obj_type resolved_hints = get_type_hints(obj_type) for field in dataclasses.fields(obj): name = field.name @@ -345,8 +352,10 @@ def get_dataclass_data( value=value, parent=dummy_parent, ) - except ValidationError as ex: - format_and_raise(node=None, key=name, value=value, cause=ex, msg=str(ex)) + except (ValidationError, GrammarParseError) as ex: + format_and_raise( + node=dummy_parent, key=name, value=value, cause=ex, msg=str(ex) + ) d[name]._set_parent(None) dict_subclass_data = extract_dict_subclass_data(obj=obj, parent=dummy_parent) if dict_subclass_data is not None: @@ -721,7 +730,7 @@ def format_and_raise( child_node: Optional[Node] = None if node is None: - full_key = "" + full_key = key if key is not None else "" object_type = None ref_type = None ref_type_str = None diff --git a/omegaconf/dictconfig.py b/omegaconf/dictconfig.py index af968face..575253569 100644 --- a/omegaconf/dictconfig.py +++ b/omegaconf/dictconfig.py @@ -110,7 +110,7 @@ def __init__( self.__dict__["_metadata"] = metadata self._set_value(content, flags=flags) except Exception as ex: - format_and_raise(node=None, key=None, value=None, cause=ex, msg=str(ex)) + format_and_raise(node=None, key=key, value=None, cause=ex, msg=str(ex)) def __deepcopy__(self, memo: Dict[int, Any]) -> "DictConfig": res = DictConfig(None) diff --git a/omegaconf/listconfig.py b/omegaconf/listconfig.py index b71c69878..b6ce03f38 100644 --- a/omegaconf/listconfig.py +++ b/omegaconf/listconfig.py @@ -77,7 +77,7 @@ def __init__( self.__dict__["_content"] = None self._set_value(value=content, flags=flags) except Exception as ex: - format_and_raise(node=None, key=None, value=None, cause=ex, msg=str(ex)) + format_and_raise(node=None, key=key, value=None, cause=ex, msg=str(ex)) def _validate_get(self, key: Any, value: Any = None) -> None: if not isinstance(key, (int, slice)): diff --git a/tests/__init__.py b/tests/__init__.py index 1d10b5c16..be1abf765 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -117,6 +117,16 @@ class UnionError: x: Union[int, str] = 10 +@dataclass +class StructuredWithBadDict: + foo: Dict[str, str] = 123 # type: ignore + + +@dataclass +class StructuredWithBadList: + foo: List[str] = 123 # type: ignore + + @dataclass class MissingList: list: List[str] = MISSING diff --git a/tests/test_errors.py b/tests/test_errors.py index 7c3c2d8d4..71b02b2ec 100644 --- a/tests/test_errors.py +++ b/tests/test_errors.py @@ -39,6 +39,8 @@ Package, Plugin, Str2Int, + StructuredWithBadDict, + StructuredWithBadList, StructuredWithMissing, SubscriptedDict, UnionError, @@ -605,7 +607,9 @@ def finalize(self, cfg: Any) -> None: exception_type=ValidationError, msg="Non optional field cannot be assigned None", key="foo", - full_key="", + full_key="foo", + parent_node=lambda _: {}, + object_type=NotOptionalInt, ), id="dict:create_none_optional_with_none", ), @@ -616,7 +620,9 @@ def finalize(self, cfg: Any) -> None: exception_type=ValidationError, msg="Non optional field cannot be assigned None", key="foo", - full_key="", + full_key="foo", + parent_node=lambda _: {}, + object_type=NotOptionalInt, ), id="dict:create:not_optional_int_field_with_none", ), @@ -654,7 +660,9 @@ def finalize(self, cfg: Any) -> None: exception_type=ValidationError, msg="Value 'x' could not be converted to Integer", key="foo", - full_key="", + full_key="foo", + parent_node=lambda _: {}, + object_type=ConcretePlugin.FoobarParams, ), id="structured:create_with_invalid_value", ), @@ -795,11 +803,31 @@ def finalize(self, cfg: Any) -> None: msg="Value 'qux' could not be converted to Integer", object_type=None, key="bar", - full_key="", + full_key="bar", parent_node=lambda cfg: None, ), id="structured,Dict_subclass:bad_value_type", ), + param( + Expected( + create=lambda: None, + op=lambda _: OmegaConf.structured(StructuredWithBadDict), + exception_type=ValidationError, + msg="Cannot assign int to Dict[str, str]", + key="foo", + ), + id="structured,bad_default_value_for_dict", + ), + param( + Expected( + create=lambda: None, + op=lambda _: OmegaConf.structured(StructuredWithBadList), + exception_type=ValidationError, + msg="Invalid value assigned: int is not a ListConfig, list or tuple.", + key="foo", + ), + id="structured,bad_default_value_for_list", + ), ############## # ListConfig # ##############