Issue 1350 - Multi-input callback with sync event handling

[Marc-Andre-Rivet]

Fixes #1350

[alexcjohnson]

In order to ensure DIRECT doesn't turn into INDIRECT we need something like:

dash/dash-renderer/src/actions/dependencies_ts.ts

Line 25 in d9a8d9c

export const mergeMax = mergeWith(Math.max);

[Marc-Andre-Rivet]

@alexcjohnson Thanks for the heads up. Also distinguishing the initial callback from subsequent callbacks 8b6ff26

[Marc-Andre-Rivet]

Adding this little wait apparently has wide-ranging consequences on the timing of various features. So far:

• the rendering effect raising rendered is now incorrectly timed which causes isAppReady to never resolve at initialization as the event is triggered before the request is made, update here https://github.com/plotly/dash/pull/1385/files#diff-31f5093f6a582b2369ad2cd8216c4f67R69
• document title is now applied out of order and overridden by clientside callbacks with document.title = ..., updated here https://github.com/plotly/dash/pull/1385/files#diff-bd786cd26acb24c396ec355c2ff1c4a3R16
• timing of certain exceptions seems different enough to cause ordering issues in devtools, updated test here https://github.com/plotly/dash/pull/1385/files#diff-7b6da2b5053a169045af0651bfcb1b80R680

[Marc-Andre-Rivet]

I really dislike this artificial delay being required to counteract the additional processing delay in https://github.com/plotly/dash/pull/1385/files#diff-178b2bd05a773877fd1b117eef8b1e8cR63 but I can't think of a better alternative atm.

[Marc-Andre-Rivet]

Just some flexibility when creating observers

[Marc-Andre-Rivet]

Replaces <DocumentTitle /> and makes it arguably simpler to handle store changes and DOM mutations in a consistent manner as it doesn't need to deal with component lifecycle anymore.

[Marc-Andre-Rivet]

This solution should be more robust as it can react to both the DOM changing and the store changing in ways that are not dependant on exact execution timing (the additional wait broke the test/behavior)

[Marc-Andre-Rivet]

Merge callbacks, keep correct changePropIds DIRECT/INDIRECT and last know executionGroup if there is one. Always drop the initial callback if there is one and there are duplicates.

[Marc-Andre-Rivet]

I assume that this is due to timing changes: The not a tuple error started appearing first instead of second and broke the test. Fixing it.

[alexcjohnson]

@Marc-Andre-Rivet this PR appears to MOSTLY fix the issue @bpostlethwaite encountered 🎉 🏆 🍹

Would you mind including the app below (and its corrected behavior) as a test case?

The premise: when prop A affects prop B, and both of them affect prop C, then the C callback should list both A and B as triggers. In the app below, there are in fact two primary inputs (A and A') affecting B, and the important thing - the reason we care about having both inputs appear as triggers - is for C to know whether it was A or A' that caused the callback to fire.

Ben's problem: only B was being listed as a trigger.

What I see on this branch:

• On initial load, only B seems to be a trigger, when it should be all three: A, A', and B. But I bet Ben doesn't care about this case ;)
• Later on, when the user changes A, both A and B appear as triggers 🎉

Here's Ben's app:

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


app = dash.Dash(__name__)
app.layout = html.Div(
    [
        html.Div(
            style={"display": "block"},
            children=[
                html.Div(
                    [
                        html.Label("ID: input-number-1"),
                        dcc.Input(id="input-number-1", type="number", value=0),
                    ]
                ),
                html.Div(
                    [
                        html.Label("ID: input-number-2"),
                        dcc.Input(id="input-number-2", type="number", value=0),
                    ]
                ),
                html.Div(
                    [
                        html.Label("ID: sum-number"),
                        dcc.Input(
                            id="sum-number", type="number", value=0, disabled=True
                        ),
                    ]
                ),
            ],
        ),
        html.Div(
            id="results",
            style={
                "border": "antiquewhite",
                "borderStyle": "dashed",
                "height": "200px",
                "width": "475px",
            },
        ),
    ]
)


@app.callback(
    Output("sum-number", "value"),
    [Input("input-number-1", "value"), Input("input-number-2", "value")],
)
def update_sum_number(n1, n2):
    return n1 + n2


@app.callback(
    Output("results", "children"),
    [
        Input("input-number-1", "value"),
        Input("input-number-2", "value"),
        Input("sum-number", "value"),
    ],
)
def update_results(n1, n2, nsum):
    ctx = dash.callback_context
    return [
        "{} + {} = {}".format(n1, n2, nsum),
        html.Br(),
        "ctx.triggered={}".format(ctx.triggered)
    ]


if __name__ == "__main__":
    app.run_server()

[Marc-Andre-Rivet]

@alexcjohnson Dash version breakdown of callback context

Dash v1.5.0 to Dash v1.10.0 (pre PMC)
[{'prop_id': 'input-number-2.value', 'value': 0}]
[{'prop_id': 'sum-number.value', 'value': 0}]

Dash v1.11.0 and above
[{'prop_id': '.', 'value': None}]
[{'prop_id': 'sum-number.value', 'value': 0}]

---

After discussion, having initial callbacks without predecessors have no context feels like what was desired (triggered neither by user action of another callback). Callbacks with predecessors (e.g. sum-number.value above) will have triggers corresponding to the input props that got updated in previous callbacks. The Dash v1.11.0+ behavior will be locked in with a test and be considered the desired behavior.

[Marc-Andre-Rivet]

@alexcjohnson Wondering why we override the "no trigger context" case with https://github.com/plotly/dash/blob/dev/dash/_callback_context.py#L31

[alexcjohnson]

After discussion, having initial callbacks without predecessors have no context feels like what was desired (triggered neither by user action of another callback).

Yep right, I was forgetting this behavior is exactly what we decided during the original callback refactor back in #1103 - so as of this PR we can consider Ben's issue completely resolved.

Wondering why we override the "no trigger context" case with https://github.com/plotly/dash/blob/dev/dash/_callback_context.py#L31

See this discussion #1103 (comment) and response #1103 (comment) for the backward compatibility concerns that prompted this.

[alexcjohnson]

Rather than globals, most of our tests use multiprocessing.Value for this type of situation - a little cleaner I think, and doesn't depend on threading vs processes when the server and selenium parts branch.

dash/tests/integration/callbacks/test_multiple_callbacks.py

Line 18 in 9268480

call_count = Value("i", 0)

[Marc-Andre-Rivet]

Since callback_contexts is an array of dicts and clicks is a dict, I don't think I can easily use Value as it expects simple types. Instead used a scoped context class that should not leak outside of the test function. 90697b1

[alexcjohnson]

Well ok, I won't insist - it's probably fine, since we should always be using threads. I still like the multiprocessing pattern better as a general rule though, just to make it crystal clear that you're using this to communicate between the server thread and the selenium thread.

[alexcjohnson]

LGTM! Just one suggestion re: test patterns, everything else looks great!

[alexcjohnson]

This test-27 failure is interesting - seems plausible that one of the callback invocations was aborted before being requested because another keystroke came in first, which isn't a bad thing at all. Which means if we even want to keep this assertion maybe we just loosen it?