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

[Tidy] KPI Infinite scroll AgGrid #519

Open petar-qb opened 3 weeks ago

petar-qb commented 3 weeks ago

Description

FYI:

Additionally:

Open Question:

Screenshot

Notice

maxschulz-COL commented 3 weeks ago

Hey @petar-qb - thanks for the quick investigation! πŸš€

I would like to keep this PR open as a draft for discussion when @antonymilne is back, but I wouldn't like to merge it into the kpi dashboard if this can only be implemented when we turn off filtering and sorting.

The page with the ag-grid doesn't have any controls on purpose, as it looks nicer if the table takes up all space and people don't have to switch between the side panel and main panel. But that's why we do need the filtering and sorting functionality on that table. I would prefer the slower speed over not having filtering/sorting enabled in this case.

I am sure you have looked at this @petar-qb, but just in case you had not seen: https://dash.plotly.com/dash-ag-grid/infinite-scroll#infinite-scroll-with-sort-and-filter

Would this work?

petar-qb commented 3 weeks ago

I am sure you have looked at this @petar-qb, but just in case you had not seen: https://dash.plotly.com/dash-ag-grid/infinite-scroll#infinite-scroll-with-sort-and-filter

Would this work?

@maxschulz-COL Thanks for pointing this out (I forgot even though I saw it). I don't know how good idea it is to push this code for the demo example (since its complexity could confuse our users and repeal them to use Vizro).

However, let me investigate how generic this code is on Monday. If it is, maybe we can implicitly enable this code if the infinite scroll is turned on.

petar-qb commented 2 weeks ago

@maxschulz-COL @huong-li-nguyen @antonymilne

The code from the link works πŸŽ‰.

What do you think, do we want to predefined the infinite_scroll_dash_ag_grid?

maxschulz-COL commented 2 weeks ago

@maxschulz-COL @huong-li-nguyen @antonymilne

The code from the link works πŸŽ‰.

What do you think, do we want to predefined the infinite_scroll_dash_ag_grid?

I am not opposed to having another grid callable, it would be in line with what we are doing for KPI cards.

But I think we need to abstract the entire callback logic away. Ideally we also have only a minimum set of defaults.

Another route would be to enhance the existing callable for the case that someone chooses infinite scroll: e.g. we could make this an argument of the dash_ag_grid and when chosen True, we would take care of the rest. Not sure I like it, but it would reduce the number of CapturedCallable we produce, but could lead to other maintenance issue.

Overall I think I am in favour if we can prove that this setting improves the performance noticeably. If we can enable it in an easy way, then I think this is exactly what Vizro stands for

antonymilne commented 3 days ago

Sorry for the very delayed response to this but hopefully better late than never... 🀞 Just to check we're all on the same page here, I think this is where things stand:

I think @maxschulz-COL was describing above two possible APIs:

  1. keep our dash_ag_grid with client-side row model, no callbacks + new dash_ag_grid_infinite with infinite row model and callback
  2. keep our dash_ag_grid with client-side row model, no callbacks. Intercept the argument rowModelType="infinite" and use that to make it infinite row model and callback

In theory I agree with either API, but I'm not sure how easily we can actually implement either one? The main problem I see here is where do you define the callback?

  1. if it's outside dash_ag_grid then you don't have correct access to data_frame and so can't get the correct data (after filters/parameters on control panel). If you don't have any controls then it's possible to do a hack here to just do data_manager["iris"].load() directly in the function but not a good general solution
  2. if it's inside the dash_ag_grid then will this actually work? We would be redefining a callback every time the function is run, which might work ok but doesn't feel right[^1]. Possibly there's a way to avoid this and look to see if the callback is already defined though to avoid duplication? πŸ€”

Am I missing something here? If not then next steps here would be to investigate 2, which would be useful anyway due to https://github.com/mckinsey/vizro/issues/109.

[^1]: we already do this every Dash page navigation callback since this rebuilds the page and hence e.g. the rangeslider callbacks. This already feels quite wrong but seems to work ok. See also https://github.com/mckinsey/vizro/issues/109#issuecomment-1768715486)

antonymilne commented 3 days ago

The other thing I'm curious about and maybe @AnnMarieW can answer... Where does all the code in this example come from? How well tested and generic is this? i.e. could we safely apply it to any dataframe, or is it really just suitable for a few representative examples?