mckinsey / vizro

Vizro is a toolkit for creating modular data visualization applications.
https://vizro.readthedocs.io/en/stable/
Apache License 2.0
2.46k stars 109 forks source link

[Feat] Add new dcc.Loading features in dash 2.17 #487

Closed AnnMarieW closed 1 month ago

AnnMarieW commented 1 month ago

Description

Closes #486

This PR is a quick demo of a couple new features in dcc.Loading available in dash 2.17. This is just to make it easier to see how it it might work in Vizro apps.

It adds

I'm interested to hear what you think!

Notice

antonymilne commented 1 month ago

Hello @AnnMarieW! Thanks very much for making the Github issue and raising this PR. And many congratulations on https://github.com/plotly/dash/pull/2760! It's a really amazing PR ⭐

@huong-li-nguyen Following on from the slack conversation and just to make sure we're all on the same page here, because I had to remind myself of some previous work I'd done to stop graphs flickering (https://github.com/mckinsey/vizro/pull/166/) and what our current scheme is... There's currently two times when dcc.Loading is relevant:

  1. when on-page-load action replaces the empty graph (or full datatable/AgGrid as @petar-qb explained) with the "real" one containing filtered data
  2. when you change some parameter/filter data on the screen

So even though overlay_style might not make any difference to graphs in case 1, it may well do so in case 2 if the filter/parameter takes a long time to apply (we can add some time.sleep in to try it out) so I think is probably valuable already.

delay_show I think should also improve things already (again, for non-graph in case 1 and for graph/table/grid components in case 2). So if it's somehow making flickering worse that would be good to understand what's happening.

Just for the record, some other things that might be useful from https://github.com/plotly/dash/pull/2760 which @AnnMarieW didn't change here would be:

  1. the target_components property, if you can remember why that might have been useful to us (I can't) - see our conversation with Ann Marie on dcc.Loading PR on the plotly forums!
  2. display="show"/"hide" might help us get rid of on-page-load as per https://github.com/mckinsey/vizro/pull/439, but this is a longer term thing
huong-li-nguyen commented 1 month ago

@antonymilne, it's good that you mentioned it. The second case was not on my mind anymore 😄 In that case, the overlay_style does work for us!

On delay_show / delay_hide, I would love to get @AnnMarieW's opinion on this. I find it tricky to find the sweet spot of time that works well for us. See PR comments. But maybe some other advantages are not visually visible here?

I've looked into our previous discussion before the PR review and also thought that target_components looks promising! However, I think we always return the entire fig whenever we trigger any dashboard interaction, and we don't know beforehand what users might want to encode as a trigger for custom actions so triggering the dcc.Loading component on any update on the fig is probably the safest. That's why I am not sure if we could leverage this partial triggering of the dcc.Loading component with our current system of actions. @petar-qb—wdyt?

AnnMarieW commented 1 month ago

And regarding the target_components prop - I agree, I don't think it's applicable with Vizro.

petar-qb commented 1 month ago

First of all, @AnnMarieW congrats for the great Dash contribution, and also a big thank you for the Vizro contribution ❤️.

Regarding this PR, I like the overlay_style={"visibility": "visible", "opacity": 0.3}, as the default Vizro figure configuration and really think it has a place in our codebase.

Also, I've tried a lot of delay_show/hide timings combinations and it's quite difficult to find the right timings given we don't know how long the callback will take to run.

So as I understood, these are the goals we want to achieve by utilising the delay_show/hide:

  1. To avoid a figure flickering when the answer comes "immediately" from the server (by utilising the delay_show prop),
  2. To avoid a loading-icon flickering when the answer comes "immediately" after the delay_show time passes (by utilising the delay_hide prop).

So, if delay_show=500, delay_hide=500 props are set and server response time is:

So, the same effect is keep popping up even if I change delay_show/hide number of milliseconds. Because of everything listed above, my opinion is that we shouldn't hardcode the delay_show/hide (because we can't foresee server response time for all the apps created using Vizro) and should let our users to do it on themself (by creating the custom component) with the desired number of delay milliseconds.

What do you think? @AnnMarieW @antonymilne @huong-li-nguyen

AnnMarieW commented 1 month ago

Hi @petar-qb

Yes, you are correct with the objectives of the delay_show and the delay_hide props. The main purpose is to prevent the loading spinner from showing for a short time which can appear as a flash on the screen.

Taking the Viro demo site as an example, currently, the loading spinner is displayed on every user interaction. Since the dataset is small, the spinner is displayed for less than one second in most cases.

Adding delay_show of 500ms makes the spinner show up less than half the time (when I run the app locally). However, if the callback takes slightly longer, let's say 700ms, then the spinner is back to having the flashing effect because it will be visible for 200ms.

Adding the delay_hide can prevent the flash, because you can specify the minimum amount of time to display the spinner once it becomes visible. However, the delay_hide prop looks the best when the underlying components are not visible while loading (the default) to handle the case described above where the component might be updated before the delay_hide time has elapsed.

It is tricky to get right, and it also depends a lot on the app and the preferences of the users. I agree that it might be best to let people add it as a custom component. Would you like me to close this PR now?

huong-li-nguyen commented 1 month ago

Hey @AnnMarieW ,

I've talked with both @antonymilne and @petar-qb. You can still go ahead with the PR and add the overlay_style={"visibility": "visible", "opacity": 0.3} :)

As Petar said, let's not define delay_show/delay_hide for now 👍

AnnMarieW commented 1 month ago

Hey everyone,

I think adding the opacity is a nice addition and will make the transition between "loading" and "ready" smoother. However, there is still a flicker when the loading time is very short.

Could you please try one more thing before closing this? I just tried setting the delay_show=200 and I think it looks great! Check out the demo on the Benchmark Analysis page where you click on the grid and it updates the graph. This setting completely eliminates the flicker. And 200ms short enough that I don't think it will have the same problem of shifting the flicker as noted earlier.

And of course if you prefer not to add this, I'm still delighted about the addition of the new loading overlay style :slightly_smiling_face:

petar-qb commented 1 month ago

Hey everyone,

I think adding the opacity is a nice addition and will make the transition between "loading" and "ready" smoother. However, there is still a flicker when the loading time is very short.

Could you please try one more thing before closing this? I just tried setting the delay_show=200 and I think it looks great! Check out the demo on the Benchmark Analysis page where you click on the grid and it updates the graph. This setting completely eliminates the flicker. And 200ms short enough that I don't think it will have the same problem of shifting the flicker as noted earlier.

And of course if you prefer not to add this, I'm still delighted about the addition of the new loading overlay style 🙂

Hey @AnnMarieW.

I tried it out and got that the "initial page load" flickers equally as it flickers without setting the delay_show=200. Also, there is no loading icon (and no flickering) while applying filters/parameters. However, it works better only when you run the Vizro demo app locally, and we can't guarantee how all other users examples will work. So, I put time.sleep(0.15) inside the filtering method and found that the app flickered even more.

Since we don't know the performance of users apps built using Vizro (since it depends on many factors), I wouldn't hardcode any delay, and just push the overlay argument.