-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Enable storing the callback outputs in the persistence storage #3144
base: dev
Are you sure you want to change the base?
Conversation
…tputs in the persistence storage
Every time I make changes to the dash-rendered code locally, I have to run |
Is |
I see this more as a bug fix rather than a new feature. If you agree, we should then default the
@self.callback(
Output(_ID_CONTENT, "children"),
Output(_ID_STORE, "data"),
inputs=inputs,
prevent_initial_call=True,
enable_persistence=False
)
def update(pathname_, search_, **states): What are your thoughts? |
I would regard this as a bug fix. That being said I don't see why it needs to be enabled/disabled as a flag; is there an argument against having this be the default behaviour? |
Hey @ndrezn 👋 The current ( Current Code:
New Code:
This means there’s no change for existing users unless they explicitly opt in by setting Why keep the flag?If we remove
Thus, keeping Would love to hear your thoughts!
Edit: Sketches from the PR description are updated. |
Awesome PR @petar-qb! I know how much work has gone into getting to the bottom of this over the last few months! Just for context @ndrezn: we had a phone call with @T4rk1n and @gvwilson last year to discuss this. It was actually @gvwilson's idea to release this with a flag, even if it's regarded as a bug fix, to ensure that any apps that rely on the current behaviour could revert to it. He said that you guys would be able to test the changes against a suite of representative Dash apps that would tell us whether it's likely to break anyone's app or not. Personally I agree with you and would just consider this a bug fix and not put a flag in at all for it. The cases where someone was actually relying on the old behaviour seem so edge to me that I can't imagine anyway is deliberately using that behaviour. Edit: this is not quite correct - we might need the flag after all. See next comment... |
I just had a good chat with @petar-qb who explained things some more (every time I discuss this I get confused, there's so many different cases to consider, sometimes a face to face chat is just much easier 😅 And Petar has really spent A LOT of time figuring all of them out). What I said above about not exposing the flag is not correct due to the special status of the inbuilt Dash pages Basically, AIUI, it doesn't make any sense for that Dash pages callback to have So to ensure that the Dash pages
Did I get that right @petar-qb? |
@T4rk1n please have a look at this one and let us know if it can go into Dash 3.0 without disrupting things or whether we should target 3.1. thanks (and thanks to @antonymilne and @petar-qb for their hard work) - @gvwilson |
You're absolutely right! Both of your callback configuration API suggestions work for me, and I'm curious to hear what the Dash team thinks. P.S. I've updated the sketches of the current and new flowchart to better illustrate how this behaves. |
@T4rk1n please have another look at this and give @petar-qb and @antonymilne some feedback when you can. |
I think it would be better without a flag, I always thought it was weird that persistence didn't apply for the callback return value. I seem to remember @alexcjohnson saying it was done this way for a reason but I can't remember why? I would treat this as bugfix, get in dev for the final 2.18.3 (if any) and 3.1 release. |
@@ -132,8 +131,7 @@ class BaseTreeContainer extends Component { | |||
} | |||
|
|||
setProps(newProps) { | |||
const {_dashprivate_dispatch, _dashprivate_path, _dashprivate_layout} = | |||
this.props; | |||
const {_dashprivate_dispatch, _dashprivate_path} = this.props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fix only for < 3.0
My original argument was:
But @antonymilne shows some good use cases where this doesn't work, and I'm not finding cases where allowing these to persist would cause problems, so I've come around to agreeing it should just always behave this way, no flag needed. |
Thanks very much for weighing in @alexcjohnson! Now we are all agreed that we can change this behaviour everywhere and don't need a flag for users to enable/disable it. However, this does still leave a question of how to handle the Dash pages
So if we don't expose a flag to users then we should go for the second solution here and put in a manual exception for the Dash pages callback. What would be the best way to do this? |
You mean it should treat the page callback as the layout and don't apply persistence for that one? The only thing that comes to mind is a heavy refactor of the page callback logic to be inside the layout route instead of the callback, but that would be way much change. |
Hey team @T4rk1n @gvwilson @ndrezn @antonymilne 👋 I just pushed the 1a3173e that implements the bugfix solution without the TLDR:
N.B. Distinguishment between the internal |
…enable_persistence
); | ||
let props = updatedProps; | ||
|
||
if (id === '_pages_content') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be the opposite id !== '_pages_content'
for it to record apply the persistence?
In any case the comments here are now confusing, might be worth rewriting them with the actual logic of what it does here and how it get there.
Hey team 👋 @T4rk1n @gvwilson @ndrezn @antonymilne here's an update for you. The current situation with this PR is that it's implemented like a There are 13 CI failing tests, and all of them fail because they contain a callback that
I created another PR #3173 that solves the same issue but with an additional optional callback flag called To continue with the contribution I would need the input from you. Do you think this should be implemented as a Here's TLDR section about these two approaches:
P.S. Both approaches solve the bug when a callback directly |
@petar-qb could you give an example of a practical type of app that wouldn't be possible to build any longer if this was implemented without a flag? I'm always hesitant to add & maintain more API unless we know that there's a specific set of use cases. It'll be hard for most developers to grok what the functional difference is whether the flag is enabled or not and it likely would remain mostly untouched. Knowing a couple example situations where you might want to disable this behaviour would be useful to decide here. |
Hey @ndrezn 👋
TLDR: Implementation of the custom server-side tabs functionality. So, if I update Here's the more detailed break-through. There are 4 different solutions/configurations:
All these four solutions share the two same principles:
There are three more ambiguous cases which results differ between these four aforementioned solutions/configurations: Legend:
Now let's deep dive into the table.
So, everyone easily agrees that the For The only question is: Should Dash users have the ability to explicitly configure custom callback that "recreates" a component to ensure that the server-returned value always takes precedence over the persistence storage and saves this result in the persistence storage? So, should 🟡,❌ -> ✅ be enabled on demand? The theoretical reasons behind this could be: Here's the practical example too: Why it matters that the newest values are persisted in the storage?? Because if I switch to another tab and then get back, I really want to see the latest changes without a need to run the long "Fetch the latest" job. So, the only unclear case is when a callback "recreates" a component. Should users be able to explicitly set the returned server component to take precedence and overwrite the persistence storage, just like when a callback "updates" the component’s value? Thanks for your patience and sorry for that long comment.. |
Fixes #2678
This PR:
TLDR:
This PR adds a callback's property
enable_persistence: bool
(by defaultFalse
). If it'sTrue
, callback output value will be written within the browser's persistence storage for itsdcc
components that haspersistence=True
set.For the following code (also posted in the Dash Plotly forum question that's linked above):
There are screen recordings of how it works from the
plotly/dash::dev
branch vs thepetar-qb/dash::feature/callback_enable_persistence
branch:How it works from the
plotly/dash::dev
(it works exactly the same from the feature branch too if the callbackenable_persistence
is not set or is set toFalse
):Screen.Recording.2025-01-30.at.11.07.24.mov
How it works from the
petar-qb/dash::feature/callback_enable_persistence
whenenable_persistence=True
is added :Screen.Recording.2025-01-30.at.11.09.25.mov
There's a sketch that represents the current behaviour:

There's a sketch that represents the new behaviour:

Contributor Checklist
I have broken down my PR scope into the following TODO tasks
enable_persistence
into the callback and clientside_callback signature (by default it'sFalse
).recordUiEdit
even on callback response.enable_persistence=True
,enable_persistence=True
,updateProps
that will callrecordUiEdit
only ifenable_persistence=True
recordUiEdit
can recursively record edits for children. This enables that setting the persistence from the callback output works even if nested object is returned.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
CHANGELOG.md