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

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

Closed
spatiald opened this Issue Mar 9, 2017 · 9 comments

Comments

Projects
None yet
2 participants
@spatiald

spatiald commented Mar 9, 2017

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

  • Operating system: Intel CAP v1 running Ubuntu 12.04 LTS
  • Version: 0.17.0
  • Browser: N/A

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

This comment has been minimized.

Show comment
Hide comment
@spatiald

spatiald Mar 9, 2017

Related to issue #105

spatiald commented Mar 9, 2017

Related to issue #105

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Mar 13, 2017

Member

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?

Member

benjaoming commented Mar 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Mar 16, 2017

Member

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__)
Member

benjaoming commented Mar 16, 2017

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

This comment has been minimized.

Show comment
Hide comment
@spatiald

spatiald Apr 14, 2017

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

spatiald commented Apr 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 14, 2017

Member

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

Member

benjaoming commented Apr 14, 2017

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 benjaoming changed the title from "Module django was already imported" <- it's back to Unused django package put in kalite/packages/dist Apr 14, 2017

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 14, 2017

Member

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
Member

benjaoming commented Apr 14, 2017

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

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming Apr 14, 2017

Member

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

Fixed in #5447 - will be included in 0.17.1.

Member

benjaoming commented Apr 14, 2017

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

Fixed in #5447 - will be included in 0.17.1.

@benjaoming benjaoming closed this Apr 14, 2017

@spatiald

This comment has been minimized.

Show comment
Hide comment
@spatiald

spatiald May 12, 2017

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

spatiald commented May 12, 2017

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

@benjaoming

This comment has been minimized.

Show comment
Hide comment
@benjaoming

benjaoming May 17, 2017

Member

Thanks for testing this @spatiald :)

Member

benjaoming commented May 17, 2017

Thanks for testing this @spatiald :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment