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

Input output callback #1525

Merged
merged 16 commits into from
Jan 16, 2021
Merged

Conversation

chadaeschliman
Copy link
Contributor

Allow circular callbacks in the special case of passing a component+property as an input and output of the same callback. By combining this capability with dash.callback_context it is straightforward to have multiple components remain synchronized. Other forms of circularity (involving multiple callbacks) are still detected and blocked. This is related to: #889

Example code for a linked slider and numeric input:

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

external_stylesheets = ['https://codepen.io/chriddyp/pen/bWLwgP.css']
app = dash.Dash(__name__, external_stylesheets=external_stylesheets)

app.layout = html.Div(children=[
    html.H1(children='Circular Test'),
    dcc.Slider(
        id='slider',
        min=0, max=20,
    ),
    dcc.Input(
        id='input',
        type='number',
        debounce=False,
        value=3
    ),
])

@app.callback(
    Output('input', 'value'),
    Output('slider', 'value'),
    Input('input', 'value'),
    Input('slider', 'value'),
)
def callback(iv, sv):
    ctx = dash.callback_context
    if not ctx.triggered:
        # default to input value at init
        if iv is not None:
            value = iv
        else:
            value = sv
    else:
        trigger_id = ctx.triggered[0]["prop_id"].split(".")[0]
        if trigger_id=='input':
            value = iv
        else:
            value = sv
    return value, value

app.run_server(debug=True)

Result:
circular_test

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • Disable checking for input/output overlap
    • Disable checking for circularity in the special case that an output is also an input
    • Make changes to ensure callbacks execute correctly (wasn't needed)
    • Modify tests
  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

@alexcjohnson
Copy link
Collaborator

Thanks @chadaeschliman - this is really interesting. I think you're correct that logically this is self-consistent: any time one of the inputs to this callback changes, we should execute the callback exactly once and whatever values it returns become the final values of the input/output props. This pattern could also be used for input validation.

@chriddyp @Marc-Andre-Rivet thoughts? Am I missing anything? Any reason we wouldn't want to allow this kind of circularity?

There are a number of cases we'll need to write CI tests for. Consider this set of callbacks where a and b are connected circularly and there's an extra input e and extra outputs c and (via another callback) d:

cb1:
Output('a', 'value'), Output('b', 'value'), Output('c', 'value'),
Input('e', 'value'), Input('a', 'value'), Input('b', 'value')

cb2:
Output('d', 'value'),
Input('a', 'value')

If you change a, it's important that first cb1 fires, changing b and c and possibly changing a too (maybe rounding, maybe validating it), and then cb2 fires using the value of a returned by cb1 and NOT the one the user entered (that was fed to cb1. cb1 and cb2 should each fire exactly once.

If you change b or e, the same should happen though in these cases it's more obvious that we'd use the final value of a since that's the only change in its value that could trigger cb2

@chadaeschliman
Copy link
Contributor Author

chadaeschliman commented Jan 14, 2021

Thanks @alexcjohnson for considering this. You were correct that depending on the order the callbacks were defined, sometimes cb2 would be called twice (first with the original value then with the new value). The issue was that initially there were no "readyCallbacks" because cb1 depended on itself and so the fallback code was triggered which resulted in cb2 firing first. I made a straightforward fix by modifying getReadyCallbacks to not consider overlapped inputs/outputs as outstanding outputs. With this change, cb1 is initially the only readyCallback so it fires first followed by cb2. I'm not sure how to make a CI test that verifies the order of callback execution.

@alexcjohnson
Copy link
Collaborator

With this change, cb1 is initially the only readyCallback so it fires first followed by cb2.

🎉

I'm not sure how to make a CI test that verifies the order of callback execution.

Make the circular callback actually change the values, and make the downstream callback return different values depending on which of the values it receives. Then all you have to do is inspect the final contents of each element, and count how many times each callback has been executed using a multiprocessing.Value like:

assert call_count.value == 2 + len("hello world"), "initial count + each key stroke"

If both of those come out right, we know the order was correct.

@chadaeschliman
Copy link
Contributor Author

Thanks @alexcjohnson for the tip. I added test cbsc015 which checks this. I'm not sure why percy/dash failed on the latest build.

@alexcjohnson
Copy link
Collaborator

The new test looks great! Don't worry about the Percy failure, that came from some authorization issues we're working through with CircleCI.

It occurs to me it would be good to have the same test also with both callbacks clientside, since that can have different sequencing. That obviously can't use a multiprocessing.Value but it can stash a value in a window variable and read it back out with dash_duo.driver.execute_script.

And unless @Marc-Andre-Rivet has anything else to add, the only other thing I think we need is a changelog entry.

@chadaeschliman
Copy link
Contributor Author

Good idea. I added clsd014 which is the clientside version of cbsc015. I've never used clientside callbacks before. It seemed pretty straightforward but please check it over and make sure I implemented it correctly. Thanks

CHANGELOG.md Outdated Show resolved Hide resolved
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.

Yep, your clientside test looks great - hah, these days we're recommending putting the js function inline like this:

app.clientside_callback(
"""
function(vals) {
var len = vals.length;
return len < 2 ? len : +(vals[len - 1] || 0) + +(vals[len - 2] || 0);
}
""",
Output({"i": MATCH}, "children"),
Input({"i": ALLSMALLER}, "children"),
)

But ironically we haven't included that form in test_clientside 🙄

Looks great in the callback graph - here's your app plus one downstream callback:
Screen Shot 2021-01-15 at 7 43 55 PM

So I think we're good to go - thanks again for the PR! 💃

@chadaeschliman
Copy link
Contributor Author

Yeah! Thanks for your help!

@nirvana-msu
Copy link

I have an exact opposite use-case to the prunning implemented here - I need a callback to actually fire in an infinite circular fashion - see #2139 for details. Would be great if we could have an option to avoid this infinite loop (of a single callback) instead of prunning as it is done here.

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.

3 participants