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

petar-qb · 2024-12-13T16:03:46Z

Description

This is bugfix made on top of #903.

This PR enables that model_manager can find components that are nested arbitrarily deep inside basic type structures (e.g. lists within lists) too, and not just specific attributes like (e.g. "components", "tabs", "controls", "actions", "selector"). So, this PR enables you to define any component structure, For example: my_custom_components_property: list[list[tuple[vm.VizroBaseComponent]]].

This PR does not solve nesting components inside dict type. This is known limitation emphasised with the "TODO" already, and will be solved separately.

See how scratch/app.py works from this, but doesn't work from the main branch. The best way to see the problem is to refresh the page and see if the controls are taken into account when the page loads.

Screenshot

Notice

I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
- I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
- I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
- I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
- I have not referenced individuals, products or companies in any commits, directly or indirectly.
- I have not added data or restricted code in any commits, directly or indirectly.

…pe structures too

github-actions · 2024-12-13T16:04:54Z

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2024-12-13 16:04:53 UTC
Commit: e857ce0

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/

maxschulz-COL

Can we write some tests for this case ;)

petar-qb · 2024-12-13T16:57:59Z

Can we write some tests for this case ;)

I also had this on my mind, but I've decided not to do so because all these util methods are still private. We don't test private methods much because it can easily happen that their structure or API completely changes. And, model_manager will be changed again (maybe even drastically changed) when we introduce pydantic_v2 (this is also highlighted in the TODO comments).

huong-li-nguyen · 2024-12-16T09:44:10Z

Can we write some tests for this case ;)

I also had this on my mind, but I've decided not to do so because all these util methods are still private. We don't test private methods much because it can easily happen that their structure or API completely changes. And, model_manager will be changed again (maybe even drastically changed) when we introduce pydantic_v2 (this is also highlighted in the TODO comments).

I agree with @maxschulz-COL that we should include unit tests though. We do test private methods in other models, but for some we have deprioritized it. Ideally, anything that could be unintentionally broken should be tested. We don't need to test every possible level of nesting, but we should cover basic cases, such as the code snippet mentioned in this issue: #82

Is this scenario included in our tests? If not, would you mind adding it? I remember Antony sent out this issue again, but it broke after the action refactoring, and we only noticed when another user requested a similar feature right? I think optimally unit tests should have caught this earlier, but we didn't have them in place 😬 So maybe let's use the chance now?

huong-li-nguyen · 2024-12-16T09:20:08Z

vizro-core/src/vizro/managers/_model_manager.py

@@ -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.


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?

huong-li-nguyen · 2024-12-16T09:37:24Z

vizro-core/changelog.d/20241213_161527_petar_pejovic_fix_model_manager_get_model_children.md

+
+### 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))


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

huong-li-nguyen · 2024-12-16T09:45:10Z

vizro-core/examples/scratch_dev/app.py

+    controls: List[Tuple[str, List[ControlType]]] = [[]]
+
+    def build(self):
+        return html.Div(


Amazing, how this works now! 🥳 🍾

petar-qb · 2024-12-17T10:15:25Z

Due to prioritisation and capacity for the current sprint, I will write unit tests and finalise this PR next year.

Enable that components can be nested arbitrarily deep inside basic ty…

e857ce0

…pe structures too

petar-qb self-assigned this Dec 13, 2024

petar-qb requested review from Joseph-Perkins, antonymilne, huong-li-nguyen, maxschulz-COL and lingyielia as code owners December 13, 2024 16:03

maxschulz-COL reviewed Dec 13, 2024

View reviewed changes

huong-li-nguyen reviewed Dec 16, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

petar-qb commented Dec 13, 2024

github-actions bot commented Dec 13, 2024

maxschulz-COL left a comment

petar-qb commented Dec 13, 2024 •

edited

Loading

huong-li-nguyen commented Dec 16, 2024 •

edited

Loading

huong-li-nguyen Dec 16, 2024 •

edited

Loading

huong-li-nguyen Dec 16, 2024

huong-li-nguyen Dec 16, 2024

petar-qb commented Dec 17, 2024

	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.


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

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

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

Are you sure you want to change the base?

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

Conversation

petar-qb commented Dec 13, 2024

Description

Screenshot

Notice

github-actions bot commented Dec 13, 2024

View the example dashboards of the current commit live on PyCafe ☕ 🚀

maxschulz-COL left a comment

Choose a reason for hiding this comment

petar-qb commented Dec 13, 2024 • edited Loading

huong-li-nguyen commented Dec 16, 2024 • edited Loading

huong-li-nguyen Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

huong-li-nguyen Dec 16, 2024

Choose a reason for hiding this comment

huong-li-nguyen Dec 16, 2024

Choose a reason for hiding this comment

petar-qb commented Dec 17, 2024

petar-qb commented Dec 13, 2024 •

edited

Loading

huong-li-nguyen commented Dec 16, 2024 •

edited

Loading

huong-li-nguyen Dec 16, 2024 •

edited

Loading