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

fix for callbacks waiting issue #1627

Closed
wants to merge 7 commits into from

Conversation

eff-kay
Copy link
Contributor

@eff-kay eff-kay commented May 7, 2021

This is a potential fix for 1519, contributed only for discussion purposes. Not the final fix. The following checklist is not relevant for now

Contributor Checklist

  • I have broken down my PR scope into the following TODO tasks
    • task 1
    • task 2
  • 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

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

@Marc-Andre-Rivet
Copy link
Contributor

Marc-Andre-Rivet commented May 10, 2021

The behavior described is indeed what is happening 😞. I was initially surprised at the behavior as it is pretty clear that the display children callback should wait on citites values update from the cities options callback before executing.

For the immediate scenario in the issue: https://github.com/plotly/dash/blob/dev/dash-renderer/src/observers/requestedCallbacks.ts#L234-L237 is too eager at declaring callbacks ready as it doesn’t check for subsequent calls a callback may trigger down the chain (

export const getReadyCallbacks = (
paths: any,
candidates: ICallback[],
callbacks: ICallback[] = candidates
): ICallback[] => {
// Skip if there's no candidates
if (!candidates.length) {
return [];
}
// Find all outputs of all active callbacks
const outputs = map(
combineIdAndProp,
reduce<ICallback, any[]>(
(o, cb) => concat(o, flatten(cb.getOutputs(paths))),
[],
callbacks
)
);
// Make `outputs` hash table for faster access
const outputsMap: {[key: string]: boolean} = {};
forEach(output => (outputsMap[output] = true), outputs);
// 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(
cb =>
all<ILayoutCallbackProperty>(
cbp => !outputsMap[combineIdAndProp(cbp)],
difference(
flatten(cb.getInputs(paths)),
flatten(cb.getOutputs(paths))
)
),
candidates
);
};
) .

It seems to me the issue could be handled by ensuring that all subsequent callbacks of country values are guaranteed not to cause a change in an input displayed children uses instead of just checking against active callbacks — the logic to access this information already exists for the prioritized -> executing prioritization scheme which involves the depth of the callback tree for a given callback. Repurposing the logic of

export function getPriority(
to extract all affected props in a callback’s tree.

If a prop change from a callback triggers a component initiated prop change it’s true the renderer can’t be aware of it. I agree there no clear way to solve this problem unless we make the renderer aware of these internal component links, which doesn’t seem desirable. In a way what we are saying with the current behavior is that user initiated changes and component initiated changes are equivalent — both are callbacks triggered by the UI are it’s as impossible to predict user interactions as it is to predict component interactions.

image

If I understand the proposed fix correctly, https://github.com/plotly/dash/pull/1627/files#diff-9ef142ebb62ec253df4d4611f96d54124ca69347d9d27fd78c5ae6e1202f5ef4R350 would remove redundant callbacks from the initially triggered callbacks before ever requesting them? If that’s the case I don’t think this fix can work as there is no guarantee that, in this scenario, cities options will actually trigger a change (PreventUpdate, no_update) and that would prevent diplayed children from being updated altogether.

Simplest description of the callbacks DAG I guess is that callback chains are “maybe connected” with each arrow in the screenshots above not representing a connection but rather 0 or 1 connection - and that is resolved at runtime with each individual callback execution — pruning on user interaction assumes 1 connection and hence can’t work.

@eff-kay
Copy link
Contributor Author

eff-kay commented Jun 17, 2021

I have pushed the new changes that resolves the defect. In summary I am updating the getReadyCallbacks function.

https://github.com/plotly/dash/pull/1627/files#diff-9ef142ebb62ec253df4d4611f96d54124ca69347d9d27fd78c5ae6e1202f5ef4R249

The important changeset is the if block that starts from 281. Basically, now I am looking at all of the outputs potentially touched by the active callbacks and populating the outputMap from that.

@eff-kay
Copy link
Contributor Author

eff-kay commented Jun 17, 2021

I have added a test that I think would actually validate the issue reported. Let me know if you think of a scenario in which this would pass without the fix.

https://github.com/plotly/dash/pull/1627/files#diff-d94c2be667bd3581c7fbb922b85c16d0ce1ad2bdc25a47444c9eaf5650421b85R590

@eff-kay eff-kay changed the title 1519 callacks waiting fix [draft] 1519 callacks waiting fix May 6, 2022
@eff-kay eff-kay mentioned this pull request Nov 30, 2022
9 tasks
@gvwilson gvwilson self-assigned this Jul 23, 2024
@gvwilson gvwilson removed their assignment Aug 2, 2024
@gvwilson gvwilson changed the title 1519 callacks waiting fix fix for callbacks waiting issue Aug 13, 2024
@gvwilson gvwilson added P2 considered for next cycle fix fixes something broken community community contribution labels Aug 13, 2024
@ndrezn
Copy link
Member

ndrezn commented Aug 22, 2024

This PR was superseded by: #2344, which was released.

@ndrezn ndrezn closed this Aug 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution fix fixes something broken P2 considered for next cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants