Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '
Skip to content

Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix"); } } catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms"); } } catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links"); } } catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); } })(); (function(){ try { var __m = "youtube.com"; var __re = new RegExp('^' + "youtube\\.com" + '
Skip to content

Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default"); } } catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers"); } } catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); } })(); (function(){ try { var __m = "*"; var __re = new RegExp('^' + ".*" + '
Skip to content

Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode"); } } catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); } })(); })();
Skip to content

Give a more informative error for JSON not serializable. (#269) - #273

Merged
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json
Aug 1, 2018
Merged

Give a more informative error for JSON not serializable. (#269)#273
rmarren1 merged 8 commits into
plotly:masterfrom
rmarren1:json

Conversation

@rmarren1

@rmarren1rmarren1 commented Jun 17, 2018

Copy link
Copy Markdown
Contributor

Fix for #269
when a non JSON serializable object is returned from a dash callback, the following exception is raised:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `<property>` from component id `<id>`
returned a value `<value>` of type `<type>`,
which is not JSON serializable.

an example app:

import dash
import dash_html_components as html
from dash.dependencies import Input, Output
app = dash.Dash("")
app.layout = html.Div([
html.Div(id='output'),
html.Button(id='button')
])
@app.callback(Output('output', 'children'),
[Input('button', 'n_clicks')])
def test(n_clicks):
if n_clicks:
return dash
return "HELLO WORLD"
app.run_server()

results in:

dash.exceptions.ReturnValueNotJSONSerializable:
Callback for property `children` from component id `output`
returned a value `<module 'dash' from '/home/ryan/Dash/dash/dash/__init__.py'>` of type `module`,
which is not JSON serializable.

when the button is clicked (and returns the dash module rather than a value).

@chriddyp

Copy link
Copy Markdown
Member

Very nice! One case that I'm interested in is how we can deal with objects that have a non-JSON-serializable object deep inside an object, for example:

html.Div([
html.Div(lambda: 'test')
])

With this current exception, it would say that "Div is not JSON serializable" which could be misleading. In an ideal case, we would be able to say something like:

The property "children" of a component "Div" is a "function" which is not serializable. This Div is located at:
Div.children[0].children
In general, Dash properties can only be dash components, strings, dictionaries, numbers, None, or lists of those.

Or, if the particular parent object was assigned an ID, like

html.Div([
html.Div(lambda: 'test', id='some-div')
])

then our error message could be like:

The property "children" on the element "some-div" is not JSON serializable.

Now, I'm not actually sure the best way to go about doing this. One way might be to subclass the plotly.utils.PlotlyJSONEncoder. That class tries to encode an object a bunch of different ways. If it fails all of the ways, it raises an error:
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L213-L229

It's been a few years since I worked with that class, but I believe that json.dumps traverses the object that you give it and calls cls.default with the nested object
https://github.com/plotly/plotly.py/blob/519e0ab32b2164583f78b51168ce3412c08334e5/plotly/utils.py#L182

So perhaps we could raise a nicer exception within there somehow. I would be OK with our own JSON encoder class that uses that plotly.utils.PlotlyJSONEncoder.

Alternatively, we might be able to traverse the tree ourselves. If the object isn't serializable, then we could walk down the component tree until we find the particular property that isn't serializable. I actually wrote a method that traverses the tree: traverse:

deftraverse(self):

which I believe you can call with something like:

for object in layout:
print(object)

However, I don't think that really gives us the path of the component, it would only give us each individual component in the tree.

Another alternative would be to just raise an exception for the particular property and print the entire component. The user might be able to figure out which component was having the issue if they say both the property that wasn't JSON serializable and if the saw the rest of the properties that weren't serializable. For example, if the issue was something like:

html.Div([
html.Div(style={'color': 'blue'}, children=lambda: 'test')
])

Then the error message would be like:

The property `children` in the following component is not serializable:
Div(children=<function <lambda> at 0x109181840>, style={'color': 'blue'})

I believe this would be relatively easy with the traverse method or even with subclassing the plotly.utils.PlotlyJSONEncoder and raising a dash.exceptions instead of the default TypeError

Oddly, I can't seem to find any other solution out there.

@rmarren1rmarren1 changed the title Give a more informative error for JSON not serializable. (#269)[WIP] Give a more informative error for JSON not serializable. (#269)Jun 19, 2018
@rmarren1

rmarren1 commented Jun 19, 2018

Copy link
Copy Markdown
ContributorAuthor

I think a good solution would be to edit traverse to recursively build paths, like here:

 def traverse_with_paths(self):
"""Yield each item with its path in the tree."""
children = getattr(self, 'children', None)
children_type = type(children).__name__
# children is just a component
if isinstance(children, Component):
yield children_type, children
for p, t in children.traverse_with_paths():
yield " -> ".join([children_type, p]), t
# children is a list of components
elif isinstance(children, collections.MutableSequence):
for idx, i in enumerate(children):
list_path = "{:s} index {:d} (type {:s})".format(
children_type,
idx,
type(i).__name__
)
yield list_path, i
if isinstance(i, Component):
for p, t in i.traverse_with_paths():
yield " -> ".join([list_path, p]), t

then traverse can just be

 def traverse(self):
"""Yield each item in the tree."""
for t in self.traverse_with_paths():
yield t[1]

We can then test if every value in this traversal is one of these types valid = [str, dict, int, float, type(None), Component, dict]

actually, we would need to do something like

def _value_is_valid(val):
return (
any([isinstance(val, x) for x in valid]) or
type(val).__name__ == 'unicode'
)

to account for that python 2 edge case.

This can give very detailed exceptions, which are constructed here: https://github.com/rmarren1/dash/blob/da65b56ebd532ebf9081424467bf100d3a537f44/dash/dash.py#L481

One example I tried was returning this from a callback:

 html.Div(["Hello", "world", html.Span(["hi", html.Div(["hello", html.P([lambda: 'hi'])])])])

which raises this

dash.exceptions.ReturnValueNotJSONSerializable: The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
`Div -> list index 2 (type Span) -> list index 1 (type Div) -> list index 1 (type P) -> list index 0 (type function)`
and has string representation
`<function test.<locals>.<lambda> at 0x7f9b0d64c9d8>`.
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

Also tox was passing locally, so I'll check into what the issue is with CircleCI. I'm going to add more test cases to make sure there arent valid values this is raising on.

@chriddyp

Copy link
Copy Markdown
Member

😻 that looks SO good @rmarren1 ! This seems like the winning solution.

@plotly/dash - Anyone else like to review?

@nicolaskruchten

nicolaskruchten commented Jun 21, 2018

Copy link
Copy Markdown
Contributor

Love this! I would recommend putting each path element on its own line rather than separating them with -> as those lines will get looong :)

We could also structure the lines a bit differently like:

The value in question is located at
Div
[2]: Span
[1]: Div
[1]: P
[0]: function

or something? Maybe add in the id of those elements if present?

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

It turns out that traverse doesn't yield leaves of the tree which are not of type Component or collections.MutableSequence, which complicates things a bit since an exception will not be thrown for return values like

html.Div(
html.Div(lambda: 'hi')
)

since the lambda is not wrapped in a list. (Is this the expected behavior of traverse? When I change it to return such elements, test cases fail.)

My quick patch was to check each value in the traversal, and if is a Component and has a child not of type collections.MutableSequence we validate the child.

Also, I changed the output format (thanks @nicolaskruchten). Outputs now read

The callback for property `children` of component `output`
returned a tree with one value having type `function`
which is not JSON serializable.
The value in question is located at
- Div (id=top-div)
- Div (id=list-div)
[2] Span - function
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fc67896d6a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

I also added special cases for when the bad value is the only value returned or is a list (previously, the error message would talk about trees and print out a tree where there is only one element, this could be confusing). These error messages look like this now:

The callback for property `children` of component `output`
returned a value having type `function`
which is not JSON serializable.
The value in question is either the only value returned,
or is in the top level of the returned list,
and has string representation
`<function change_button.<locals>.<lambda> at 0x7fb2f3ca66a8>`
In general, Dash properties can only be
dash components, strings, dictionaries, numbers, None,
or lists of those.

@rmarren1rmarren1 changed the title [WIP] Give a more informative error for JSON not serializable. (#269)Give a more informative error for JSON not serializable. (#269)Jun 25, 2018
@nicolaskruchten

Copy link
Copy Markdown
Contributor

Nice! I would favour the [index] for every element in the list, even when the id is present, so that you could mechanically just walk to the right place in the tree if you wanted do :)

@rmarren1

rmarren1 commented Jun 26, 2018

Copy link
Copy Markdown
ContributorAuthor

I think that was just a coincidence in the example I used, which was something like this:

html.Div(
id='top-div',
children=html.Div(
id='list-div',
children=['hi', 'hello', Span(lambda: 'hi')]
)
)

If the id is present it should be printed no matter if the component is a single child or a member of a list: https://github.com/plotly/dash/pull/273/files#diff-228c9aefac729977642672aa1416bacaR511.

When an element is a singleton child the prefix is '- ' so that it aligns with the list elements that have prefix '[i] '

I will double check this

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Right, so what I'm saying is that even if elements are singletons, I think we should show [0] rather than -.

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

How could we then differentiate between a singleton element and the first element in a list?

c = html.Div(html.Div(lambda: 'hi'))

would need to be accessed like this:

c.children.children

and

html.Div([html.Div([lambda: 'hi', "valid string"])])

would need to be accessed like this:

c.children[0].children[0]

Showing - instead of [0] could tip off a crawler to use .children rather than .children[0].

I think that would make sense if Dash automatically converted singleton elements into length one lists (which might make coding a bunch of the internals easier).

@nicolaskruchten

Copy link
Copy Markdown
Contributor

Ah, I see what the problem is. In that case I might suggest [*] just for nicer visual effect scanning down :)

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Error paths now read like this:

[*] Div (id=top-div)
[*] Div (id=list-div)
[2] Span [*] function

looks better imo, thanks @nicolaskruchten

@rmarren1

Copy link
Copy Markdown
ContributorAuthor

Is this good to merge?

Comment threaddash/dash.py
output.component_property).replace(' ', ''))

def _validate_callback_output(self, output_value, output):
valid = [str, dict, int, float, type(None), Component]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since we JSON serialize with the json.dumps(obj, cls=plotly.utils.PlotlyJSONEncoder), this list actually has a few other items: https://github.com/plotly/plotly.py/blob/6b3a0135977b92b3f7e0be2a1e8b418d995c70e3/plotly/utils.py#L137-L332

So, if there was a component property that accepted a list of numbers, then technically the user could return a numpy array or a dataframe.

The only example that immediately comes to mind is updating the figure property in a callback with the plotly.graph_objs. These objects get serialized because they have a to_plotly_json method.

So, I wonder if instead of validating up-front, we should only run this routine only if our json.dumps(resp, plotly.utils.PlotlyJSONEncoder) call fails.

This would have the other advantage of being faster for the non-error case. If the user returns a huge object (e.g. some of my callbacks in apps return a 2MB nested component), doing this recursive validation might be slow.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

That makes sense. I am not sure how large of a speed up that would be since validating a large callback output is likely much faster than the network delay of sending that large output, but it definitely makes it easier than crafting a perfect validator (that would need to update every time we want to extend PlotlyJSONEncoder).

@chriddyp

Copy link
Copy Markdown
Member

I have just one more comment about the placement of the validation call. Otherwise, I'm 👍 with the code.

It would be great if another dash dev could review this so that more people are familiar with this code. @T4rk1n , could you take a look as well?

@T4rk1nT4rk1n left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall.

Comment threaddash/exceptions.py Outdated
pass


class ReturnValueNotJSONSerializable(CallbackException):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe just me, but I don't like that exception name, it gives too much info on the cause while not giving the true nature of the error, that is the return value of a callback is invalid. I would change it to InvalidCallbackReturnValue.

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense to me, pushed those changes.

Comment threaddash/dash.py
response,
cls=plotly.utils.PlotlyJSONEncoder
)
except TypeError:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 perfect

@chriddyp

Copy link
Copy Markdown
Member

💃 Looks good, let's do this!

@rmarren1
rmarren1 merged commit 30353a9 into plotly:masterAug 1, 2018
rmarren1 added a commit that referenced this pull request Aug 1, 2018
Sign up for freeto 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.

4 participants

@rmarren1@chriddyp@nicolaskruchten@T4rk1n