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

[WIP] Make Dash Pages & packages play nice & experimenting with clearing Pages registry #2271

Closed
wants to merge 2 commits into from

Conversation

ned2
Copy link
Contributor

@ned2 ned2 commented Oct 13, 2022

Please don't merge.

This is something of an experiment to try to address the problem of Dash Pages not playing nicely when used with a Dash app that's housed in a Python package, as discussed in #2263.

The solution I adopted was to prefix the modules programatically imported by Dash Pages (eg the contents of pages/*.py) with the name parameter of the Dash instance. All the user needs to do then is invokes their apps as Dash(name=__package__). This largely worked (as far as I can tell) but broke some tests that made some assumptions about the contents of _pages.PAGE_REGISTRY across multiple Dash app instantiations.

The more experimental part of this PR is to clear the contents of the _pages.PAGE_REGISTRY on __init__ of each Dash instance.

@ned2
Copy link
Contributor Author

ned2 commented Nov 2, 2022

hey @alexcjohnson, curious to get your feedback on this proposal if you have some time to review. Would be great to get Dash apps with Pages to properly support living inside Python packages.

My solution involved clearing some dash module global state on app startup, which might seem like a heavy handed approach, but on reflection, I wonder if working towards doing this for all global app-specific state would be a sensible move anyway (eg for callbacks). Currently, multiple Dash app instances started in the same process that uses either Dash Pages or the dash.callback decorator run the risk of dirty state causing conflicts, as I discovered the hard way while implementing the first part of my solution.

I had thought there was some tests that were failing, but it looks like that might be to do with some local weirdness when I try to run the full test sweet, as the tests have passed ok in CI. Though maybe there's something fishy is going on with Percy tests?

@alexcjohnson
Copy link
Collaborator

Thanks @ned2 - I saw this PR come in (along with your extensive investigations in #2263) but haven't had a chance to look at it in detail yet. I recall when @AnnMarieW and I were discussing pages prior to its release we identified some cases where keeping this global state is important, and other cases where it's problematic. That may just have had to do with tests though, I'll need to poke around again.

Can you give me a little more context on when and why you run multiple apps in the same process, just so I can understand the use case a bit better?

maybe there's something fishy is going on with Percy tests?

That should be cleared up next time you merge/rebase on master

@ned2
Copy link
Contributor Author

ned2 commented Nov 5, 2022

Can you give me a little more context on when and why you run multiple apps in the same process, just so I can understand the use case a bit better?

I haven't thought of any contexts where I'd want to run multiple Dash apps in the same process, besides the motivating context of running consecutive tests. So the catalysing motivation for clearing the Page registry is to fix the clashing page keys within tests that my solution introduced, as described here: #2263 (comment).

I'm not totally fixed in clearing the registry on app init. I imagine there's other strategies to solve the package issue... either a different solution that doesn't introduce conflicting keys in the registry, or using the same approach, but making changes to the test harness to address the conflicts.

I proposed clearing the registry because a) it feels like a clean solution, b) would mean Dash devs don't have to have to spend brain cycles on futzing around clearing the registry manually in tests, and b) it feels desirable to eliminate the possibility of dirty global state breaking user apps in unexpected and probably rather opaque ways.

I'd be curious to know if there are any situations where it is important to keep global page registry state (or callback state) present across multiple Dash instances.

…de one

- Dash Pages layout modules are now prefixed with `Dash.name`. Providing that
  the supplied `name` param matches the package name (eg
  `Dash(name=__package__)`), Pages modules will be attached as expected into
  your package namespace. eg `pages.page1` --> `my_package.pages.page1`.
- To prevent the pages registry from sharing state across multiple `Dash` app
  instances, the pages registry is now cleared on `Dash` instance init. This is
  necessary as the existing tests relied on the fact that multiple app inits in
  the one process would clobber the contents of previous page registeries, but
  with the above changes, we now see conflicting entries for the same pages
  folder. eg `test1.pages.page1` and `test2.pages.page1`, which both reference
  the same `page1.py`, resulting in invalid duplicate entries in the layout and
  page registry.
@ned2
Copy link
Contributor Author

ned2 commented Jan 27, 2023

I've since come up with a cleaner way to address #2263 that doesn't rely on modifying any global state, so will close this and pick up in another PR.

@ned2 ned2 closed this Jan 27, 2023
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.

2 participants