Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

1193 multipage validation #1201

Merged
merged 6 commits into from
Apr 22, 2020
Merged

1193 multipage validation #1201

merged 6 commits into from
Apr 22, 2020

Conversation

alexcjohnson
Copy link
Collaborator

@alexcjohnson alexcjohnson commented Apr 17, 2020

Fixes #1193 - the flask.has_request_context trick inside an app.layout function can be used again to supply a special layout just for validation purposes.

In addition, added a new attribute app.validation_layout that does the same thing more explicitly.

Contributor Checklist

  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR
  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follow

@@ -730,11 +730,7 @@ def multipage_app(validation=False):
layout_page_2 = html.Div(
[
html.H2("Page 2"),
dcc.Dropdown(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

dcc.Dropdown is giving React deprecated lifecycle method warnings ATM... I thought we had already updated all of these in the 16.13 push? Anyway switched to Input to get my assert not dash_duo.get_logs() clean console test to pass.

dash/dash.py Outdated
_validate.validate_layout(value, layout_value)
self.validation_layout = simple_clone(
# pylint: disable=protected-access
layout_value, [simple_clone(c) for c in layout_value._traverse_ids()]
Copy link
Collaborator Author

@alexcjohnson alexcjohnson Apr 17, 2020

Choose a reason for hiding this comment

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

Make this validation layout as simple as possible, to minimize its overhead on the page:

  • strip out components with no ID
  • strip out most prop values, at least in Py3 where we can fully inspect the component constructor signature so we know what props are required.

This step is only run once when you set the layout, and even then only when you set it to a function AND you haven't already set app.validation_layout

Copy link
Member

Choose a reason for hiding this comment

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

If we're just using this to validate IDs w.r.t. callbacks, why do we need the required values?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We also validate props against the component type, so we can't just create dummy components, they have to be the same type as the real components. And unless I were to recreate the JSON serialization for components, the easiest way to do that is to actually instantiate the same component class. But doing so requires that we include required props, because the generated components enforce the required props during construction.

Copy link
Member

Choose a reason for hiding this comment

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

Ohhhhhh by "props against the component type" do you mean during callback validation with messages like e.g. "You registered a callback to update a Div's childrenz property. However, Div does not have a "children" property."?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

e.g. "You registered a callback to update a Div's childrenz property. However, Div does not have a "childrenz" property."?

exactly.

const validateIds = !config.suppress_callback_exceptions;
let layout, paths;
if (validateIds && config.validation_layout) {
layout = config.validation_layout;
Copy link
Member

Choose a reason for hiding this comment

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

Is this because validation_layout is pruned down, so faster to crawl?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just saying if we have a separate validation layout, that's the one we need to check the callbacks against. The fact that it's pruned should make computePaths (next line) faster, but the main reason for pruning was to reduce the size of _dash-config on the index page - I was imagining it could become enormous, if you have multiple tables and graphs on different pages all concatenated together.

dash/dash.py Outdated
self._layout = value

# for using flask.has_request_context() to deliver a full layout for
# validation inside a layout function - track if a user might be doing this.
if self._layout_is_function and not self.validation_layout:
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if this is something we should turn off if debug=False too, or introduce a new dev_tools_validate_layout function. Just to reduce the performance hit if there are many small components (e.g. a dash_html_components.Table)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point. Those mostly wouldn't have IDs, so wouldn't be sent to the front end anyway, but with debug off we don't need it at all. In fact I guess we should be able to short-circuit all the front-end validation in that case too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added clauses to skip this when suppress_callback_validation is used, but didn't remove it entirely when debug=False. There's more to think about here - on the one hand, without this validation the app behavior can change, and on the other there's a lot more validation we could be skipping if that's what we want to do. I've created #1206 to follow up.

Copy link
Member

@chriddyp chriddyp left a comment

Choose a reason for hiding this comment

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

👯 once debug is considered

@alexcjohnson alexcjohnson merged commit d7d71ac into dev Apr 22, 2020
@alexcjohnson alexcjohnson deleted the 1193-multipage-validation branch April 22, 2020 02:53
@eddy-geek
Copy link
Contributor

I confirm the fix works on my app (tested with dev branch, after remembering to rebuild dash_renderer ;-)). Thanks for the responsiveness!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] dash 1.11 breaks Dynamic Multipage App Validation
3 participants