Skip to content

Conversation

@maxschulz-COL
Copy link
Contributor

@maxschulz-COL maxschulz-COL commented Aug 29, 2025

Closes https://github.com/McK-Internal/vizro-internal/issues/1866, closes #109

check for more...

How to review

First part will focus on making sure add_type works correctly, which is technically independent of this PRs purpose, but since the MM PR will require revalidate_instance="always", we have to make changes: https://mckinsey-hub.slack.com/archives/D03N7KXFXV3/p1756475773136999

Currently we are thinking about 2 options:

  • fix add_type to properly update the schema for ALL models
  • OR removing the need to use add_type at all

Second part with then focus on the actual MM bit.

(note: moved description over from #1317)

Description

Architecture

Key new concepts are:

  • there is no more model_manager.__models, it is replaced by a nutree tree that (atm) gets populated in dashboard.build
  • pre-build order it is depth-first, post-order --> some things needed to change in order to make this possibe
    • there are (currently) no exceptions from post-order traversal
    • _validate_pages moved to pre-build from a field_validator (which it really always should have been!)
    • lower-level models such as selectors or navlinks fetch their needed infos on demand from the tree as opposed to being set from (grand-)parent models
    • models created during pre-build are instantiated using an alternative __init__ which adds them to the tree AND calls pre-build if present
  • the above concepts are certainly not break-free/fool proof, but if core guidelines are followed, it looks good:
    • minimal modification of other models (this really causes headaches and confusion..), rather active request of models when needing it
    • (so far) no creation upwards, or at least not where post-order traversal would still move
    • unconditional info searching by any model (ie up and down the tree)

EDIT:
After going over the Parameter and Filter again, I am wondering if it is ok if certain non-pre-build relevant attributes of lower models can be set by parents. So far we have things like:

  • (not changed) selector titles, options etc
  • (changed, but this was not required) selectors _in_container flag when in Container --> this forced filters to be pre built before Container so selectors are guaranteed to exist
  • (never possible) wanting to set filter.targets in Container pre-build (see https://github.com/McK-Internal/vizro-internal/issues/1761#issuecomment-2985098855) --> this is solved by the filters just requesting the targets, instead of them being set

My rule of thumb would be:
OK, if this does not influence pre-build order, otherwise a strong NO

ToDos

  • Write unit tests
  • Write dev docs
  • Decide on how MM is best accessed
  • decide on global nature
  • decide on whether all models do get their node as attribute
  • make a new duplicate ID mechanism

For @antonymilne to specifically check, but can tell more in detail later:

  • Do data_frame Parameters / dynamic Filters work - I did not do any specific work for this
  • Do legacy actions work - I removed some code that you commented as maybe needing to stay
  • Discuss with @maxschulz-COL if and how we should stops the MM being a global variable, at least before the server starts

Preliminary intuition why up the tree pre-building is better than down the tree

It is very useful to know for certain that when you pre-build a model, everything below is already set, not vice versa. E.g. the Filter needs to be before Page because the Page checks if its filters are dynamic

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.

@maxschulz-COL maxschulz-COL changed the title [Refactor] Rework add_model functionality [Refactor] Rework Model Manager Try 2 Aug 29, 2025
Copy link
Contributor

@antonymilne antonymilne left a comment

Choose a reason for hiding this comment

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

Just doing this as a reminder to myself to look at my comments in #1317 and put these somewhere better to track in future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[MM] Rerun Vizro Dashboard in Jupyter doesn't work

3 participants