learningequality / ka-lite

KA Lite: lightweight web server for serving core Khan Academy content (videos and exercises) without needing internet connectivity
https://learningequality.org/ka-lite/
Other
457 stars 304 forks source link

Unused django package put in kalite/packages/dist #5419

Closed spatiald closed 7 years ago

spatiald commented 7 years ago

Summary

After upgrading from ka-lite-bundle_0.16.9-0ubuntu3_all.deb to ka-lite-bundle_0.17.0-0ubuntu1_all.deb, a previously occurring bug reappears.

System information

Traceback or relevant snippet from server.log

/usr/local/lib/python2.7/dist-packages/pytz/__init__.py:29: UserWarning: Module django was already imported from /usr/lib/python2.7/dist-packages/kalite/packages/bundled/django/__init__.pyc, but /usr/lib/python2.7/dist-packages/kalite/packages/dist is being added to sys.path
  from pkg_resources import resource_stream

Occurs when running kalite commands, for example:

root@WRTD-303N-Server:~# kalite --version
/usr/local/lib/python2.7/dist-packages/pytz/__init__.py:29: UserWarning: Module django was already imported from /usr/lib/python2.7/dist-packages/kalite/packages/bundled/django/__init__.pyc, but /usr/lib/python2.7/dist-packages/kalite/packages/dist is being added to sys.path
  from pkg_resources import resource_stream
0.17.0

How to reproduce

  1. Upgrade KA Lite from 0.16.3 to 0.17.0
  2. Run any kalite command

Real-life consequences

Honestly, this issue doesn't affect the deployment. It is just a duplicate warning that only is noticed in logs and terminal. I don't believe that affects KA Lite performance.

spatiald commented 7 years ago

Related to issue https://github.com/learningequality/ka-lite/issues/105

benjaoming commented 7 years ago

Thanks for reporting this @spatiald

It makes me wonder if during the 0.17.0 cleanup, we've removed code like this:

https://github.com/learningequality/ka-lite/pull/140/files

Because certainly the warnings.filterwarnings could have been called anywhere.

As you rightly say, it's harmless, but shouldn't be there.

Can you also reproduce this without even upgrading?

benjaoming commented 7 years ago

Just looked at your traceback:

/usr/local/lib/python2.7/dist-packages/pytz/__init__.py:29: UserWarning: Module django was already imported from /usr/lib/python2.7/dist-packages/kalite/packages/bundled/django/__init__.pyc, but /usr/lib/python2.7/dist-packages/kalite/packages/dist is being added to sys.path
  from pkg_resources import resource_stream

I notice the path /usr/local/lib/python2.7/dist-packages/pytz/__init__.py which indicates a local installation of pytz.

Could you try running this in Python?

import pytz
print(pytz.__file__)
print(pytz.__version__)
spatiald commented 7 years ago

Was on the road...thanks for your patience.
Here you go:

root@WRTD-303N-Server:~# python
Python 2.7.3 (default, Apr 10 2013, 05:46:21) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pytz

>>> print(pytz.__file__)
/usr/local/lib/python2.7/dist-packages/pytz/__init__.pyc

>>> print(pytz.__version__)
2015.4
benjaoming commented 7 years ago

I guess the nature of the warning is this: Django is already imported, yet a directory containing Django is being imported.

I think we can solve this by ensuring that django isn't installed to kalite/packages/dist which is populated when we build the source distribution.

I have pytz==2016.6.1 on my system. Looking at how they have changed L:29 from your traceback with the from pkg_resources import resource_stream statement, I think you might also be able to remove the warning by upgrading pytz. Take a look at open_resource(): https://github.com/stub42/pytz/blob/release_2016.6.1/src/pytz/__init__.py#L74

benjaoming commented 7 years ago

It looks like the underlying cause is a change in pip. Before, it would just install django/, but now it's opting for egg format, installing to Django-1.5.12-py2.7.egg-info/. Thus, the removal script fails because it does shutil.rmtree(os.path.join(STATIC_DIST_PACKAGES, "django")).

I'm trying to disable it by passing as_egg = False to InstallCommand() in setup.py, but it seems pip somehow goes on to ignore this.

Using pipdeptree, we can trace why Django 1.5 is installed:

pipdeptree                     
CherryPy==3.3.0
django-annoying==0.8.4
django-appconf==1.0.1
  - six [required: Any, installed: 1.10.0]
django-js-reverse==0.5.0
  - Django [required: >=1.5, installed: 1.5.12]
  - slimit [required: ==0.8.1., installed: 0.8.1]
    - ply [required: >=3.4, installed: 3.4]
django-tastypie-legacy==0.12.2.post2
  - python-dateutil [required: >=1.5,!=2.0, installed: 2.4.2]
    - six [required: >=1.5, installed: 1.10.0]
  - python-mimeparse [required: >=0.1.4, installed: 1.6.0]
docopt==0.6.2
ifcfg==0.9.3
importlib==1.0.3
pbkdf2==1.3
peewee==2.6.4
pipdeptree==0.10.1
  - pip [required: >=6.0.0, installed: 9.0.1]
requests==2.11.1
rsa==3.4.2
  - pyasn1 [required: >=0.1.3, installed: 0.2.3]
setuptools==34.4.1
  - appdirs [required: >=1.4.0, installed: 1.4.3]
  - packaging [required: >=16.8, installed: 16.8]
    - pyparsing [required: Any, installed: 2.2.0]
    - six [required: Any, installed: 1.10.0]
  - six [required: >=1.6.0, installed: 1.10.0]
South==1.0.2
wheel==0.29.0
youtube-dl==2015.11.24
benjaoming commented 7 years ago

This was worth investigating, thanks for the report @spatiald !

Fixed in #5447 - will be included in 0.17.1.

spatiald commented 7 years ago

Follow-up...thanks for the fix. No issues with this with 0.17.1

benjaoming commented 7 years ago

Thanks for testing this @spatiald :)