Closed antoinerg closed 4 years ago
cc @josegonzalez @alexcjohnson
Can we wrap that in a try/ImportError
?
Or can we move the logic out of the __init__.py
? I'd say that its pretty bad practice to put code in there (although its probably easier for imports).
Can we wrap that in a
try/ImportError
?
What are you suggesting we wrap?
Or can we move the logic out of the init.py? I'd say that its pretty bad practice to put code in there (although its probably easier for imports).
It's worth a try. I'm a little bit confused by the code in here. Is it me or the file _api_requests is not imported/used anywhere?
we can (and should) move all that stuff out of __init__.py
, but that won't fix the issue because __init__.py
will just import it from wherever that code goes. And in principle we could wrap the import dash_core_components as _dcc
in try/ImportError
but if dcc isn't there we're going to have problems later. dcc should come in with dash, which is listed in setup.cfg
- does Py 3.6.0 fail to correctly read setup.cfg
or something? Or is it just not installed in the right order, and by the end of the installation we have it?
does Py 3.6.0 fail to correctly read
setup.cfg
or something
The use of setup.cfg
was introduced in setuptools 30.3.0
Py 3.6.0 has an older version of setuptools so it should choke
$ docker run -it -v $(pwd):$(pwd) -w $(pwd) python:3.6.0 python -c "import setuptools; print(setuptools.__version__)"
28.8.0
Py 3.6.4 is fine:
$ docker run -it -v $(pwd):$(pwd) -w $(pwd) python:3.6.4 python -c "import setuptools; print(setuptools.__version__)"
39.0.1
but it still fail
Possibly: https://bugs.python.org/issue28279
I also mean that the __init__.py
should also not do that import either, and the code to use enterprise auth would be:
# auth is dash_enterprise_auth/auth.py
from dash_enterprise_auth import auth
# the methods are now in the auth module
auth.create_logout_button()
This wouldn't be bc-compatible of course.
Could we avoid using a setup.cfg
? That might also fix the issue. I've never used a cfg file (only setup.py) so not sure of any disadvantages of dropping it.
Could we avoid using a
setup.cfg
? That might also fix the issue. I've never used a cfg file (only setup.py) so not sure of any disadvantages of dropping it.
That's what @alexcjohnson recommended doing and I'm hopeful it will solve the issue. It's not clear setup.cfg
can be properly parsed in Python 3.6
as per https://bugs.python.org/issue28279 !
I also mean that the
__init__.py
should also not do that import either
API-breaking aside, honestly that seems backward to me - it both makes the package more verbose to use and defers this problem from install time to run time, which would make debugging even harder. Yes I'd prefer we move the functionality to dash_enterprise_auth/auth.py
but then I still think __init__.py
should import and re-export the public API
I'm personally of the opinion that packages should have no logic in their __init__.py
files - this has caused me more issues in production than I care to remember. Numpy/scipy are another package that does this - I recall that scipy depends on numpy during the install but doesn't need to, and neither project wants to fix the underlying issue - and it's quite painful to see in prod.
We don't actually depend on dash
but on dcc
in this project, though the install_requires
points at dash
. If there was no logic in that __init__.py
and we had the correct import, as long as the package installed, then there wouldn't be a problem.
The prod deployment argument makes sense, but there must be a way around that without compromising usability for local users. Makes me wonder why the package needs to be imported at all during installation... but this is getting far off topic. Glad it was just a .py
vs .cfg
issue, it's really useful to know for future reference that we just shouldn't use setup.cfg
.
From the root of the project on
master
, the following fails:whereas it succeeds with python
3.6.5
:It might be related to https://github.com/plotly/dash-enterprise-auth/issues/5