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.

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

IntegerNode/FloatNode ValidationError message: include value type #744

Merged
merged 7 commits into from
Jun 7, 2021

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Jun 7, 2021

Closes #743

The ValidationError raised by IntegerNode and FloatNode now contains the type of the value that triggered the error.

Copy link
Owner

@omry omry left a comment

Choose a reason for hiding this comment

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

looks good, one nit.

Comment on lines 102 to 106
@dataclass
class TypedFields:
bar: float = 123.456


Copy link
Owner

Choose a reason for hiding this comment

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

As an alternative to creating a type here, you can use:

DictConfig({"bar": FloatNode(123.456)})

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, I made the change in f1b441d.

@Jasha10 Jasha10 marked this pull request as ready for review June 7, 2021 19:25
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Jun 7, 2021

Currently we have this error message for numpy floats:

>>> cfg.bar = np.float32(0.9)
Traceback (most recent call last):
...
omegaconf.errors.ValidationError: Value '0.9' of type 'float32' could not be converted to Float

I've been experimenting with a diff to _utils.format_and_raise that includes the module name in the error message:

>>> cfg.bar = np.float32(0.9)
Traceback (most recent call last):
...
omegaconf.errors.ValidationError: Value '0.9' of type 'numpy.float32' could not be converted to Float

The diff looks like this:

diff --git a/omegaconf/_utils.py b/omegaconf/_utils.py
index d3dafe9..72bcc07 100644
--- a/omegaconf/_utils.py
+++ b/omegaconf/_utils.py
@@ -768,13 +768,18 @@ def format_and_raise(
         ref_type = get_ref_type(node)
         ref_type_str = type_str(ref_type)

+    value_type = type(value)
+    if hasattr(value_type, "__module__") and value_type.__module__ != "builtins":
+        value_type_str = value_type.__module__ + "." + value_type.__name__
+    else:
+        value_type_str = value_type.__name__
     msg = string.Template(msg).safe_substitute(
         REF_TYPE=ref_type_str,
         OBJECT_TYPE=object_type_str,
         KEY=key,
         FULL_KEY=full_key,
         VALUE=value,
-        VALUE_TYPE=f"{type(value).__name__}",
+        VALUE_TYPE=value_type_str,
         KEY_TYPE=f"{type(key).__name__}",
     )

@omry, what do you think?

@omry
Copy link
Owner

omry commented Jun 7, 2021

Since 2.1.0 is already out, please add a news fragment. otherwise looks good.

@omry
Copy link
Owner

omry commented Jun 7, 2021

@omry, what do you think?

lgtm, extract it to a function in _utils.
_fully_qualified_type_name?

Alternatively, and probably better - is to add support for fully-qualified-name to _utils.type_str.
It can handle compound types and annotations as well.

Either way works.

@Jasha10 Jasha10 requested a review from omry June 7, 2021 21:18
news/743.api_change Outdated Show resolved Hide resolved
Co-authored-by: Omry Yadan <omry@fb.com>
@Jasha10 Jasha10 merged commit 2b9a208 into omry:master Jun 7, 2021
@Jasha10 Jasha10 deleted the closes743 branch June 7, 2021 23:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FloatNode/IntegerNode ValidationError message: include value type
2 participants