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

Support debug=True if native namespace-packages are present #1895

Merged
merged 4 commits into from Jan 28, 2022
Merged

Support debug=True if native namespace-packages are present #1895

merged 4 commits into from Jan 28, 2022

Conversation

ghost
Copy link

@ghost ghost commented Jan 20, 2022

debug=True fails if namespace-packages are present.

Traceback:

Traceback (most recent call last):
  File "tmp.py", line 67, in <module>
    app.run_server(dev_tools_silence_routes_logging=True, debug=True)
  File "/home/lschmelting/.pyenv/versions/py38/lib/python3.8/site-packages/dash/dash.py", line 1958, in run_server
    debug = self.enable_dev_tools(
  File "/home/lschmelting/.pyenv/versions/py38/lib/python3.8/site-packages/dash/dash.py", line 1721, in enable_dev_tools
    component_packages_dist = [
  File "/home/lschmelting/.pyenv/versions/py38/lib/python3.8/site-packages/dash/dash.py", line 1726, in <listcomp>
    else package.filename
AttributeError: '_NamespaceLoader' object has no attribute 'filename'

This PR solves the problem for me, though i don't know if there is a more elegant way.

@ghost ghost requested a review from alexcjohnson as a code owner January 20, 2022 11:10
@ghost
Copy link
Author

ghost commented Jan 20, 2022

Linting failed because i'm accessing _path:
W0212: Access to a protected member _path of a client class (protected-access)

but as far as i know, there is no public access to path for namespace-packages, atleast before python3.10
(python/importlib_resources#196 (comment)).
(python/importlib_resources#68 (comment))

I'm looking forward to your suggestions on how to handle this @alexcjohnson .

@alexcjohnson
Copy link
Collaborator

Super, thanks for the PR @lschmelting!

You can override this linting rule on the relevant line(s):

for p in c._prop_names # pylint: disable=protected-access

Is there an easy way we could add a test for this? ie include a namespace package in a test like the hot reloading test - we don't need to actually trigger hot reloading from the namespace package IMO, but we need to hit the hot reload code while there's a namespace package loaded. Perhaps turn something like dash-test-components into a namespace package? Or perhaps there's a way to mock this in the test by directly manipulating ComponentRegistry.registry in a way that would fail before this patch and succeed with it?

Then last thing is to add a changelog entry.

@ghost
Copy link
Author

ghost commented Jan 23, 2022

The problem only seems to affect native namespace packages. Turning dash-test-components into a native namespace package would require a different folder structure, since implicit namespace packages don't have an __ init__.py in the package directory (https://packaging.python.org/en/latest/guides/packaging-namespace-packages/#native-namespace-packages).

I took a closer look at the code surrounding ComponentRegistry.registry, but i don't see how we could simply mock the existence of a namespace-package yet.

@alexcjohnson
Copy link
Collaborator

Alright, let's not worry about a test - would be an awfully big lift compared to the code changes here. So the linting fix and a changelog entry and we'll be ready to go :)

@ghost ghost changed the title Support debug=True if namespace-packages are present Support debug=True if native namespace-packages are present Jan 28, 2022
Copy link
Collaborator

@alexcjohnson alexcjohnson left a comment

Choose a reason for hiding this comment

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

💃 Thank you very much @lschmelting!

@alexcjohnson alexcjohnson merged commit 89f1e6b into plotly:dev Jan 28, 2022
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.

1 participant