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

Immediate callback validation makes combining layout and callbacks in same module difficult #519

Closed
ned2 opened this issue Dec 30, 2018 · 12 comments

Comments

@ned2
Copy link
Contributor

ned2 commented Dec 30, 2018

Callback validation is great, especially for larger apps with more callbacks. In order to apply callback validation for multi-page apps, the trick described in the Dash Docs under Dynamically Create a Layout for Multi-Page App Validation, needs to be used. This involves creating a layout containing the concatenated layouts of all pages, which is only served if not within a request context (ie for for collback validation, but not for handling a web request). Doing this however makes it tricky to put layouts and callbacks of different pages within the same module.

Consider the following way of defining a layout function for multi-page apps that doesn't require setting app.config.suppress_callback_exceptions = True:

from .pages import page1, page2, page3

pages = [page1, page2, page3]

def layout_with_validation():
    this_layout = my_layout_func
    if flask.has_request_context():
         return my_layout_func
    return [my_layout_func] + [page.layout for page in pages]

If the callbacks for each page are defined along with the layouts within each page module (as they are done in the example multi-page app layout in the Dash Docs), then this will raise a dash.exceptions.LayoutIsNotDefined exception as the callback method calls _validate_callback immediately, before the app.layout attribute has been assigned. This means layout attributes and direct uses of app.callback() can't be combined within the same module while supporting callback validation.

One workaround would be to defer evaluation of the callbacks by wrapping all callbacks in a function that then must be manually called after. This is a bit cumbersome, and makes swapping these pages between multi-page Dash apps and single page ones harder. Are there any more sensible workarounds that this?

Another possibility is changing Dash to defer calling _validate_callback on each callback until app load, such as in the enable_dev_tools method.

@chriddyp
Copy link
Member

chriddyp commented Jan 7, 2019

Another possibility is changing Dash to defer calling _validate_callback on each callback until app load, such as in the enable_dev_tools method.

This seems like a decent way to go.

From that chapter in the user guide:

def serve_layout():
    if flask.has_request_context():
        return url_bar_and_content_div
    return html.Div([
        url_bar_and_content_div,
        layout_index,
        layout_page_1,
        layout_page_2,
    ])

This feels too complicated to me (checking for request context)

@ned2
Copy link
Contributor Author

ned2 commented Jan 8, 2019

This seems like a decent way to go.

I'd be up for putting together a PR for this. I think this is an important part of providing a good development experience for building making Dash apps.

This feels too complicated to me (checking for request context)

I agree; it would be good if we could make callback validation available without having to jump through so many hoops. I tried to improve this situation for my own projects by creating a decorator that applies this logic to a plain layout function, so you would only have to do this:

@validate_layout
def main_layout_sidebar():
    return html.Div(["home page text", dcc.Location(id="url", refresh=False)])

This however requires baking in the concatenated layouts from all the multi-pages into the decorator, making it only applicable to the one project. In this case, I'm pulling out all the layouts associated with the URLs from the router callback.

def validate_layout(func):
    """Decorator that allows callback validation for dynamic layout functions"""
    @wraps(func)
    def function_wrapper(*args, **kwargs):
        requested_layout = func(*args, **kwargs)
        if has_request_context():
            # This is a normal page request, return the layout                                                                                                                                 
            return requested_layout
        # Not handling a request, return a layout with every page                                                                                                                              
        # layout concatenated together for validation                                                                                                                                          
        all_layouts = [requested_layout]
        return all_layouts + [url[1] for url in server.config['URLS']]
    return function_wrapper

We could generalise this by making the sequence of layouts a parameter to the decorator:

@validate_layout([page1_layout, page2_layout, page3_layout])
def main_layout_sidebar():
    return html.Div(["home page text", dcc.Location(id="url", refresh=False)])

But this is still awkward, as developers have to manually keep track all the page layouts in order to validate them. I think Dash should help people do this rather than require them to manage it themselves.

This makes me wonder (more broadly than this issue), maybe Dash could provide a utility for registering and managing page layouts and their corresponding URLs? Something along the lines of:

app.router(container_id, urls_to_page_mappings)

Where urls_to_page_mappings is either dictionary or list of url patterns, Django style. With the layouts registered with Dash, they could then be automatically validated without the developer having to do it themselves.

Keen to get thoughts from more @plotly/dash folk.

@ned2
Copy link
Contributor Author

ned2 commented Feb 24, 2019

I'm thinking about how to move callback validation to be done in the enable_dev_tools method. The main challenge is that _validate_callback requires the actual component dependency objects (ie the output, inputs, and states args) however these are not kept, with just the relevant data being extracted and stored in the callback_map dict.

What about adding the original dependency objects into the callback map dictionary, so each dependency would get an 'obj' key along with 'id' and 'property'? We'd also have to add an 'output' dependency key as well, since currently only inputs and states are stored in the callback map.

Thoughts @T4rk1n and @alexcjohnson?

@T4rk1n
Copy link
Contributor

T4rk1n commented Feb 25, 2019

I agree that callback validation could be moved to enable_dev_tools so callbacks can be defined before the layout. I refactored the dependencies objects, they can now be used as dict keys.

@alexcjohnson
Copy link
Collaborator

Right now we do most of the validation at callback definition, but there's a little bit - specifically circular dependency detection - in the renderer. I wonder if it wouldn't be better to move the layout-dependent pieces of validation to the renderer as well? A few reasons for this:

  • We don't need to worry about how the pieces get served (run_server vs WSGI) - on the front end there's only one system
  • In principle in fact it needs to happen there, or if we keep it on the back end it needs to happen every time the layout is served. If app.layout is a function, the validation results could be different each time.
  • Less duplication as we add more back ends (https://github.com/plotly/dashR)
  • We're working toward displaying the errors client-side anyway (Dash Dev Tools dash-renderer#100)

Anyway this only necessarily applies to the layout-dependent parts. I believe we can still make a "clean" callback map during @app.callback calls, where we ensure that each callback is self-consistent (no duplication between inputs & outputs or among outputs) and independent of other callbacks (no output duplication across callbacks). The only part we need to defer is matching layout items to callback inputs/outputs.

@ned2
Copy link
Contributor Author

ned2 commented Mar 12, 2019

Client-side validation for the layout-dependent parts of callback validation hadn't occurred to me. You make some good points in favour of this approach.

One question I have is how we would go about validating multi-page apps on the client side given that the complete layout won't be sent to the client initially?

My motivation for pushing delayed callback validation was to be able to stop suppressing callback validation for multi-page apps. Currently, even with delayed callback validation enabled on the server-side, this still requires creating a dummy layout (when not serving a request) with all layout fragments of each page concatenated together. I agree with @chriddyp that this is not a great interface. So would be good to improve on this whichever direction is taken.

This brings me back to my thoughts from above, that Dash might benefit from an explicit URL router as part of it's API, where different page layouts and their endpoints are registered with the Dash instance. This could allow the complete concatenated layout to be sent down to the client for validation, but only in debug mode to avoid slowing things down in production.

@sjtrny
Copy link

sjtrny commented Apr 20, 2019

@ned2 I agree that Dash needs at least a simple URL router/dispatcher that directs flow to a sub-app. The problem isn't so much that you have to put layout and callbacks in seperate .py files but that managing the flow and name spacing callbacks is a nightmare! I would greatly appreciate this feature.

@sjtrny
Copy link

sjtrny commented Apr 20, 2019

I've made some progress towards @ned2 suggestion https://github.com/sjtrny/dash-multipage

Apps are defined as classes, where each app defines layout() and callbacks(), which are retrieved when necessary by MultiPageApp. Apps are agnostic of other apps and do not need to worry about namespace collision of layout ids. MultiPageApp automatically takes care of namespacing.

Routing is still seperate and managed by index.py but shouldn't be too hard to roll into the load_apps function.

Here's what the current interface looks like

app = MultiPageApp(__name__, external_stylesheets=external_stylesheets)
server = app.server

nav_bar = html.Div([
    dcc.Link('Home', href='/'),
    html.Br(),
    dcc.Link('Navigate to App 1', href='/apps/app1'),
    html.Br(),
    dcc.Link('Navigate to App 2', href='/apps/app2'),
])

pages = {
    'app1': App1(),
    'app2': App2(),
}

app.load_apps(pages)

routing_dict = {
    '/apps/app1': pages['app1'],
    '/apps/app2': pages['app2'],
}

def main_layout(pathname):

    extras = []
    if pathname in routing_dict:
        extras.extend(routing_dict[pathname].layout().children)
    return html.Div(nav_bar.children + extras)

app.set_layout(main_layout)

if __name__ == '__main__':
    app.run_server(debug=True)

@ned2
Copy link
Contributor Author

ned2 commented Apr 21, 2019

Neat abstraction @sjtrny! I would dearly love for namespacing of Dash pages (ie pairings of layout trees and groups of callbacks) to be supported by Dash natively. Your approach definitely solves the problem, but now you're working with an abstraction built on top of Dash.

I've got a conversation going about strategies for namespacing component IDs on the Dash forums here.

@sjtrny
Copy link

sjtrny commented Apr 21, 2019

Wish I saw that earlier, It's almost the same as mine!

@sjtrny
Copy link

sjtrny commented Aug 21, 2019

I just want to follow up on this idea, has any progress been made? It seemed like there was quite a bit of support for refactoring callback validation.

@alexcjohnson
Copy link
Collaborator

The immediate issue here was largely addressed in #1103 by moving validation to the front end. The namespacing / encapsulation part is still open as far as base Dash is concerned, but let's continue that discussion in #637 - closing this issue.

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

No branches or pull requests

5 participants