Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[BUG] Dash Pages does not support relative package imports within page modules #2263

Closed
ned2 opened this issue Oct 8, 2022 · 3 comments · Fixed by #2392
Closed

[BUG] Dash Pages does not support relative package imports within page modules #2263

ned2 opened this issue Oct 8, 2022 · 3 comments · Fixed by #2392

Comments

@ned2
Copy link
Contributor

ned2 commented Oct 8, 2022

Env:

dash                 2.6.2
dash-core-components 2.0.0
dash-html-components 2.0.0
dash-table           5.0.0

Describe the bug

I typically make my Dash apps as Python packages, using relative imports within my app. I've just discovered that Dash Pages is not happy with relative imports being used within your page modules (eg pages/page1.py). As far as I can tell, this is because Dash manually loads the contents of pages as modules using exec_module, but, if your app is inside a package, it does not attach the page modules to the package. So when you try to access a utils.py in the base of your package, eg with from ..utils import blah within pages/page1.py, you'll get:

ImportError: attempted relative import beyond top-level package

So effectively, every module within your pages directory is orphaned from the package, and you won't be able to use relative imports to share utility functions across your page modules and the rest of your app.

Minimal example

__init__.py (empty)
app.py

import dash
app = dash.Dash(__name__, use_pages=True)

app.layout = dash.html.Div(["main page", dash.page_container])

app.run_server(debug=True)

pages/__init__.py (empty)
pages/page.py

import dash

from ..utils import blah

dash.register_page(__name__, path="/page")

layout = dash.html.Div("page")

utils.py

def blah():
    pass

Runnning python -m dash_pages_bug.app from the directory above results in:

ImportError: attempted relative import beyond top-level package
@ned2
Copy link
Contributor Author

ned2 commented Oct 9, 2022

I think I've tracked down a workaround...

The problem is that within _import_layouts_from_pages, in dash.py, module_name is set to pages.page, which does not include a reference to the package I'm working in.

            module_name = ".".join([pages_folder, page_filename])

            spec = importlib.util.spec_from_file_location(
                module_name, os.path.join(root, file)
            )
            page_module = importlib.util.module_from_spec(spec)
            spec.loader.exec_module(page_module)

So we need to get the package name reflected in the module_name variable as my_package.pages.page. Since my standard practice is to set the name param of the Dash instance to the value of __package__, I can leverage that by changing the first line of the above snippet in dash.py to:

 module_name = ".".join([self.config.name, pages_folder, page_filename])

I also had to make a similar change in the router so that the 404 page can be retrieved from the router, to match the module name structure now found in the registry.

Not sure if this workaround will generalise to apps not defined in packages. I might have a go trying these changes against the test suite.

@ned2
Copy link
Contributor Author

ned2 commented Oct 9, 2022

So after running Dash's tests/integrations/multi_page tests, the main problem I'm seeing with my attempted solution is that the page registry is shared globally across all Dash instances defined during the life of a process, without any name-spacing. So my above modification has the effect of populating duplicate entries in the page registry. eg:

test_pages_order.pages.page1
test_pages_layout.pages.page1

Previously, the second would have overwritten the first, as they both would have resolved to pages.page1, but now, because page1 is attached twice, we end up with a Dash app that fails validation (twice) as it has duplicate IDs in the layout and duplicate paths in the registry. Not sure how often we'd see apps using multiple Dash instances that have different name params and share the same page modules. Feels like an unlikely scenario in the wild, but it is certainly done intentionally in the test harness.

@AnnMarieW do you have any other ideas for getting Dash Pages modules to play nice with relative imports inside a package? Or is it worth pursuing the line of thinking I was going down?

@ned2 ned2 changed the title [BUG] Dash pages does not support relative package imports within page modules [BUG] Dash Pages does not support relative package imports within page modules Oct 9, 2022
@ned2
Copy link
Contributor Author

ned2 commented Oct 13, 2022

ok I have a candidate solution to the above issues that I'd be keen to get your take on @T4rk1n @alexcjohnson, and @ann-marie-ward

the problem with my solution of prefixing Pages layout modules with the name attribute of Dash instances, is actually more of a side effect of the fact that Dash apps have some global state in _pages.PAGE_REGISTRY that persists into subsequent Dash app inits.

I've done some experimentation in this PR #2271 to see what happens when we clear the contents of _pages.PAGE_REGISTRY on init of each Dash instance. the TLDR is that it fixes the issue of dirty state polluting the now more-sensitive index used by the registry, but breaks two tests in some possibly surprising ways.

          551 passed
             2 failed
               - tests/integration/callbacks/test_basic_callback.py:282 test_cbsc006_array_of_objects[orjson]
               - tests/integration/devtools/test_hot_reload.py:33 test_dvhr001_hot_reload
             6 skipped

it looks like they are sensitive to dirty state hanging around from previous app inits. something that I think is a particular smell is that test_cbsc006_array_of_objects actually runs fine when run in isolation without any pre-existing state in the registry. ie with:

pytest -k test_cbsc006_array_of_objects

I'm wondering if this also opens up a slightly larger question of whether all global Dash state should be reset on app init. beyond Dash pages, there's also the global state that the dash.callback introduced, which I could imagine resulting in conflicting state across Dash apps in the same process.

but for now, how do we feel about clearing _pages.PAGE_REGISTRY on init?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant