Repository navigation
[BUG] Concurrent callbacks broken in Dash 1.13+? #1350
Description
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.
Activity
ryanRahul5 commentedon Aug 27, 2020
I encountered the same issue with new versions of Dash (1.12 onward).
Marc-Andre-Rivet commentedon 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
divtriggers one callback:Clicking on the
buttontriggers two callbacks:For Dash v1.11 and v1.12, I get a slightly different:
Clicking on the
divtriggers one callback:Clicking on the
buttontriggers two callbacks: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
divandbuttonn_clicksbeing 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 thebuttonand thedivclick 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 commentedon Aug 27, 2020
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 propertiesMarc-Andre-Rivet commentedon Aug 27, 2020
I don't think it's related as both the
divand thebuttonwould callsetPropsfor legitimate reasons - the callbacks are not identical.alexcjohnson commentedon Aug 27, 2020
Weird that 1.11 and 1.12 trigger with
button.n_clickand thenbutton.n_clicksanddiv.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 commentedon Aug 27, 2020
@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
requestedCallbackswill be pushed at the end of the JS execution queue, which should be after all current event handling.Marc-Andre-Rivet commentedon Aug 27, 2020
@alexcjohnson Adding a delay does cause the
requestedCallbackobserver 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 subsequentdivcallback 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 commentedon 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:
PreventUpdate, so we say "A didn't change after all" and we remove it from the trigger list for CB1Anyway 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:
dash/dash-renderer/src/actions/dependencies.js
Lines 929 to 933 in d9a8d9c