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

[BUG] Concurrent callbacks broken in Dash 1.13+? #1350

Closed
m-ad opened this issue Aug 1, 2020 · 8 comments · Fixed by #1385
Closed

[BUG] Concurrent callbacks broken in Dash 1.13+? #1350

m-ad opened this issue Aug 1, 2020 · 8 comments · Fixed by #1385
Assignees
Milestone

Comments

@m-ad
Copy link

m-ad commented Aug 1, 2020

Describe your context
The MWE code below does work in this environment:

dash                              1.12.0
dash-core-components              1.10.0
dash-html-components              1.0.3
dash-leaflet                      0.0.19
dash-renderer                     1.4.1
dash-table                        4.7.0

The MWE code below does not work in this environment:

dash                              1.13.4
dash-core-components              1.10.1
dash-html-components              1.0.3
dash-leaflet                      0.0.19
dash-renderer                     1.5.1
dash-table                        4.8.1

Describe the bug

When there are nested elements with callbacks (e.g. a button inside a div and both have conflicting callbacks), the documented method for differentiating between them (checking ctx.triggered[0]['prop_id'].split('.')[0]) does not work for Dash 1.13+ (it works with 1.12, though). I am not completely sure whether this was a deliberate change and an update to the documentation is needed or this is a bug.

I have also asked in the community forum first.

Minimal working example code

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

app = dash.Dash()
app.layout = html.Div(id="div", style={"background": "yellow", "padding": "200px"}, children=[
    html.Button(id="button", children="show infotext"),
    html.Div(id="info", children="INFOTEXT", style={"display": "none"})
])


@app.callback(
    Output("info", "style"),
    [Input("button", "n_clicks"),
     Input("div", "n_clicks")])
def test(btn, div):
    ctx = dash.callback_context
    print("Callback", ctx.triggered)
    prop_ids = [x['prop_id'].split('.')[0] for x in ctx.triggered]
    if "button" in prop_ids:
        print("Action: show")
        return {"display": "block"}
    else:
        print("Action: hide")
        return {"display": "none"}

app.run_server(debug=True)

Expected behavior

When the button is pressed, the info box should be displayed. When the background is clicked, the infobox should be hidden.

This code produces the expected behaviour in Dash 1.12. There, the callback is executed once only and prop_ids is a list of all callbacks ([“button”, “div”] when clicking on the button and [“div”] when clicking on the div).

Actual behavior

Since version 1.13, dash behaves differently. The callback function is now executed twice, one time for the button callback and one time for the div. This makes it impossible to figure out which one was actually pressed.

Example output (Dash 1.13) for the above code after clicking one time on the button (the None callback is just the initial callback on load an can be ignored):

Callback [{'prop_id': '.', 'value': None}]
Action: hide
Callback [{'prop_id': 'button.n_clicks', 'value': 1}]
Action: show
Callback [{'prop_id': 'div.n_clicks', 'value': 1}]
Action: hide

So, the button callback is executed, the infobox is displayed, but then the div callback immediately hides the infobox again.

@ryanRahul5
Copy link

I encountered the same issue with new versions of Dash (1.12 onward).

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Aug 27, 2020

@alexcjohnson This is rather interesting, running this against Dash v1.5.0 and above, I get the following callbacks on click for all versions but Dash v1.11 and Dash v1.12:

Clicking on the div triggers one callback:

Callback [{'prop_id': 'div.n_clicks', 'value': 1}]

Clicking on the button triggers two callbacks:

Callback [{'prop_id': 'button.n_clicks', 'value': 1}]
Callback [{'prop_id': 'div.n_clicks', 'value': 3}]

For Dash v1.11 and v1.12, I get a slightly different:

Clicking on the div triggers one callback:

Callback [{'prop_id': 'div.n_clicks', 'value': 1}]

Clicking on the button triggers two callbacks:

Callback [{'prop_id': 'button.n_clicks', 'value': 1}]
Callback [{'prop_id': 'button.n_clicks', 'value': 1}, {'prop_id': 'div.n_clicks', 'value': 3}]

v1.11 and v1.12 are the only versions for which the code above can work.
This hiatus corresponds to the introduction of pattern matching callbacks in v1.11 and the callback chain rework of v1.13.
Seems like v1.11 changed an undocumented Dash behavior that got reverted accidentally in v1.13


Interestingly enough since React does not prevent event propagation automatically when an event is handled, it's normal to see both the div and button n_clicks being triggered. So while technically not a regression with the baseline behavior of Dash since <= v1.10, it does raise the question of how to accurately distinguish the button and the div click events.

I wonder if the correct approach for html components would be to by-default (or opt-in) stop event propagation when it triggers a callback. Allowing for the use case described here to actually work.

@chriddyp
Copy link
Member

Seeing "this triggers the callback twice" reminds me of #1049. Not sure if related or not or #1049 is just something with the dcc.Location & its derived properties

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Aug 27, 2020

I don't think it's related as both the div and the button would call setProps for legitimate reasons - the callbacks are not identical.

@alexcjohnson
Copy link
Collaborator

Weird that 1.11 and 1.12 trigger with button.n_click and then button.n_clicks and div.n_clicks - ie the same button click appears as triggering twice (albeit with the same value)?

If I had thought about this situation before seeing what we did, I'd say it should trigger the callback once with both props triggering. It's only a single action after all, so it should appear as a single action. That looks like it would work for this use case, and even without changing the code. If we later add an ability to stop propagation then fine, but that will need to be opt-in regardless.

@Marc-Andre-Rivet perhaps there's a way we can make what I'm describing happen by somehow waiting to fire callbacks until propagation has had a chance to complete?

@Marc-Andre-Rivet
Copy link
Contributor

@alexcjohnson I think the best candidate might be introducing an artificial async/wait delay here, just before getting the variables: https://github.com/plotly/dash/blob/dev/dash-renderer/src/observers/requestedCallbacks.ts#L56 - processing of requestedCallbacks will be pushed at the end of the JS execution queue, which should be after all current event handling.

@Marc-Andre-Rivet
Copy link
Contributor

@alexcjohnson Adding a delay does cause the requestedCallback observer to wait for the event propagation to completes and to have access to both callbacks at the same time. The problem now is that the callbacks get identified as duplicated (which is fine so far). The first callback (btn) is dropped as it is oldest and only the subsequent divcallback is sent.

Seems that instead of simply dropping the older callbacks we would now want to merge them, giving priority to props of newer callbacks.

@alexcjohnson
Copy link
Collaborator

alexcjohnson commented Aug 27, 2020

good point! It doesn't seem to me as though we ever want to simply drop the older copy, and there's no ambiguity about prop values, the only question is what to do with triggers. I think merging them is correct, just trying to think whether there are any edge cases we need to consider.

OK here's one. I don't know why you'd do this but see if this makes sense:

  • CB1 has inputs A and B
  • CB2 has input B and output A (maybe to conditionally clear the value of A?)
  • The event triggers A and then bubbles to B
  • We get two invocations of CB1, which we merge into one, with both A and B listed as triggers
  • But CB1 is deferred until CB2 returns
  • CB2 calls PreventUpdate, so we say "A didn't change after all" and we remove it from the trigger list for CB1
  • So even though A did change vs the previous time CB1 was invoked, so should be a trigger, only B appears as a trigger.

Anyway merging the triggers would still be the correct thing to do, this is just a second-order issue to look out for after that.

Edit: we might already handle this case correctly, based on this logic:

* changedPropIds: an object of {[idAndProp]: v} triggering this callback
* v = DIRECT (2): the prop was changed in the front end, so dependent
* callbacks *MUST* be executed.
* v = INDIRECT (1): the prop is expected to be changed by a callback,
* but if this is prevented, dependent callbacks may be pruned.

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

Successfully merging a pull request may close this issue.

5 participants