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

Rerun Vizro Dashboard in Jupyter doesn't work #109

Open
1 task done
noklam opened this issue Oct 12, 2023 · 12 comments · Fixed by #120
Open
1 task done

Rerun Vizro Dashboard in Jupyter doesn't work #109

noklam opened this issue Oct 12, 2023 · 12 comments · Fixed by #120
Assignees
Labels
Bug Report 🐛 Issue contains a bug report

Comments

@noklam
Copy link

noklam commented Oct 12, 2023

Description

ValidationError: 1 validation error for Graph
figure
Component with id=scatter_chart already exists and is mapped to dataset 140638475834896. Components must uniquely map to a dataset across the whole dashboard. If you are working from a Jupyter Notebook, please either restart the kernel, or use 'from vizro.managers import data_manager; data_manager._reset()`. (type=value_error)

'from vizro.managers import data_manager; data_manager._reset()`. After I follow the instruction and rerun the cell, I run into a new error.

DuplicateIDError: Model with id=scatter_chart already exists. Models must have a unique id across the whole dashboard. If you are working from a Jupyter Notebook, please either restart the kernel, or use 'from vizro.managers import model_manager; model_manager._reset()`.

After I run 'from vizro.managers import model_manager; model_manager._reset()`, no error is raised but it doesn't show the dashboard UI anymore.

image

Expected behavior

I expect the dashboard render properly. Workaround is restart the Kernel which work fine but is considerably slower.

vizro version

0.1.4

Python version

3.8.5

OS

MacOS

How to Reproduce

Follow the tutorial and run it in a notebook twice.

https://vizro.readthedocs.io/en/stable/pages/tutorials/first_dashboard/#4-explore-further

Output

No response

Code of Conduct

@noklam noklam added Bug Report 🐛 Issue contains a bug report Needs triage 🔍 Issue needs triaging labels Oct 12, 2023
@maxschulz-COL
Copy link
Contributor

maxschulz-COL commented Oct 13, 2023

Hey @noklam,

thanks so much for getting in touch :) I was able to reproduce this issue in parts (dashboard not rendering), and it is indeed a bug. We are working on a smoother Jupyter experience already (see #65), but it looks like that the short term solution described in this issue does not capture this case.

So in the short term we would recommend going via the approach of restarting the kernel, this is as you say slower, but the most reliable method. We will get back once there is a more comprehensive solution.

In the meantime, could I ask you whether you receive the above error message after resetting both managers, and rerunning the shown cell again? This is how I tried to reproduce it, and for me the dashboard does not render, but I also do not get an error message. You mentioned that "no error is raised", so just want to check whether I am seeing the same thing, or I should be seeing the error in the screenshot

@maxschulz-COL maxschulz-COL removed the Needs triage 🔍 Issue needs triaging label Oct 13, 2023
@maxschulz-COL maxschulz-COL self-assigned this Oct 13, 2023
@maxschulz-COL
Copy link
Contributor

Reproducing this error works, although atm the error message does not show for me. Code can be found at: https://github.com/mckinsey/vizro/tree/bug/109

Observations:

  • page registries are different but not empty
  • comparing the global variable dash seems to suggest that only supplied_layout and layout are differing, and in that, only the IDs
  • resetting the managers, the dash.page_registry as well as choosing a new port does not fix the issue

@ned2
Copy link

ned2 commented Oct 17, 2023

I ran into this issue too. If you open up the browser dev tools, you'll see that the Dash renderer is throwing a duplicate callback issue. This is caused by some global state about registered callbacks that is shared across the entire dash module to enable the dash.callback convenience decorator, which saves you from having to thread your dash.Dash instance throughout different modules (ie app.callback()). Resetting dash._callback.GLOBAL_CALLBACK_LIST and dash._callback.GLOBAL_CALLBACK_MAP before re-running the Vizro app fixes this issue.

As already mentioned, there's also some global state for Dash Page registry, and also for Page config. These should be reset too. I ran into this when adding tests for the Pages feature, where this global state was interfering with subsequent tests, so added this fixture to clear the global Pages state.

Putting it all together, here's a recipe for clearing (hopefully) everything, and which allows the demo app to be re-run when you call the function at the top of the cell containing the app:

import dash
from vizro.managers import data_manager, model_manager


def reset_state():
    data_manager._reset()
    model_manager._reset()
    dash._callback.GLOBAL_CALLBACK_LIST = []
    dash._callback.GLOBAL_CALLBACK_MAP = {}
    dash._pages.PAGE_REGISTRY.clear()
    dash._pages.CONFIG.clear()
    dash._pages.CONFIG.__dict__.clear()

@antonymilne
Copy link
Contributor

Hello @ned2 and thank you very much for the tip! I've seen your name pop up a lot when I read the plotly forums and so it's amazing to see you here 😀 I'm just curious how you found out about vizro in the first place?

As you can see we've been on a similar journey trying to reset the dash page registry in a test fixture but couldn't find a way to clear the whole thing. Given what you recommend above I presume there's no way to do it with dash.page_registry directly and you have to go through the protected dash._pages instead?

Also in the clear_pages_state fixture you linked to is there any reason you do init_pages_state both before and after the yield? I would have thought that just clearing up after a test would be sufficient?

@huong-li-nguyen looks like what @ned2 above is the right way to do this test fixture 🎉

@maxschulz-COL I think we should rename _reset to _clear given that the interface for both data manager and model manager is similar to a dictionary, which uses .clear().

@ned2
Copy link

ned2 commented Oct 17, 2023

heya @antonymilne! I heard about vizro essentially by word of mouth. Someone shared it on the viz channel of a slack community I hangout in. Really love what y'all are working on here!!

Unfortunately there's not an interface for clearing any of the global state in the dash module. Understandbly, the Dash team are focusing on supporting ease of development for the overwhelming majority of Dash use-cases that never have more than a single Dash instance defined in a process. Using Dash as you are here falls into a bit more of a niche use-case. Though now that I think about it, have you looked into how Dash's builtin support for running in Jupyter notebooks works? From a user's perspective, they seem to have solved this problem.

Given that Dash is already implicitly making the assumption that users won' be creating multiple Dash instances within the same process, I think there's a strong argument to be made that Dash itself should lean into actively making the one instance-per-process assumption and remove this footgun by explicitly clearing all global state in the dash module within the Dash.__init__().

Oh yes, in that fixture in the Dash repo, the reason I cleared the state before and after the test contents are run. The tests I added were apparently the first tests sensitive to existing stale global Page state and I didn't want to assume that people would know to add this decorator to tests that mutated the registry. So essentially I was protecting my new tests from being impacted by other tests. The initial call could be removed if you new that every other text was required to use the same fixture. Again, this would all be a non-issue if the Dash class itself cleared global state.

@antonymilne
Copy link
Contributor

Thanks for all the comments @ned2 - it's super helpful. Given what you say I'd be tempted to add the code that resets the Dash globals to our own Vizro.__init__ which just calls Dash.__init__ (though our data_manager and model_manager couldn't be cleared there since they're needed later on). My hesitation would be that it's too brittle a solution since these hidden global variables are not part of the Dash API, so presumably a future version of Dash could remove/rename them without warning and leave our Vizro() broken.

Just to add to the above: we'll also need to clear GLOBAL_INLINE_SCRIPTS since we have some clientside callbacks. Since Dash exposes page_registry at top-level I guess we could do dash.page_registry.clear() rather than dash._pages.PAGE_REGISTRY.clear() but since we need to access the other stuff through _pages anyway it doesn't make much difference.

Great point about checking how JupyterDash works 👍 I don't see anything that clears global state there. There's something which is meant to kill an existing process so you can re-run on the same port (which has actually never worked for me and been on my long-term todo list to try to fix) but nothing else as far as I can see. So actually I wonder...

  1. Where does the limitation that "Multi-page apps using Dash Pages are not supported in notebooks" come from? We are using Dash Pages in notebooks and it works fine (I think anyway?) so perhaps the limitation is just because they need to reset the page registry etc.
  2. Why do they not need to reset callbacks? Maybe actually they should be doing so, just no one yet found a bug that resulted from not doing this

@ned9
Copy link

ned9 commented Oct 18, 2023

Why do they not need to reset callbacks? Maybe actually they should be doing so, just no one yet found a bug that resulted from not doing this

yeah that's a good question. I dug into this, and it looks like Dash itself clears the contents of GLOBAL_CALLBACK_LIST when the app receives it's first request. More specifically, this happens in _setup_routes() here, which is registered using Flask.before_request to run before every request, but the function returns immediately if it's not the first request the app sees.

So that seems to be how stale values in GLOBAL_CALLBACK_LIST are avoided on rerunning Dash apps in a notebook. And why multipage apps don't work I guess---there's nothing done to protect the registry from going stale.

As for why it's not working in Vizro. I poked around using a funky tool I found to observe when GLOBAL_CALLBACK_LIST is mutated, and I think I might have a handle on what's going on.

When using Dash Pages, on app init, inside dash.enable_pages, the router function is registered with Flask to run on every request so that it can get the right layout fragment to insert into the main layout. If the page item fetched from the Pages registry is a callable, it needs to be called to get the layout object. It so happens that Page.build() calls Page._update_graph_theme, which registers the update_graph_theme callback. That means that a callback is being registered after the Dash server has started running. Ordinarily this would just result in a callback that doesn't work (hence why Dash docs mentions you don't want to do this), but the side-effect of this for us here is that GLOBAL_INLINE_SCRIPTS gets a new callback entry that won't be cleared by Dash, as it only does that before the first callback (as mentioned above).

So TLDR it sounds like the root cause of the problem is that the Pages.build() method couples layout creation and callback registration together.

@maxschulz-COL
Copy link
Contributor

Implemented a _clear_state() function as a first step towards a smoother Jupyter experience.

Thanks again @ned2 for your valuable insights. We will make sure to build upon this for a solution that eventually may not even require a code-snippet at the top of a cell.

@antonymilne
Copy link
Contributor

antonymilne commented Oct 20, 2023

@ned9 That makes sense, thank you very much for digging into it! I hadn't seen watchpoints before and that looks like it will be very handy for this sort of thing.

Do you think that the Dash class itself should be clearing the dash._pages variables? Given that it takes responsibility for clearing the callbacks, it would feel like maybe that would ultimately also be the right place for the dash._pages clearing code to live. This would presumably enable them to lift the restriction that multi-page apps don't work in Jupyter, although I guess in practice most multi-page apps would need to be structured quite differently from the conventional style so that they could work from a single notebook (rather than one Python file per page), so it's maybe not much of interest to Dash in general. If you think this is the right approach though then I will try to raise a PR on the Dash; otherwise we'll leave the code that does the clearing just in Vizro itself.

If we do move the _page clearing code to Dash itself then this would leave the Vizro clearing responsibility as:

  • objects that we're responsible for: data_manager and model_manager, which makes sense
  • things which we seem to have caused to malfunction a bit like the GLOBAL_CALLBACK_LIST you mention above, which we might not need to do if we restructure things

FYI @petar-qb what @ned9 says above about callback registration is very useful to know!

@maxschulz-COL
Copy link
Contributor

Reopen for visibility

@maxschulz-COL maxschulz-COL reopened this Oct 23, 2023
@LucaYoy
Copy link

LucaYoy commented Oct 25, 2023

Does anyone know anything about the error <ContextVar name='callback_context' at 0x7f78465b2f90> when trying to run a dashboard in jupyterHub?

@antonymilne
Copy link
Contributor

@LucaYoy this sounds like a separate issue to me so I've split it off into a new issue: #124. I'll follow up there 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Report 🐛 Issue contains a bug report
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants