[Feat] Implement AgGrid model #289
Conversation
maxschulz-COL
added
Status: Ready for Review ☑️
and removed
Stage: Technical Design 🎨
labels
maxschulz-COL
requested review from
Joseph-Perkins,
antonymilne,
huong-li-nguyen and
Anna-Xiong
as code owners
for more information, see https://pre-commit.ci
| def _filter_interaction( | ||
| self, data_frame: pd.DataFrame, target: str, ctd_filter_interaction: Dict[str, CallbackTriggerDict] | ||
| ) -> pd.DataFrame: | ||
| """Function to be carried out for pre-defined `filter_interaction`.""" | ||
| ctd_click_data = ctd_filter_interaction["clickData"] | ||
| if not ctd_click_data["value"]: | ||
| return data_frame | ||
|
|
||
| source_graph_id: ModelID = ctd_click_data["id"] | ||
| source_graph_actions = _get_component_actions(model_manager[source_graph_id]) | ||
| try: | ||
| custom_data_columns = model_manager[source_graph_id]["custom_data"] | ||
| except KeyError as exc: | ||
| raise KeyError(f"No `custom_data` argument found for source graph with id {source_graph_id}.") from exc | ||
|
|
||
| customdata = ctd_click_data["value"]["points"][0]["customdata"] | ||
|
|
||
| for action in source_graph_actions: | ||
| if action.function._function.__name__ != "filter_interaction" or target not in action.function["targets"]: | ||
| continue | ||
| for custom_data_idx, column in enumerate(custom_data_columns): | ||
| data_frame = data_frame[data_frame[column].isin([customdata[custom_data_idx]])] | ||
|
|
||
| return data_frame |
There was a problem hiding this comment.
It looks like _filter_interaction method is pretty similar in Graph/Table/Grid model. What do you think about the idea of implementing _extract_filtering_info method per model instead (e.g. it can just return real selected value and maybe the column that has to be filtered), and conduct filtering inside the _actions_utils._apply_filter_interaction?
There was a problem hiding this comment.
That is an interesting idea! I think we should definitely discuss it. I guess the pros are:
- simplified and shortened code in the respective models
- avoiding repretition
The cons I can think of are:
- potentially wrong abstraction level given that e.g. the graph currently filters multiple columns, which the table/grid don't
- like this we are giving the user less control over the filtering
Open to both really :)
There was a problem hiding this comment.
I agree that it is not easy to make this kind of abstraction in general, hence I call it - "actions_info" - the biggest open question we currently have.
If we make this kind of abstraction properly, we could say that the "filter_interaction" is just the same as the "_filter" action, which sounds perfectly correct to me.
From top of my head: All "actions_info" we need to set for every Vizro model (e.g. Button, Card, Dropdown, Graph, Grid...) is:
_get_input_components- e.g. {model_id: State(...), derived_viewport_data: State(...), active_cell: State(...)} OR {model_id: State(...), value: State(...)};_extract_value_from_input- somefunction,_get_output_components- e.g. {model_id: Output(...)}
It means that every Vizro model will have implemented these methods in a form of (class methods, or some actions_info: Dict[str, any] argument). Also, these methods/arguments could be overwritten in case of custom component creation.
There was a problem hiding this comment.
I would suggest we leave it as is for now and come back to it later. This is still a big open question indeed, and I think "Duplication is cheaper than the wrong abstraction" applies in this case.
Generally I think I agree with @petar-qb's idea, but there's still a lot of moving pieces here so hard to tell right now what the right solution is. So for now I think just focus on:
- getting these models working correctly
- initial actions v2 work to make those action
CapturedCallableclasses
And after that we can figure out what to do with all this actions info stuff.
Let's unresolve this conversation after merging though so we can easily refer back to it.
There was a problem hiding this comment.
Generally looks good, although I didn't actually play around with using it yet 👍 Thanks for all the work on different versions of this, I know it's not been an easy task!
I left some initial comments so you don't wait too long for a review, but I don't think I will have much more to say after this round anyway, so long as we're confident it works ok.
What happens if a user does AgGrid(figure=dash_data_table(...) or Table(figure=dash_ag_grid(...)? I guess it renders and all works well apart from the filter interaction ?
| @_log_call | ||
| def pre_build(self): | ||
| if self.actions: | ||
| kwargs = self.figure._arguments.copy() | ||
|
|
||
| # This workaround is needed because the underlying table object requires a data_frame | ||
| kwargs["data_frame"] = DataFrame() | ||
| kwargs["data_frame"] = pd.DataFrame() | ||
|
|
||
| # The underlying table object is pre-built, so we can fetch its ID. | ||
| underlying_table_object = self.figure._function(**kwargs) |
There was a problem hiding this comment.
Not related to this PR, but @petar-qb do you remember we had a conversation a while ago about trying to simplify all this stuff? I think we should try and do that still, especially now it's duplicated for AGGrid and Table.
There was a problem hiding this comment.
I really can't remember what we talked about exactly..
At least we can extract the implementation of this method and reuse it in the vm.AgGrid and vm.Table.
Also, I think we should get rid of "if self.actions:" statement in pre_build method and always conduct fetching of underlying id. In that way we can avoid raising a dash exception (something like: "Input of callback X doesn't exist."). This exception will be raised:
- if user provides an ID for the underlying table,
- if the underlying table is an input (dash.State) of the custom action,
- if there is no "
actions" defined for the vm.Table / vm.AgGrid
We should double check this assumption and if it's true we should also change one line in the build method.
From:
dash_table.DataTable(**({"id": self._callable_object_id} if self.actions else {})), id=self.id
To:
dash_table.DataTable(id=self._callable_object_id), id=self.id
There was a problem hiding this comment.
My idea was that we can remove basically all of this, like so:
Index: vizro-core/src/vizro/models/_components/table.py
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vizro-core/src/vizro/models/_components/table.py b/vizro-core/src/vizro/models/_components/table.py
--- a/vizro-core/src/vizro/models/_components/table.py (revision 4ea5b5f35cf9db4217537038d4f36ed4e8fc3ee4)
+++ b/vizro-core/src/vizro/models/_components/table.py (date 1705597769502)
@@ -48,7 +48,9 @@
# Convenience wrapper/syntactic sugar.
def __call__(self, **kwargs):
kwargs.setdefault("data_frame", data_manager._get_component_data(self.id))
- return self.figure(**kwargs)
+ figure = self.figure(**kwargs)
+ figure.id = self._callable_object_id
+ return figure
# Convenience wrapper/syntactic sugar.
def __getitem__(self, arg_name: str):
@@ -60,31 +62,14 @@
@_log_call
def pre_build(self):
- if self.actions:
- kwargs = self.figure._arguments.copy()
-
- # This workaround is needed because the underlying table object requires a data_frame
- kwargs["data_frame"] = DataFrame()
-
- # The underlying table object is pre-built, so we can fetch its ID.
- underlying_table_object = self.figure._function(**kwargs)
-
- if not hasattr(underlying_table_object, "id"):
- raise ValueError(
- "Underlying `Table` callable has no attribute 'id'. To enable actions triggered by the `Table`"
- " a valid 'id' has to be provided to the `Table` callable."
- )
-
- self._callable_object_id = underlying_table_object.id
+ self._callable_object_id = self.id + "***"
def build(self):
return dcc.Loading(
html.Div(
[
html.H3(self.title, className="table-title") if self.title else None,
- html.Div(
- dash_table.DataTable(**({"id": self._callable_object_id} if self.actions else {})), id=self.id
- ),
+ html.Div(dash_table.DataTable(id=self._callable_object_id), id=self.id),
],
className="table-container",
id=f"{self.id}_outer",
Definitely not for this PR, but just wanted to make sure we didn't forget it.
There was a problem hiding this comment.
Ah yes, thanks for remembering 😄. This change really reduces the complexity of the table model ⚡.
There was a problem hiding this comment.
Ok will keep this un-resolved but will not implement in this PR
| def _filter_interaction( | ||
| self, data_frame: pd.DataFrame, target: str, ctd_filter_interaction: Dict[str, CallbackTriggerDict] | ||
| ) -> pd.DataFrame: | ||
| """Function to be carried out for pre-defined `filter_interaction`.""" | ||
| ctd_click_data = ctd_filter_interaction["clickData"] | ||
| if not ctd_click_data["value"]: | ||
| return data_frame | ||
|
|
||
| source_graph_id: ModelID = ctd_click_data["id"] | ||
| source_graph_actions = _get_component_actions(model_manager[source_graph_id]) | ||
| try: | ||
| custom_data_columns = model_manager[source_graph_id]["custom_data"] | ||
| except KeyError as exc: | ||
| raise KeyError(f"No `custom_data` argument found for source graph with id {source_graph_id}.") from exc | ||
|
|
||
| customdata = ctd_click_data["value"]["points"][0]["customdata"] | ||
|
|
||
| for action in source_graph_actions: | ||
| if action.function._function.__name__ != "filter_interaction" or target not in action.function["targets"]: | ||
| continue | ||
| for custom_data_idx, column in enumerate(custom_data_columns): | ||
| data_frame = data_frame[data_frame[column].isin([customdata[custom_data_idx]])] | ||
|
|
||
| return data_frame |
There was a problem hiding this comment.
I would suggest we leave it as is for now and come back to it later. This is still a big open question indeed, and I think "Duplication is cheaper than the wrong abstraction" applies in this case.
Generally I think I agree with @petar-qb's idea, but there's still a lot of moving pieces here so hard to tell right now what the right solution is. So for now I think just focus on:
- getting these models working correctly
- initial actions v2 work to make those action
CapturedCallableclasses
And after that we can figure out what to do with all this actions info stuff.
Let's unresolve this conversation after merging though so we can easily refer back to it.
There was a problem hiding this comment.
I won't review this in detail as others already do :)
As long as I remember - it would be amazing ⭐ if you could add the AG-grid example inside the features dashboard and add the yaml version of it (if applicable) as well. You can also replace the data table inside the demo dashboard with the ag-grid one already👍
I'll leave that up to you though, whether you want to add it in this PR or your docs PR.
maxschulz-COL
changed the title
vizro-core/tests/unit/vizro/actions/test_filter_interaction_action.py
Outdated
Show resolved
Hide resolved
huong-li-nguyen
changed the title
maxschulz-COL
changed the title
|
The initial bug is solved with the latest changes ec85e0c you pushed. Awesome work Max! 🤖 |
| # The pagination setting (and potentially others) only work when the initially built AgGrid has the same | ||
| # setting as the object that is built on-page-load and rendered finally. | ||
| dash_ag_grid_conf = self.figure._arguments.copy() | ||
| dash_ag_grid_conf["data_frame"] = pd.DataFrame() | ||
| if hasattr(self, "_callable_object_id"): | ||
| dash_ag_grid_conf["id"] = self._callable_object_id |
There was a problem hiding this comment.
This is very clean solution! I just think that the following two lines:
if hasattr(self, "_callable_object_id"):
dash_ag_grid_conf["id"] = self._callable_object_id
don't make any difference as the id is already copied to the dash_ag_grid_conf in the previous lines.
Also, it seems like it makes sense to push similar approach for the vm.Table - dash_data_table solution.
Description
This PR is an iteration of discussion in #260. Here we decided to implement one model per
Callabletype.31.0.0Styling, docs and tests will be in separate PRs.
Notable activated features
Cell formatting
dollar,euro,percent,numeric- see third page of dev example and Screenshotpd.datetimecolumn into a validdateString(required by AGGrid to function for dates out of the box)Screenshot
Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":