New issue

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

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

Already on GitHub? Sign in to your account

Validate component properties #264 - March 1 #452

Open
wants to merge 41 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@rmarren1
Copy link
Member

rmarren1 commented Nov 8, 2018

Note: This PR is a continuation of #340. That PR had ~80 comments (most were outdated and no longer relevant), and 96 commits. This PR is the same code, just more readable.


PR for #264

Current Prerelease

pip install dash==0.35.0rc1
pip install dash-renderer==0.15.0rc1
pip install dash-core-components==0.42.0rc1
pip install dash-html-components==0.14.0rc6

Validation is on by default. To turn it off, you can set app.config.disable_component_validation = True

Test Cases to run for demo

import dash
import dash_html_components as html
import dash_core_components as dcc
import plotly.graph_objs as go
from dash.dependencies import Input, Output

app = dash.Dash()
app.scripts.config.serve_locally=True

app.layout = html.Div([
    html.Button(id='click1', children='click to return bad Div children'),
    html.Div(id='output1', **{'data-cb': 'foo'}),
    html.Button(id='click2', children='click to return a bad figure'),
    dcc.Graph(id='output2', figure={'data': [], 'layout': {}}),
    html.Button(id='click3', children='click to return a bad radio'),
    dcc.RadioItems(id='output3', options=[{'value': 'okay', 'label': 'okay'}]),
    html.Button(id='click4', children='click to make a figure with no id'),
    html.Div(id='output4'),
])



@app.callback(Output('output1', 'children'),
              [Input('click1', 'n_clicks')])
def crash_it1(clicks):
    if clicks:
        return [[]]
    return clicks

@app.callback(Output('output2', 'figure'),
              [Input('click2', 'n_clicks')])
def crash_it2(clicks):
    if clicks:
        return {'data': {'x': [1, 2, 3], 'y': [1, 2, 3], 'type': 'scatter'}, 'layout': {}}
    return go.Figure(data=[go.Scatter(x=[1,2,3], y=[1,2,3])], layout=go.Layout()) 

@app.callback(Output('output3', 'options'),
              [Input('click3', 'n_clicks')])
def crash_it3(clicks):
    if clicks:
        return [{'value': {'not okay': True}, 'labl': 'not okay'}]
    return [{'value': 'okay', 'label': 'okay'}]

@app.callback(Output('output4', 'children'),
              [Input('click4', 'n_clicks')])
def crash_it4(clicks):
    if clicks:
        return dcc.Graph()
    return dcc.Graph(id='hi')

app.run_server(debug=True, port=8050)

Example Error Messages

CallbackOutputValidationError: 


A Dash Callback produced an invalid value!

Dash tried to update the `figure` prop of the
`Graph` with id `output2` by calling the
`crash_it2` function with `(1)` as arguments.

This function call returned `{'layout': {}, 'data': {'y': [1, 2, 3], 'x': [1, 2, 3], 'type': 'scatter'}}`, which did not pass
validation tests for the `Graph` component.

The expected schema for the `figure` prop of the
`Graph` component is:

***************************************************************
{'validator': 'plotly_figure'}
***************************************************************

The errors in validation are as follows:

* figure	<- Invalid Plotly Figure:


    Invalid value of type '__builtin__.dict' received for the 'data' property of 
        Received value: {'y': [1, 2, 3], 'x': [1, 2, 3], 'type': 'scatter'}

    The 'data' property is a tuple of trace instances
    that may be specified as:
      - A list or tuple of trace instances
        (e.g. [Scatter(...), Bar(...)])
      - A list or tuple of dicts of string/value properties where:
        - The 'type' property specifies the trace type
            One of: ['mesh3d', 'splom', 'scattercarpet',
                     'scattergl', 'scatterternary', 'pie',
                     'surface', 'histogram', 'ohlc', 'heatmapgl',
                     'cone', 'scatterpolar', 'table',
                     'scatterpolargl', 'histogram2d', 'contour',
                     'carpet', 'box', 'violin', 'bar',
                     'contourcarpet', 'area', 'choropleth',
                     'candlestick', 'streamtube', 'parcoords',
                     'heatmap', 'barpolar', 'scattermapbox',
                     'scatter3d', 'pointcloud',
                     'histogram2dcontour', 'scatter', 'scattergeo',
                     'sankey']

        - All remaining properties are passed to the constructor of
          the specified trace type

        (e.g. [{'type': 'scatter', ...}, {'type': 'bar, ...}])

CallbackOutputValidationError: 


A Dash Callback produced an invalid value!

Dash tried to update the `options` prop of the
`RadioItems` with id `output3` by calling the
`crash_it3` function with `(1)` as arguments.

This function call returned `[{'value': {'not okay': True}, 'labl': 'not okay'}]`, which did not pass
validation tests for the `RadioItems` component.

The expected schema for the `options` prop of the
`RadioItems` component is:

***************************************************************
{'schema': {'allow_unknown': False,
            'nullable': False,
            'schema': {'disabled': {'type': 'boolean'},
                       'label': {'type': 'string'},
                       'value': {'type': 'string'}},
            'type': 'dict'},
 'type': 'list'}
***************************************************************

The errors in validation are as follows:

* options
 * 0
  * value	<- must be of string type
  * labl	<- unknown field

ComponentInitializationValidationError: 


A Dash Component was initialized with invalid properties!

Dash tried to create a `RadioItems` component with the
following arguments, which caused a validation failure:

***************************************************************
{'id': 'output3', 'options': [{'label': 'okay', 'value': {}}]}
***************************************************************

The expected schema for the `RadioItems` component is:

***************************************************************
{'className': {'type': 'string'},
 'dashEvents': {'allowed': ['change'], 'type': ('string', 'number')},
 'fireEvent': {},
 'id': {'type': 'string'},
 'inputClassName': {'type': 'string'},
 'inputStyle': {'type': 'dict'},
 'labelClassName': {'type': 'string'},
 'labelStyle': {'type': 'dict'},
 'options': {'schema': {'allow_unknown': False,
                        'nullable': False,
                        'schema': {'disabled': {'type': 'boolean'},
                                   'label': {'type': 'string'},
                                   'value': {'type': 'string'}},
                        'type': 'dict'},
             'type': 'list'},
 'setProps': {},
 'style': {'type': 'dict'},
 'value': {'type': 'string'}}
***************************************************************

The errors in validation are as follows:


* options
 * 0
  * value	<- must be of string type

PropTypes to Cerberus reference

PropType Cerberus Schema Validated Against Known Current Limitations
array {'type': 'list'}
bool {'type': 'boolean'}
func {} No validation occurs.
object {'type': 'dict'} Validates that input is explicitly a dict object. We cannot be more general (e.g. collections.abc.Mapping) since the core Component is an instance of that.
string {'type': 'string'}
symbol {} No validation occurs
node {'anyof': [{'type': 'component'}, {'type': 'boolean'}, {'type': 'number'}, {'type': 'string'}, {'type': 'list', 'schema': {'anyof': [{'type': 'component'}, {'type': 'boolean'}, {'type': 'number'}, {'type': 'string'}]}}]}
instanceOf(Object) {} No validation occurs
oneOf(['val1', 2]) {'allowed': [None, 'val1', 2]} Strings will have ' characters stripped off each end. This is because metadata.json generation serializes the literal values as json, so for example PropTypes.oneOf(['News', 'Photos']) serializes to ["'News'", "'Photos'"]. reactjs/react-docgen#57
oneOfType( [ PropTypes.string, PropTypes.bool ] ) {'anyof': [{'type': 'string', 'type': 'boolean'}]} If one of the types is a PropType that cannot be validated individually (e.g. PropType.func), no validation will occur and the schema will effectively be {}
arrayOf( PropTypes.number ) {'type': 'list', 'schema': {'type': 'number'}}
objectOf( PropTypes.number ) {'type': 'dict', 'valueschema': {'type': 'number'}}
shape( { k1: PropTypes.string, k2: PropTypes.number } ) {'type': 'dict', 'allow_unknown': False, 'schema': {'k1': {'type': 'string}, 'k2': {'type': 'number'}}}
any {'type': ('boolean', 'number', 'string', 'dict', 'list')}

@rmarren1 rmarren1 changed the title ignore this PR Validate component properties #264 Nov 8, 2018

@rmarren1

This comment has been minimized.

Copy link
Member

rmarren1 commented Nov 8, 2018

@T4rk1n I made the commit history more readable, hopefully this is easier to review. This includes fixes to address the review you made in #340

@T4rk1n

This comment has been minimized.

Copy link
Contributor

T4rk1n commented Nov 8, 2018

Thanks, I'll have a good look on friday review party.

@@ -884,6 +894,7 @@ def add_context(*args, **kwargs):
mimetype='application/json'
)

self.callback_map[callback_id]['func'] = func

This comment has been minimized.

@T4rk1n

T4rk1n Nov 9, 2018

Contributor

Why do that in two difference part ? Couldn't your additions in dispatch be in add_context ?

This comment has been minimized.

@rmarren1

rmarren1 Nov 11, 2018

Member

We would have to pass namespace, component_id, component_property, to this since that info is available in dispatch. I originally thought this might clash with the other arguments (e.g. if you had a property called namespace) although I now think this would be fine, but it might make it difficult if we want to support passing props with keyword arguments in the future.

rmarren1 added some commits Nov 17, 2018

@rmarren1

This comment has been minimized.

Copy link
Member

rmarren1 commented Nov 18, 2018

In response to those other alternatives...

  1. I considered this but the main drawback was, as you mention, that it does not validate validate at runtime. For example, if you are creating components dynamically and setting their props via a variable, I do not think this solution would get you very far.
  2. (2-3) Honestly did not know these solutions existed. I think that both would have a similar code footprint, since a majority of the code in this PR is just converting a component's metadata.json to something Python understands, which would need to be done for both of these as well.
  3. This actually looks very similar to Cerberus, the package used in this PR. Not sure if one is 'better', but they seem to have the same set of features.
@T4rk1n

This comment has been minimized.

Copy link
Contributor

T4rk1n commented Nov 19, 2018

I couldn't generate any validation errors when I tried, the schema of the component was always {}, do I need a version of dcc and html components ?

Edit: guess I missed the current pre releases, I installed from the dev-requirements.txt so maybe lock them for this PR, don't think the tests are running the right versions.

@T4rk1n

This comment has been minimized.

Copy link
Contributor

T4rk1n commented Nov 19, 2018

This did not validate with a validation error:

app.layout = html.Div({'hello'}) -> TypeError: set(['hello']) is not JSON serializable

@T4rk1n

This comment has been minimized.

Copy link
Contributor

T4rk1n commented Nov 19, 2018

Same for:

class MyOtherType:
    def __init__(self):
        pass


app.layout = html.Div([
    html.Div(MyOtherType())
])

Got AttributeError: MyOtherType instance has no attribute '__trunc__'

And:
app.layout = html.Div(MyOtherType()) -> TypeError: <__main__.MyOtherType instance at 0x00000000189B5708> is not JSON serializable

app.layout = html.Div(id={'hello': 'world'}) -> TypeError: unhashable type: 'dict'

This validated true:

app.layout = html.Div(className={'hello': 'world'})

@rmarren1 rmarren1 force-pushed the rmarren1:validate2 branch from edb26fc to d842651 Nov 25, 2018

@rmarren1

This comment has been minimized.

Copy link
Member

rmarren1 commented Nov 25, 2018

  • - Fix app.layout = html.Div({'hello'}) -> TypeError: set(['hello']) is not JSON serializable
    Had to change the list validation logic to exclude set, it was being included since it is castable to a list but erroring since it is not serializable as a list
  • - AttributeError: MyOtherType instance has no attribute '__trunc__'
    Had to adjust number validation logic, normally things throw a ValueError if they are not castable to an int, but this was an exception

For the dash-html-components I am currently only validating children since all dash-html-components proptypes are strings, as discussed in plotly/dash-html-components#71. I think this should be re-visited. I do not think we should manually type each component since there are too many props, but I think it would be reasonable for now to ensure that all non-children props are scalars (e.g. str, int, bool, or None).

@rmarren1

This comment has been minimized.

Copy link
Member

rmarren1 commented Nov 30, 2018

Updates:

  • Discovered and fixed an issue where required falsy values were not detected (e.g. on dcc.Checklist, if values=[] it would not properly detect it and say values is required during validation).
  • Removed old method of required component validation (e.g. a check in the __init__ function of a component). This is now handled by validation done in this PR.
  • [ X] Discovered a small issue in figure validation. The plan was to run a figure value for go.Figure for validation, but there is one issue in dash-docs where go.Figure will error when a proper plot would have been produced. (plotly/dash-core-components#257 (comment)) Edit: looks like this was actually a bug in plotly.py, so we should be fine.

Bumped all release candidates.

@rmarren1

This comment has been minimized.

Copy link
Member

rmarren1 commented Dec 4, 2018

Tested the RCs with dash-docs, and a few Dash apps I have built.
@plotly/dash if you have a large dash app and some time, please try to run it with these RCs and let me know how it goes!

@rmarren1 rmarren1 referenced this pull request Dec 11, 2018

Open

Components with schema #289

rmarren1 added some commits Dec 14, 2018

@rmarren1

This comment has been minimized.

Copy link
Member

rmarren1 commented Dec 18, 2018

  • rebased dash, dash-html-components and dash-core-components
  • bumped versions in top level comment
  • sanity check on dash-docs: plotly/dash-docs#314
    @T4rk1n please take another look, hopefully we are getting close!
@T4rk1n

This comment has been minimized.

Copy link
Contributor

T4rk1n commented Dec 28, 2018

Testing the demo with the rc version I got a figure validation error and the message:

You can turn off these validation exceptions by setting
`app.config.suppress_validation_exceptions=True`

Validation is on by default. To turn it off, you can set app.config.disable_component_validation = True

Is it app.config.disable_component_validation or app.config.suppress_validation_exceptions ?

@T4rk1n

This comment has been minimized.

Copy link
Contributor

T4rk1n commented Dec 28, 2018

Dash tried to update the `figure` prop of the
`Graph` with id `output2` by calling the
`crash_it2` function with `(None)` as arguments.

This function call returned `Figure({
    'data': [{'type': 'scatter', 'uid': '91f6c03c-17bb-42a3-a502-4d9441696413', 'x': [1, 2, 3], 'y': [1, 2, 3]}],
    'layout': {}
})`, which did not pass
validation tests for the `Graph` component.

The expected schema for the `figure` prop of the
`Graph` component is:

***************************************************************
{'nullable': False, 'required': False, 'type': 'dict'}
***************************************************************

The errors in validation are as follows:

* figure	<- must be of dict type

You can turn off these validation exceptions by setting
`app.config.suppress_validation_exceptions=True`

Shouldn't Figure be valid ?

@T4rk1n

This comment has been minimized.

Copy link
Contributor

T4rk1n commented Dec 28, 2018

html.Button(id='click4', children='click to make a figure with no id'),

I think the graph now put a random id if there's none.

@chriddyp chriddyp changed the title Validate component properties #264 Validate component properties #264 - March 1 Jan 10, 2019

@chriddyp chriddyp added the Sponsored label Jan 10, 2019

@chubukov

This comment has been minimized.

Copy link

chubukov commented Jan 11, 2019

Wanted to note that the issue I raised in the other thread still exists. Numerical types in checklist values and dropdown values (and probably other components too) still raise errors with the latest release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment