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

Multi output callbacks support. #436

Merged
merged 38 commits into from
Mar 1, 2019
Merged

Multi output callbacks support. #436

merged 38 commits into from
Mar 1, 2019

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Oct 25, 2018

Release candidates

pip install dash==0.38.0rc1

Add multi output callbacks support.

  • Use a list/tuple of Output as output in callbacks.
  • Return a tuple/list of value from the callbacks
    • The returned list must have the same length as the list of output
    • The output props are applied in the order they were declared in the output list.

Usage example:

import dash

import dash_html_components as html
import dash_core_components as dcc
from dash.dependencies import Output, Input, State
from dash.exceptions import PreventUpdate

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

app.layout = html.Div([
    html.Button('OUTPUT', id='output-btn'),

    html.Table([
        html.Thead([
            html.Tr([
                html.Th('Output 1'),
                html.Th('Output 2')
            ])
        ]),
        html.Tbody([
            html.Tr([html.Td(id='output1'), html.Td(id='output2')]),
        ])
    ]),
])


@app.callback([Output('output1', 'children'), Output('output2', 'children')],
              [Input('output-btn', 'n_clicks')],
              [State('output-btn', 'n_clicks_timestamp')])
def on_click(n_clicks, n_clicks_timestamp):
    if n_clicks is None:
        raise PreventUpdate

    return n_clicks, n_clicks_timestamp


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

Needs plotly/dash-renderer#91

Tests will fails until I release a rc version of dash-renderer, this is kind of breaking changes as no apps will run if they don't have the right version of dash-renderer (The json payload changed).

Todos:

  • rc versions for tests
  • re-enable callback validation
  • add integration test

@sdementen
Copy link

hello,
I just saw the 1.0.0 announcement #469 but could not find this topic in it (multiple output).
Is it too early for it to be included ?

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 18, 2018

@sdementen This is a standalone update. You can try it right now with:

pip install dash==0.36.0rc1
pip install dash-renderer==0.17.0rc1

@T4rk1n T4rk1n changed the title [WIP] Multi output callbacks support. Multi output callbacks support. Dec 18, 2018
@T4rk1n T4rk1n requested a review from rmarren1 December 18, 2018 22:46
@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 18, 2018

@rmarren1 Please review.

@rmarren1
Copy link
Contributor

rmarren1 commented Dec 20, 2018

This allows you to define multiple callbacks to the same output.

For example, adding

@app.callback(Output('output1', 'children'),
              [Input('output-btn', 'n_clicks')])
def on_click(n_clicks):
    if n_clicks is None:
        raise PreventUpdate

    return 'something else'

to the example you give above results in a race condition (sometimes Output 1 is 'something else', sometimes it is the number of clicks).

I think this just needs logic changed in the condition throwing exceptions.CantHaveMultipleOutputs

@rmarren1
Copy link
Contributor

rmarren1 commented Dec 20, 2018

I think we should eventually allow multiple callbacks to be defined for an output component on the condition that those input sets are disjoint (e.g. I1, I2 -> O1 and I3, I4 -> O1 can be simultaneously defined but not I1, I2 -> O1 and I2, I3 -> O1 ). Definitely the scope of this PR, but something to keep in mind as a future possibility when doing above ^

@alexcjohnson
Copy link
Collaborator

I think we should eventually allow multiple callbacks to be defined for an output component on the condition that those input sets are disjoint

This discussion might belong in #475, but I'm skeptical of both the idea of multiple callbacks and the disjoint exception. I guess you're imagining cases where one set of inputs disappears and another appears? But you could still have some other global setting that factors in to the final output... so seems to me the cleaner way to handle it is to make the output depend on all of the inputs, but the conditionally-available ones are wildcarded somehow - eg @chriddyp's #475 (comment)

dash/dash.py Outdated
@@ -320,6 +321,7 @@ def serve_layout(self):

def _config(self):
config = {
'multi_output': True,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the idea to make this configurable by the user?

Copy link
Contributor

Choose a reason for hiding this comment

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

Or is this for backward compatibility?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's for backward compatibility.

dash/dash.py Outdated Show resolved Hide resolved
@T4rk1n T4rk1n force-pushed the multi-output branch 6 times, most recently from d87013b to 98d34e9 Compare December 26, 2018 23:53
@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 27, 2018

@rmarren1

I think this just needs logic changed in the condition throwing exceptions.CantHaveMultipleOutputs

I added validation for multi output and changed CantHaveMultipleOutputs to DuplicateCallbackOutput. There's two tests for the logic, think of any other test cases ?

dash/dash.py Show resolved Hide resolved
dash/dash.py Outdated Show resolved Hide resolved
dash/dash.py Outdated Show resolved Hide resolved
dash/dash.py Outdated Show resolved Hide resolved
@rmarren1
Copy link
Contributor

I think there will be an issue encoding the IDs of multi-output callbacks as an ASCII-delimited string, since there will be a backwards compatibility issue with apps that use the ASCII characters we choose as delimiters.

One idea: We already blacklisted ., so we can transform [output1.prop1:output2.prop2] to ...output1.prop1..output2.prop2..., e.g. replace [ and ] with ... and replace : with .., pretty hacky but it should work.

Another idea is to use a different key for self.callback_map. The retrieval of callbacks is an internally facing API and it appears the only reason we use the output ID to identify these is that it is naturally unique per callback. We could use hash(frozenset(['output1.prop1', 'output2.prop2'])), and then just pass a list of outputs back-and-forth between the front-end and back-end like we do with the output id, re-computing the hash when needed. I think this may be more natural, e.g. to test if a callback is multi-output on the front-end you just see if the output is a list rather than if it starts with [.

Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

Excellent work @T4rk1n, this is going to make a lot of people happy 🎉 💃
I'll leave it up to you how to manage the renderer-dash dependency tango as these two PRs are merged 😅

@alexcjohnson alexcjohnson merged commit 9db79cb into master Mar 1, 2019
@alexcjohnson alexcjohnson deleted the multi-output branch March 1, 2019 18:37
@@ -28,7 +28,7 @@ jobs:
sudo pip install virtualenv
virtualenv venv
. venv/bin/activate
pip install -r $REQUIREMENTS_FILE
pip install -r $REQUIREMENTS_FILE --force-reinstall
Copy link
Collaborator

Choose a reason for hiding this comment

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

@T4rk1n I've already merged this but... was --force-reinstall just for debugging or do we need it going forward if we're using git branches in requirements files?

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.

6 participants