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

Potential ways forward regarding a smoother experience in Jupyter #65

Closed
maxschulz-COL opened this issue Sep 26, 2023 · 5 comments
Closed
Assignees

Comments

@maxschulz-COL
Copy link
Contributor

At the moment, using Vizro in a Jupyter notebook may feel annoying when re-evaluating cells, because

  • adding a new model with the same ID causes the an error.
  • in addition, there is a further issue, that currently every instantiation of a model without ID adds it newly to the model_manager (MM), thus creating a situation where models are in the MM that are not used within the dashboard

Solving this may entail multiple levels of investigation/considerations. See also #56 for some notes and code around this issue.

Short term solution
In the short term, #59 ensures that users get a more comprehensive error message when working in Jupyter.

Note that this does not catch all cases, as a cell could also not contain a model with manual ID, thus never triggering an error that catches the additional models added to the MM.

Medium term solution

  • remove the restrictions to add duplicate ID's but at the same time improve all reference to the MM, especially _items_with_type
  • _items_with_type in particular should not be a wild card search to any type, but if at all search for a Dashboard, all subsequent scans should be hierarchical and on a per need basis
  • likely further investigation about consequences needed
  • note also: removing the duplicate ID restriction can also cause confusion when users do NOT want to update a model, but by mistake copied one

Long term solution

Idea 1

  • go back to hierarchical registering approach (ie not register upon instantiation)

Reasoning from @antonymilne as to why this is NOT a no-brainer:

I think this is one of the disadvantages of the recursive approach to registering models in the model manager. Let’s say you go for the recursive approach like this:

  1. call dashboard.register that recursively registers all models
  2. pre_build uses models existing in model manager
  3. pre_build can itself create models
  4. if we want those models to also be registered in the model manager then we need to somehow register them e.g. by manually calling selector.register or by re-running dashboard.register after the pre-build

This step 4 with the extra manual registration stage is a bit ugly, annoying and easy to forget, whereas if it’s done automatically upon model instantiation it’s not required at all.
tbh the current solution with pre_build creating models is also not very satisfactory, as described in the comment:

  # Note that a pre_build method can itself add a model (e.g. an Action) to the model manager, and so we need to
  # iterate through set(model_manager) rather than model_manager itself or we loop through something that
  # changes size.
  # Any models that are created during the pre-build process *will not* themselves have pre_build run on them.
  # In future may add a second pre_build loop after the first one. 

Overall if we do want to change how components are registered in the model manager then we’d need to think about it in combination with pre_build. Hopefully there’s some better overall solution, but I don’t think we can change one without thinking about the other at the same time. I’d love if we could come up with an improvement here, but I don’t think there’s going to be an easy solution.

Idea 2

  • introduce some form of context_manager that associates every model with a dashboard

Idea 3

  • introduce a flag when overriding is allowed
@antonymilne
Copy link
Contributor

I'll need to spend some time properly thinking this all through, since it's a tricky topic and not something we should rush. But just to note another possible idea while I remember: maybe we need another method on VizroBaseModel which enables you to instantiate and register a model in one go. This would avoid you needing to remember to manually call register after you've created a model in pre-build, but I'm not sure exactly what that register method would do.

@antonymilne
Copy link
Contributor

Another possible solution might be to clear the managers on shutdown of the flask process. I did this in an old solution where we used to copy assets to a temporary directory. It's not super reliable (e.g. from memory there's some complexities with multiple threads), but maybe in the context of Jupyter where you're unlikely to be doing that it would work well.

@antonymilne
Copy link
Contributor

antonymilne commented Oct 12, 2023

Another idea: a global dashboard object and navigating through that hierarchy (as in the medium term solution above) might actually be a good long term solution. This would probably avoid difficulties with needing to recursively register. But then how would pre_build know what to operate on?

Or it could be Vizro.dashboard - think about comments about Dash use in #109.

@antonymilne
Copy link
Contributor

antonymilne commented Oct 19, 2023

Another point against hierarchical registration or at least in favour of keeping _items_with_type: when we implement components nested in tabs or containers, it gets harder to collect e.g. all components on the page.

Note that #142 got rid of several calls to _items_with_type that required the dashboard.

@antonymilne
Copy link
Contributor

Closing this just so we don't have the comments spread over too many different places. #120 is a good fix for Jupyter for now, and the real issue here is much deeper than just Jupyter - will continue the conversation in #109 and #111 anyway.

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

No branches or pull requests

2 participants