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

[Bug] Enable that components can be nested inside basic type structures too #929

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
<!--
A new scriv changelog fragment.

Uncomment the section that is right (remove the HTML comment wrapper).
-->

<!--
### Highlights ✨

- A bullet item for the Highlights ✨ category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX. ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
<!--
### Removed

- A bullet item for the Removed category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX. ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
<!--
### Added

- A bullet item for the Added category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX. ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
<!--
### Changed

- A bullet item for the Changed category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX. ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
<!--
### Deprecated

- A bullet item for the Deprecated category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX. ([#1](https://github.com/mckinsey/vizro/pull/1))

-->

### Fixed

- Enable that custom components can be nested arbitrarily deep inside basic type structures (e.g. lists within lists), and not just specific attributes. ([#929](https://github.com/mckinsey/vizro/pull/929))
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rephrase to below? I tried to simplify a bit

Suggested change
- Enable that custom components can be nested arbitrarily deep inside basic type structures (e.g. lists within lists), and not just specific attributes. ([#929](https://github.com/mckinsey/vizro/pull/929))
- Enable arbitrary deep nesting of custom components. All models can now be nested arbitrarily deep within lists. ([#929](https://github.com/mckinsey/vizro/pull/929))



<!--
### Security

- A bullet item for the Security category with a link to the relevant PR at the end of your entry, e.g. Enable feature XXX. ([#1](https://github.com/mckinsey/vizro/pull/1))

-->
76 changes: 57 additions & 19 deletions vizro-core/examples/scratch_dev/app.py
Original file line number Diff line number Diff line change
@@ -1,31 +1,69 @@
"""Dev app to try things out."""

from vizro import Vizro
import vizro.plotly.express as px
import vizro.models as vm
from vizro.tables import dash_ag_grid
import pandas as pd
import vizro.models as vm
import vizro.plotly.express as px
from vizro import Vizro
from typing import List, Literal, Tuple
from vizro.models.types import ControlType
from dash import html

df = pd.read_csv("https://raw.githubusercontent.com/plotly/datasets/master/ag-grid/olympic-winners.csv")
columnDefs = [
{"field": "athlete", "headerName": "The full Name of the athlete"},
{"field": "age", "headerName": "The number of Years since the athlete was born"},
{"field": "country", "headerName": "The Country the athlete was born in"},
{"field": "sport", "headerName": "The Sport the athlete participated in"},
{"field": "total", "headerName": "The Total number of medals won by the athlete"},
]

defaultColDef = {
"wrapHeaderText": True,
"autoHeaderHeight": True,
}
class CustomGroup(vm.VizroBaseModel):
"""Container to group controls."""

type: Literal["custom_group"] = "custom_group"
controls: List[Tuple[str, List[ControlType]]] = [[]]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI my preferred pattern for this would be to nest a list of models, each of which does this (a bit like tabs):

    title: str
    controls: list[ControlType] = []

Or if you want just one model then do it like this:

    controls: dict[str, list[ControlType]] = {}

As it stands we made the deliberate choice in vizro to avoid tuple. We also haven't used the dict[str, VizroBaseModel] pattern anywhere but that might change in future.


def build(self):
return html.Div(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing, how this works now! 🥳 🍾

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below but crucially this only works because controls is one of the fields we search through (see below comment, please check if I'm right @petar-qb).

In future that won't be a limitation but for now it's ok.

children=[
html.Div(
children=[html.Br(), html.H5(control_tuple[0]), *[control.build() for control in control_tuple[1]]],
)
for control_tuple in self.controls
]
)


vm.Page.add_type("controls", CustomGroup)


# Test app -----------------
page = vm.Page(
title="Page Title",
components=[vm.AgGrid(figure=dash_ag_grid(df, columnDefs=columnDefs, defaultColDef=defaultColDef))],
title="Title",
components=[
vm.Graph(id="graph_id", figure=px.scatter(px.data.iris(), x="sepal_width", y="sepal_length", color="species")),
],
controls=[
CustomGroup(
controls=[
(
"Categorical Filters",
[
vm.Filter(column="species"),
],
),
(
"Numeric Filters",
[
vm.Filter(column="petal_length"),
vm.Filter(column="sepal_length"),
],
),
],
),
vm.Parameter(
targets=["graph_id.x"],
selector=vm.RadioItems(
title="Select X Axis",
options=["sepal_width", "sepal_length", "petal_width", "petal_length"],
value="sepal_width",
),
),
],
)


dashboard = vm.Dashboard(pages=[page])

if __name__ == "__main__":
Expand Down
19 changes: 8 additions & 11 deletions vizro-core/src/vizro/managers/_model_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,28 +84,25 @@ def _get_models(
def __get_model_children(self, model: Model) -> Generator[Model, None, None]:
"""Iterates through children of `model`.

Currently looks only through certain fields so might miss some children models.
Currently, this method looks only through certain fields so might miss some children models.
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen Dec 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Currently, this method looks only through certain fields so might miss some children models.
Currently, this method looks only through certain fields so might miss some children models.

What does that mean that it looks "through certain fields"? I thought it's independent of "components", "tabs", "controls", "actions", "selector" now. Or does it refer to something else? If I understood correctly, it just misses children nested inside dict, right? If that's correct, then could we update the docstring here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Currently, this method looks only through certain fields so might miss some children models.
Currently, this method looks only through certain fields (components, tabs, controls, actions, selector) and their children so might miss some children models.

It's still not independent of the fields now - unless I missed something the PR description is actually wrong when it says it works with my_custom_components_property: list[list[tuple[vm.VizroBaseModel]]].

What has actually changed is:

  • before this PR: field_name: vm.VizroBaseModel and field_name: list[vm.VizroBaseModel] worked where field_name is one of (components, tabs, controls, actions, selector)
  • after this PR: in addition to the above, field_name: list[list[vm.VizroBaseModel]] also works but with the same constraint on field_name. If @petar-qb thinks my suggestion is good then field_name: dict[str, vm.VizroBaseModel] would also work

"""
from vizro.models import VizroBaseModel

if isinstance(model, VizroBaseModel):
yield model

# We don't handle dicts of models at the moment. See below TO-DOs for how this will all be improved in future.
if isinstance(model, (list, tuple)):
for single_model in model:
yield from self.__get_model_children(single_model)

Comment on lines 91 to +98
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some proposed changes:

  • if changed to elif to make it clearer
  • we don't use tuple anywhere in vizro and I think shouldn't encourage it for custom components either
  • handling dictionaries is actually easy I think? But I didn't test it at all
Suggested change
if isinstance(model, VizroBaseModel):
yield model
# We don't handle dicts of models at the moment. See below TO-DOs for how this will all be improved in future.
if isinstance(model, (list, tuple)):
for single_model in model:
yield from self.__get_model_children(single_model)
if isinstance(model, VizroBaseModel):
yield model
elif isinstance(model, list):
for single_model in model:
yield from self.__get_model_children(single_model)
elif isinstance(model, dict):
# We don't look through keys because Vizro models aren't hashable.
# Currently we don't have Vizro models as dictionary values anywhere but maybe we will in the future (or through a custom component).
for single_model in model.values():
yield from self.__get_model_children(single_model)

# TODO: in future this list should not be maintained manually. Instead we should look through all model children
# by looking at model.model_fields.
# by looking at model.model_fields.
model_fields = ["components", "tabs", "controls", "actions", "selector"]

for model_field in model_fields:
if (model_field_value := getattr(model, model_field, None)) is not None:
if isinstance(model_field_value, list):
# For fields like components that are list of models.
for single_model_field_value in model_field_value:
yield from self.__get_model_children(single_model_field_value)
else:
# For fields that have single model like selector.
yield from self.__get_model_children(model_field_value)
# We don't handle dicts of models at the moment. See below TODO for how this will all be improved in
# future.
yield from self.__get_model_children(model_field_value)

# TODO: Add navigation, accordions and other page objects. Won't be needed once have made whole model
# manager work better recursively and have better ways to navigate the hierarchy. In pydantic v2 this would use
Expand Down
Loading