From dc73cef9998f5a686b059bc8a04db5ffd745a135 Mon Sep 17 00:00:00 2001 From: eff-kay Date: Wed, 30 Nov 2022 02:25:58 -0500 Subject: [PATCH 1/5] fix callback issue --- .../src/actions/dependencies_ts.ts | 81 ++++++++++++++- .../src/observers/requestedCallbacks.ts | 6 +- .../callbacks/test_multiple_callbacks.py | 98 +++++++++++++++++++ 3 files changed, 181 insertions(+), 4 deletions(-) diff --git a/dash/dash-renderer/src/actions/dependencies_ts.ts b/dash/dash-renderer/src/actions/dependencies_ts.ts index 43f7902e28..8a0ef84b1f 100644 --- a/dash/dash-renderer/src/actions/dependencies_ts.ts +++ b/dash/dash-renderer/src/actions/dependencies_ts.ts @@ -145,16 +145,65 @@ export function getPriority( return map(i => Math.min(i, 35).toString(36), priority).join(''); } +export function getAllSubsequentOutputsForCallback( + graphs: any, + paths: any, + callback: ICallback +) { + let callbacks: ICallback[] = [callback]; + let touchedOutputs: {[key: string]: boolean} = {}; + + // this traverses the graph all the way to the end + while (callbacks.length) { + // don't add it if it already exists based on id and props + const outputs = filter( + o => !touchedOutputs[combineIdAndProp(o)], + flatten(map(cb => flatten(cb.getOutputs(paths)), callbacks)) + ); + + touchedOutputs = reduce( + (touched, o) => assoc(combineIdAndProp(o), true, touched), + touchedOutputs, + outputs + ); + + callbacks = flatten( + map( + ({id, property}: any) => + getCallbacksByInput( + graphs, + paths, + id, + property, + INDIRECT, + false + ), + outputs + ) + ); + } + + return touchedOutputs; +} + export const getReadyCallbacks = ( paths: any, candidates: ICallback[], - callbacks: ICallback[] = candidates + callbacks: ICallback[] = candidates, + graphs:any = {} ): ICallback[] => { // Skip if there's no candidates if (!candidates.length) { return []; } + console.log( + 'dependencies:getReadyCallbacks callbacks', + callbacks, + 'candidates', + candidates + ); + // Find all outputs of all active callbacks const outputs = map( combineIdAndProp, @@ -166,9 +215,37 @@ export const getReadyCallbacks = ( ); // Make `outputs` hash table for faster access - const outputsMap: {[key: string]: boolean} = {}; + let outputsMap: {[key: string]: boolean} = {}; forEach(output => (outputsMap[output] = true), outputs); + + // find all the outputs touched by activeCallbacks + // remove this check if graph is accessible all the time + + if (Object.keys(graphs).length) { + //not sure if graph will be accessible all the time + const allTouchedOutputs: {[key: string]: boolean}[] = flatten( + map( + cb => getAllSubsequentOutputsForCallback(graphs, paths, cb), + callbacks + ) + ); + + console.log( + 'dependencies:getReadyCallbacks allTouchedOutputs', + allTouchedOutputs + ); + + // overrrides the outputsMap, will duplicate callbacks filtered + // this is only done to silence typescript errors + if (allTouchedOutputs.length > 0) { + outputsMap = Object.assign( + allTouchedOutputs[0], + ...allTouchedOutputs + ); + } + } + // Find `requested` callbacks that do not depend on a outstanding output (as either input or state) // Outputs which overlap an input do not count as an outstanding output return filter( diff --git a/dash/dash-renderer/src/observers/requestedCallbacks.ts b/dash/dash-renderer/src/observers/requestedCallbacks.ts index 855b5f141b..e6179495d5 100644 --- a/dash/dash-renderer/src/observers/requestedCallbacks.ts +++ b/dash/dash-renderer/src/observers/requestedCallbacks.ts @@ -62,7 +62,8 @@ const observer: IStoreObserverDefinition = { const { callbacks, callbacks: {prioritized, blocked, executing, watched, stored}, - paths + paths, + graphs } = getState(); let { callbacks: {requested} @@ -234,7 +235,8 @@ const observer: IStoreObserverDefinition = { let readyCallbacks = getReadyCallbacks( paths, requested, - pendingCallbacks + pendingCallbacks, + graphs ); let oldBlocked: ICallback[] = []; diff --git a/tests/integration/callbacks/test_multiple_callbacks.py b/tests/integration/callbacks/test_multiple_callbacks.py index 4442e0aac5..8f581aaa3f 100644 --- a/tests/integration/callbacks/test_multiple_callbacks.py +++ b/tests/integration/callbacks/test_multiple_callbacks.py @@ -6,6 +6,8 @@ from dash import Dash, Input, Output, State, callback_context, html, dcc, dash_table from dash.exceptions import PreventUpdate +import dash.testing.wait as wait + def test_cbmt001_called_multiple_times_and_out_of_order(dash_duo): app = Dash(__name__) @@ -578,3 +580,99 @@ def callback(*args): assert call_counts[outputid].value == 1 assert call_counts["container"].value == (1 if generate else 0) + + + +def test_cbmt013_chained_callback_should_be_blocked(dash_duo): + all_options = { + "America": ["New York City", "San Francisco", "Cincinnati"], + "Canada": ["Montreal", "Toronto", "Ottawa"], + } + + app = dash.Dash(__name__) + app.layout = html.Div( + [ + dcc.RadioItems( + id="countries-radio", + options=[{"label": k, "value": k} for k in all_options.keys()], + value="America", + ), + html.Hr(), + dcc.RadioItems(id="cities-radio"), + html.Hr(), + html.Div(id="display-selected-values"), + ] + ) + + @app.callback(Output("cities-radio", "options"), Input("countries-radio", "value")) + def set_cities_options(selected_country): + return [{"label": i, "value": i} for i in all_options[selected_country]] + + @app.callback(Output("cities-radio", "value"), Input("cities-radio", "options")) + def set_cities_value(available_options): + return available_options[0]["value"] + + @app.callback( + Output("display-selected-values", "children"), + Input("countries-radio", "value"), + Input("cities-radio", "value"), + ) + def set_display_children(selected_country, selected_city): + return "{} is a city in {}".format(selected_city, selected_country,) + + dash_duo.start_server(app) + + not_null = ( + lambda: dash_duo.find_element("#display-selected-values").get_attribute( + "innerText" + ) + != "" + ) + + wait.until(not_null, 20) + + new_york_text = "New York City is a city in America" + current_text = dash_duo.find_element("#display-selected-values").get_attribute( + "innerText" + ) + + assert current_text == new_york_text, "{} should equal {}".format( + current_text, new_york_text + ) + + all_inputs = dash_duo.find_elements("input") + + relevant_input = dash_duo.driver.execute_script( + """ + var inp = arguments[0].filter((input)=>{ + console.log(input.parentElement.innerText) + return input.parentElement.innerText=='Canada' + }) + console.log("pringing something", inp) + return inp[0] + """, + all_inputs, + ) + + dash_duo.driver.set_network_conditions( + offline=False, + latency=5, # additional latency (ms) + download_throughput=1 * 1024, # maximal throughput + upload_throughput=1 * 1024, + ) # maximal throughput + + relevant_input.click() + + canada_text = "Montreal is a city in Canada" + expected_text = [new_york_text, canada_text] + + def wait_cond(): + current_elem = dash_duo.find_element("#display-selected-values").get_attribute( + "innerText" + ) + assert current_elem in expected_text, "{} should be one of {}".format( + current_elem, expected_text + ) + return current_elem == "Montreal is a city in Canada" + + wait.until(wait_cond, 20) \ No newline at end of file From e1a155b403c0c19611fb4efceb0a7b5c79df9d37 Mon Sep 17 00:00:00 2001 From: eff-kay Date: Wed, 30 Nov 2022 02:32:21 -0500 Subject: [PATCH 2/5] fix callback issue --- .../src/actions/dependencies_ts.ts | 17 ++--------------- .../callbacks/test_multiple_callbacks.py | 10 ++++++---- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/dash/dash-renderer/src/actions/dependencies_ts.ts b/dash/dash-renderer/src/actions/dependencies_ts.ts index 8a0ef84b1f..50c342f5c7 100644 --- a/dash/dash-renderer/src/actions/dependencies_ts.ts +++ b/dash/dash-renderer/src/actions/dependencies_ts.ts @@ -190,20 +190,13 @@ export const getReadyCallbacks = ( paths: any, candidates: ICallback[], callbacks: ICallback[] = candidates, - graphs:any = {} + graphs: any = {} ): ICallback[] => { // Skip if there's no candidates if (!candidates.length) { return []; } - console.log( - 'dependencies:getReadyCallbacks callbacks', - callbacks, - 'candidates', - candidates - ); - // Find all outputs of all active callbacks const outputs = map( combineIdAndProp, @@ -218,8 +211,7 @@ export const getReadyCallbacks = ( let outputsMap: {[key: string]: boolean} = {}; forEach(output => (outputsMap[output] = true), outputs); - - // find all the outputs touched by activeCallbacks + // find all the outputs touched by activeCallbacks // remove this check if graph is accessible all the time if (Object.keys(graphs).length) { @@ -231,11 +223,6 @@ export const getReadyCallbacks = ( ) ); - console.log( - 'dependencies:getReadyCallbacks allTouchedOutputs', - allTouchedOutputs - ); - // overrrides the outputsMap, will duplicate callbacks filtered // this is only done to silence typescript errors if (allTouchedOutputs.length > 0) { diff --git a/tests/integration/callbacks/test_multiple_callbacks.py b/tests/integration/callbacks/test_multiple_callbacks.py index 8f581aaa3f..a38443445b 100644 --- a/tests/integration/callbacks/test_multiple_callbacks.py +++ b/tests/integration/callbacks/test_multiple_callbacks.py @@ -582,14 +582,13 @@ def callback(*args): assert call_counts["container"].value == (1 if generate else 0) - def test_cbmt013_chained_callback_should_be_blocked(dash_duo): all_options = { "America": ["New York City", "San Francisco", "Cincinnati"], "Canada": ["Montreal", "Toronto", "Ottawa"], } - app = dash.Dash(__name__) + app = Dash(__name__) app.layout = html.Div( [ dcc.RadioItems( @@ -618,7 +617,10 @@ def set_cities_value(available_options): Input("cities-radio", "value"), ) def set_display_children(selected_country, selected_city): - return "{} is a city in {}".format(selected_city, selected_country,) + return "{} is a city in {}".format( + selected_city, + selected_country, + ) dash_duo.start_server(app) @@ -675,4 +677,4 @@ def wait_cond(): ) return current_elem == "Montreal is a city in Canada" - wait.until(wait_cond, 20) \ No newline at end of file + wait.until(wait_cond, 20) From 3c0e9594a0190d693c51e26bbc2d92bdc0976904 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Tue, 6 Dec 2022 15:49:48 -0500 Subject: [PATCH 3/5] simplify chained callback block test --- .../callbacks/test_multiple_callbacks.py | 94 ++++++++----------- 1 file changed, 41 insertions(+), 53 deletions(-) diff --git a/tests/integration/callbacks/test_multiple_callbacks.py b/tests/integration/callbacks/test_multiple_callbacks.py index a38443445b..33e37f0dc9 100644 --- a/tests/integration/callbacks/test_multiple_callbacks.py +++ b/tests/integration/callbacks/test_multiple_callbacks.py @@ -1,5 +1,5 @@ import time -from multiprocessing import Value +from multiprocessing import Value, Lock import pytest @@ -19,7 +19,7 @@ def test_cbmt001_called_multiple_times_and_out_of_order(dash_duo): @app.callback(Output("output", "children"), [Input("input", "n_clicks")]) def update_output(n_clicks): - call_count.value = call_count.value + 1 + call_count.value += 1 if n_clicks == 1: time.sleep(1) return n_clicks @@ -603,12 +603,19 @@ def test_cbmt013_chained_callback_should_be_blocked(dash_duo): ] ) + opts_call_count = Value("i", 0) + city_call_count = Value("i", 0) + out_call_count = Value("i", 0) + out_lock = Lock() + @app.callback(Output("cities-radio", "options"), Input("countries-radio", "value")) def set_cities_options(selected_country): + opts_call_count.value += 1 return [{"label": i, "value": i} for i in all_options[selected_country]] @app.callback(Output("cities-radio", "value"), Input("cities-radio", "options")) def set_cities_value(available_options): + city_call_count.value += 1 return available_options[0]["value"] @app.callback( @@ -617,64 +624,45 @@ def set_cities_value(available_options): Input("cities-radio", "value"), ) def set_display_children(selected_country, selected_city): - return "{} is a city in {}".format( - selected_city, - selected_country, - ) - - dash_duo.start_server(app) + # this may actually be the key to this whole test: + # these inputs should never be out of sync. + assert selected_city in all_options[selected_country] - not_null = ( - lambda: dash_duo.find_element("#display-selected-values").get_attribute( - "innerText" - ) - != "" - ) + out_call_count.value += 1 + with out_lock: + return "{} is a city in {}".format( + selected_city, + selected_country, + ) - wait.until(not_null, 20) + dash_duo.start_server(app) new_york_text = "New York City is a city in America" - current_text = dash_duo.find_element("#display-selected-values").get_attribute( - "innerText" - ) - - assert current_text == new_york_text, "{} should equal {}".format( - current_text, new_york_text - ) + canada_text = "Montreal is a city in Canada" - all_inputs = dash_duo.find_elements("input") - - relevant_input = dash_duo.driver.execute_script( - """ - var inp = arguments[0].filter((input)=>{ - console.log(input.parentElement.innerText) - return input.parentElement.innerText=='Canada' - }) - console.log("pringing something", inp) - return inp[0] - """, - all_inputs, - ) + # If we get to the correct initial state with only one call of each callback, + # then there mustn't have been any intermediate changes to the output text + dash_duo.wait_for_text_to_equal("#display-selected-values", new_york_text) + assert opts_call_count.value == 1 + assert city_call_count.value == 1 + assert out_call_count.value == 1 - dash_duo.driver.set_network_conditions( - offline=False, - latency=5, # additional latency (ms) - download_throughput=1 * 1024, # maximal throughput - upload_throughput=1 * 1024, - ) # maximal throughput + all_labels = dash_duo.find_elements("label") + canada_opt = next(i for i in all_labels if i.text == "Canada").find_element("input") - relevant_input.click() + with out_lock: + canada_opt.click() - canada_text = "Montreal is a city in Canada" - expected_text = [new_york_text, canada_text] + # all three callbacks have fired once more, but since we haven't allowed the + # last one to execute, the output hasn't been changed + wait.until(lambda: out_call_count.value == 2, timeout=3) + assert opts_call_count.value == 2 + assert city_call_count.value == 2 + assert dash_duo.find_element("#display-selected-values").text == new_york_text - def wait_cond(): - current_elem = dash_duo.find_element("#display-selected-values").get_attribute( - "innerText" - ) - assert current_elem in expected_text, "{} should be one of {}".format( - current_elem, expected_text - ) - return current_elem == "Montreal is a city in Canada" + dash_duo.wait_for_text_to_equal("#display-selected-values", canada_text) + assert opts_call_count.value == 2 + assert city_call_count.value == 2 + assert out_call_count.value == 2 - wait.until(wait_cond, 20) + assert dash_duo.get_logs() == [] From 5901fcdb84fa146d819ca9ca90012b01f00e3041 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Tue, 6 Dec 2022 18:33:52 -0500 Subject: [PATCH 4/5] fix subselector in cbmt013 --- tests/integration/callbacks/test_multiple_callbacks.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/callbacks/test_multiple_callbacks.py b/tests/integration/callbacks/test_multiple_callbacks.py index 33e37f0dc9..f78d029386 100644 --- a/tests/integration/callbacks/test_multiple_callbacks.py +++ b/tests/integration/callbacks/test_multiple_callbacks.py @@ -648,7 +648,9 @@ def set_display_children(selected_country, selected_city): assert out_call_count.value == 1 all_labels = dash_duo.find_elements("label") - canada_opt = next(i for i in all_labels if i.text == "Canada").find_element("input") + canada_opt = next( + i for i in all_labels if i.text == "Canada" + ).find_element_by_tag_name("input") with out_lock: canada_opt.click() From 2cca167ad51c439291b076fb6b2a64c5bb57a8f8 Mon Sep 17 00:00:00 2001 From: alexcjohnson Date: Wed, 7 Dec 2022 15:19:49 -0500 Subject: [PATCH 5/5] changelog for dependent callback pruning fix --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 96587693ea..25049d7211 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ This project adheres to [Semantic Versioning](https://semver.org/). ### Fixed +- [#2344](https://github.com/plotly/dash/pull/2344) Fix [#1519](https://github.com/plotly/dash/issues/1519), a case where dependent callbacks can be called too many times and with inconsistent inputs - [#2332](https://github.com/plotly/dash/pull/2332) Add key to wrapped children props in list. - [#2336](https://github.com/plotly/dash/pull/2336) Fix inserted dynamic ids in component as props.