Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve error message when initializing from a Structured Config with incorrect type #559

Merged
merged 7 commits into from
Mar 5, 2021
Merged

Improve error message when initializing from a Structured Config with incorrect type #559

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2610a53
rebased against master
Jasha10 Feb 26, 2021
cb24275
test error message for deeper nesting
Jasha10 Feb 26, 2021
a45e314
revert/remove some parent_node fields
Jasha10 Feb 25, 2021
f2a1bfd
use format_and_raise() directly
Jasha10 Mar 3, 2021
703b6df
revert dummy_parent obj_type
Jasha10 Mar 5, 2021
35731c1
clean leftovers from previous attempts
Jasha10 Mar 5, 2021
550861c
add 435.bugfix news fragment
Jasha10 Mar 5, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 28 additions & 16 deletions omegaconf/_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
ConfigTypeError,
ConfigValueError,
OmegaConfBaseException,
ValidationError,
)
from .grammar_parser import parse

Expand Down Expand Up @@ -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
omry marked this conversation as resolved.
Show resolved Hide resolved
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__)
Expand All @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you try to use format_and_raise() directly instead of through the dummy parent?
it can probably make things a bit cleaner when testing this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In f2a1bfd I've switched from dummy_parent._format_and_raise(...) to format_and_raise(node=None, ...).

The disadvantage of this change is that we no longer have object_type information in the error message (as dummy_parent was providing the object_type).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides the object_type issue, the error messages are also not reporting full_key accurately. I think that reporting on full_key would be easiest if we pass a parent_node into the get_structured_config_data function (so that full_key could be computed using the parent config). This change would require some plumbing of the methods that call get_structured_config_data. I can give it a try if you're interested...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this will trivially easier once we have the structured_ir diff baked and ready.
for now this is good enough.

d[name]._set_parent(None)
return d

Expand All @@ -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])
Expand All @@ -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

Expand Down
7 changes: 7 additions & 0 deletions tests/structured_conf/test_structured_basic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
37 changes: 26 additions & 11 deletions tests/test_errors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
omry marked this conversation as resolved.
Show resolved Hide resolved
),
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),
omry marked this conversation as resolved.
Show resolved Hide resolved
exception_type=ValidationError,
object_type=None,
key=None,
Expand All @@ -569,7 +570,7 @@ 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,
Expand All @@ -580,7 +581,21 @@ def finalize(self, cfg: Any) -> None:
pytest.param(
Expected(
create=lambda: None,
op=lambda cfg: OmegaConf.structured(IllegalType()),
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,
op=lambda _: OmegaConf.structured(IllegalType()),
exception_type=ValidationError,
msg="Object of unsupported type: 'IllegalType'",
object_type_str=None,
Expand All @@ -591,7 +606,7 @@ def finalize(self, cfg: Any) -> None:
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,
Expand Down