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] pattern-matching callbacks with empty or partly-empty Output #1216

Closed
alexcjohnson opened this issue Apr 29, 2020 · 4 comments · Fixed by #1220
Closed

[BUG] pattern-matching callbacks with empty or partly-empty Output #1216

alexcjohnson opened this issue Apr 29, 2020 · 4 comments · Fixed by #1220
Assignees
Labels

Comments

@alexcjohnson
Copy link
Collaborator

As reported in the dash 1.11 forum post we currently have problems when a pattern-matching callback matches no items in its Output(s). This particular case is straightforward, the callback simply shouldn't fire and no error should occur. But there are multiple variants that deserve scrutiny:

  1. Output({key1: ALL}) (the case posted on the forum - the ALL can match no items)
  2. Output({key1: ALL, key2: MATCH}) (same thing but the pattern has a MATCH too - again the callback should not fire with no matching items)
  3. [Output({key1: ALL}), Output({key2: ALL})] (two ALL patterns - if either one matches items the callback should fire, but if neither matches anything it should not)
  4. [Output({key1: ALL}), Output(str)] (an ALL and a simple string ID - the callback should always fire)
  5. [Output({key1: ALL, key2: MATCH}), Output({key2: MATCH})] (an ALL+MATCH and a MATCH - the callback should always fire)

Should removing items from an Output(ALL) trigger the callback?

This I'm not so sure about. Removing items from an Input(ALL) certainly should and does trigger the callback, and adding items to Output(ALL) triggers the callback as an "initial call" for the new item.

In practice this currently mainly happens when you remove the entire category - but that will change when we implement array operation callbacks #968.

The argument I can think of for not triggering is that the inputs to the callback haven't changed, and if the callback is expensive you may not want to rerun it just because the user deleted something from the page. Also if it's clear to users that this is expected, they can force the callback to run on item removal by adding Input({key1: ALL}, "id") matching the Output({key1: ALL}, prop)

The argument I see in favor of triggering is that these outputs often depend on each other, and the list of outputs you're required to return can itself be thought of as an input. Indeed, this is how I discovered the issue, as I made a test app one way to cover case (1) and then tried to modify it for case (4) and was surprised that it mostly worked, until I cleared the ALL match.

Here's the original version of the test app (only works on the missing-outputs branch):

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


app = dash.Dash(__name__)

app.layout = html.Div(children=[
    html.Button("items", id="btn-1"),
    html.Button("values", id="btn-2"),
    html.Div(id="content"),
    html.Div("Output init", id="output"),
])


@app.callback(Output("content", "children"), [Input("btn-1", "n_clicks")])
def content(n1):
    return [html.Div(id={"i": i}) for i in range((n1 or 0) % 4)]


@app.callback(Output({"i": ALL}, "children"), [Input("btn-2", "n_clicks")])
def content_inner(n2):
    n1 = len(dash.callback_context.outputs_list)
    if not n1:
        raise ValueError("should not be called with no outputs!")
    return [n2 or 0] * n1


@app.callback(Output("output", "children"), [Input({"i": ALL}, "children")])
def out2(contents):
    return sum(contents)


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

And the modification, combining the last two callbacks into one:

@app.callback(
    [Output({"i": ALL}, "children"), Output("output", "children")],
    [Input("values", "n_clicks")]
)
def content_and_output(n2):
    n1 = len(dash.callback_context.outputs_list[0])
    content = [n2 or 0] * n1
    return content, sum(content)

Which works great until 4 clicks of the items button, when content gets cleared.

You can fix it either by adding items as an input and recalculating n1 from that, or by adding Input({"i": ALL}, "id"). The question is whether that should be necessary or if I should treat this as a bug.

@alexcjohnson alexcjohnson self-assigned this Apr 29, 2020
@alexcjohnson
Copy link
Collaborator Author

Also: #1212 treats all-missing inputs as a normal situation again. What about all-missing outputs when some or all inputs are present? That feels like it might be something we should support for similar reasons, but I haven't tested the actual behavior both today and prior to 1.11.

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented Apr 29, 2020

Should removing items from an Output(ALL) trigger the callback?

Initial reaction: No.
It's not an input so does not have a say on triggering callbacks. I'd also be worried about triggering additional callbacks that wouldn't do anything outside of scenarios where there are dependencies between the outputs.

Could this be handled in a satisfactory way, and explicitly, if we were to allow callback Input to also be its output? - no infinite loops (arguably, there's nothing an infinite callback loop could do that couldn't be done in a single callback looping until it reaches some stable state?)

@alexcjohnson
Copy link
Collaborator Author

Initial reaction: No.

Good, that's the way I'm leaning as well. Curious to hear other perspectives too!

Could this be handled in a satisfactory way, and explicitly, if we were to allow callback Input to also be its output? - no infinite loops

If the goal is just to respond to changes in available elements, this is what you get from using the id prop with the same pattern as your Output - which anyway is already a trick we describe on https://dash.plotly.com/pattern-matching-callbacks as a way to get the list of matches without using callback_context, though in that example we use it as State so it wouldn't trigger.

(arguably, there's nothing an infinite callback loop could do that couldn't be done in a single callback looping until it reaches some stable state?)

haha that's an interesting thought... automatic iteration for an unknown duration. 😱

@chriddyp
Copy link
Member

Agreed: Initial reaction: No

they can force the callback to run on item removal by adding Input({key1: ALL}, "id") matching the Output({key1: ALL}, prop)

Agreed with this as well 👍

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

Successfully merging a pull request may close this issue.

3 participants