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 nested 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

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 and tuples. ([#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]]] = [[]]

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! 🥳 🍾

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?

"""
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)

# 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