Skip to content

Upgrade from distribute to modern version of setuptools - #7465

Closed
regisb wants to merge 1 commit into
openedx:masterfrom
openfun:openfun/get-rid-of-distribute
Closed

Upgrade from distribute to modern version of setuptools#7465
regisb wants to merge 1 commit into
openedx:masterfrom
openfun:openfun/get-rid-of-distribute

Conversation

@regisb

@regisb regisb commented Mar 24, 2015

Copy link
Copy Markdown
Contributor

Context: 'distribute' is the ancestor of 'setuptools', and many packages require
setuptools to be installed. Actually, some requirements of edx-platform require an old version of distribute, while others require a more recent one.

Proposed solution: migrate to a recent version of setuptools (10.1). As a consequence, Mysql-Python needs to be upgraded, too.

@regisb
regisb force-pushed the openfun/get-rid-of-distribute branch 2 times, most recently from 413f8a1 to 43cd520 Compare March 24, 2015 17:31
Comment thread requirements/edx/base.txt Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If we can't upgrade to 2.0.5 yet, please keep the dependency pinned at 2.0.4.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

edx-ora2 requires nltk==2.0.5. I thought it would be easier to upgrade to nltk==2.0.5 by first upgrading the requirements of edx-ora2, but if we pin the dependency it will not be possible to upgrade without conflict...

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

2.5 or 2.0.5? I think the issue with not pinning a dependency is that it means we can have inconsistencies across environments, and end up with unexpected failures when servers upgrade to the latest versions unexpectedly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I meant 2.0.5 (I edited my answer). I totally understand if you prefer to have pinned dependencies. You will just have to remember to upgrade both dependencies from edx-ora2 and edx-platform at the same time next time you upgrade :-)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Let's see how the test run goes, but yeah I suspect that's going to be a requirement. I'm going to have DB take a look at this pr - probably next week - since he knows more about our requirements and the workflow for upgrading.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

In general, I'd rather see the library (edx-ora2) with flexible versions, and the application (edx-platform) with fixed ones.

@sarina

sarina commented Mar 25, 2015

Copy link
Copy Markdown
Contributor

@regisb please check out https://openedx.atlassian.net/wiki/display/TE/Upgrading+Dependent+Libraries and update your pull request description with the answers to as many of the questions as apply.

Comment thread requirements/edx/pre.txt Outdated

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm curious why you're specifically pinning setuptools at version 9.1, when the latest stable version is 14.3.1. Why not use the latest stable version?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the commit message, nltk==2.0.4 is not compatible with the most recent versions of setuptools.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Gotcha. But if we pinned nltk==2.0.5, and updated that for edx-ora2 as well, then we could use setuptools 14.3.1, right? Or are there other reasons why we can't do that?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, exactly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

NLTK is now at 2.0.5 for edx-platform and edx-ora2. Can we make this PR target the latest stable version of setuptools?

@singingwolfboy

Copy link
Copy Markdown
Contributor

jenkins, test this please

@singingwolfboy

Copy link
Copy Markdown
Contributor

It appears that this pull request is blocked on #6712. Debugging context is below.

When running tests on this pull request with Jenkins, pybabel fails to run. (pybabel is used for generating translation strings.) Upon further investigation, the traceback is:

$ pybabel -q extract --mapping=conf/locale/babel_mako.cfg --add-comments="Translators:" --keyword="interpolate" . --output=conf/locale/en/LC_MESSAGES/mako.po
Traceback (most recent call last):
  File "/Users/db/.virtualenvs/edx-platform/bin/pybabel", line 5, in <module>
    from pkg_resources import load_entry_point
  File "/Users/db/.virtualenvs/edx-platform/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2957, in <module>
    working_set = WorkingSet._build_master()
  File "/Users/db/.virtualenvs/edx-platform/lib/python2.7/site-packages/pkg_resources/__init__.py", line 569, in _build_master
    return cls._build_from_requirements(__requires__)
  File "/Users/db/.virtualenvs/edx-platform/lib/python2.7/site-packages/pkg_resources/__init__.py", line 582, in _build_from_requirements
    dists = ws.resolve(reqs, Environment())
  File "/Users/db/.virtualenvs/edx-platform/lib/python2.7/site-packages/pkg_resources/__init__.py", line 761, in resolve
    raise DistributionNotFound(req)
pkg_resources.DistributionNotFound: pytz>=0a

Searching the web for information about this error message reveals lots of people having similar trouble. It seems like new versions of pip and setuptools have trouble with the versioning scheme that pytz used to use, assuming that any letter in the version indicates a development-only package of some kind. pytz's versioning scheme has been updated to work properly with what pip and setuptools believe it should be, so we need to update pytz first -- which is what #6712 is for.

@openedx-webhooks openedx-webhooks added blocked by other work PR cannot be finished until other work is complete and removed community manager review labels Mar 30, 2015
@regisb

regisb commented Mar 30, 2015

Copy link
Copy Markdown
Contributor Author

Yes, I can confirm I have had issues with pytz and setuptools. Should I create a PR to update ora2 dependencies?

@singingwolfboy

Copy link
Copy Markdown
Contributor

@regisb I commented in #6712 that I've already created PRs to upgrade pytz in edx-ora2 and ease, and I indicated in an earlier comment that I've created openedx/edx-ora2#686 to upgrade NLTK in edx-ora2. Are there other PRs that we need for this dependency chain? It's already really long and complicated. 😢

@regisb

regisb commented Mar 30, 2015

Copy link
Copy Markdown
Contributor Author

We could take the opportunity to upgrade to Django 1.7.

Just kidding :)

@jzoldak

jzoldak commented Apr 3, 2015

Copy link
Copy Markdown
Contributor

tagging @feanil who is working on this in the feanil/remove_distribute branch

@feanil

feanil commented Apr 6, 2015

Copy link
Copy Markdown
Contributor

Change makes sense to me: You'll have to update lazy and matplotlib as well to have them work correctly with setup tools.

My brach for reference: https://github.com/edx/edx-platform/compare/feanil/remove_distribute

I also removed rednose which was not compatible with setuptools but also not really necessary as far as I could tell.

@sarina

sarina commented Apr 6, 2015

Copy link
Copy Markdown
Contributor

@feanil : should @regisb continue with this work here, or are you planning to have a PR with the remove_distribute branch?

@feanil

feanil commented Apr 6, 2015

Copy link
Copy Markdown
Contributor

My PR was for testing with wheels and I don't think it is something I'll have time to work on in the near future. I think if @regisb is willing to drive this change, I'm happy to have it be merged and just rebase my stuff from there.

@regisb

regisb commented Apr 6, 2015

Copy link
Copy Markdown
Contributor Author

Hi all,

Unfortunately I'm on holidays so I won't have the time to do the additional changes until next Monday. I'll update my PR first thing next week

Régis

Sent from my portable toaster. Please excuse the brevity.

Le 3 avril 2015 16:12:35 GMT+02:00, Jesse Zoldak notifications@github.com a écrit :

tagging @feanil who is working on this in the feanil/remove_distribute
branch


Reply to this email directly or view it on GitHub:
https://github.com/edx/edx-platform/pull/7465#issuecomment-89301111

@jzoldak

jzoldak commented Apr 7, 2015

Copy link
Copy Markdown
Contributor

IIRC (and FWIW) @cpennington had added rednose a while back for colorizing test output.

@cpennington

Copy link
Copy Markdown
Contributor

Yeah, rednose is definitely optional.

@jzoldak

jzoldak commented Apr 8, 2015

Copy link
Copy Markdown
Contributor

FYI rednose was just updated by @benpatterson and @clytwynec in #7626

@regisb
regisb force-pushed the openfun/get-rid-of-distribute branch 2 times, most recently from f428055 to 67cd6a9 Compare April 13, 2015 11:52
@regisb

regisb commented Apr 13, 2015

Copy link
Copy Markdown
Contributor Author

I updated this PR with the most recent version of pytz to see if tests pass.
We can synchronize this week to merge all three PRs, if you're ok with that.

@regisb
regisb force-pushed the openfun/get-rid-of-distribute branch 3 times, most recently from 71c52e8 to e1f0c4f Compare April 13, 2015 16:55
@openedx-webhooks openedx-webhooks added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label May 13, 2015
@regisb
regisb force-pushed the openfun/get-rid-of-distribute branch 2 times, most recently from 281aa38 to 3b934fc Compare May 18, 2015 07:34
@regisb

regisb commented May 18, 2015

Copy link
Copy Markdown
Contributor Author

Good catch! I also took the liberty to remove the distribute install section from the create-dev-env.sh script (I'm not sure if this script is used any more?).

@sarina

sarina commented May 18, 2015

Copy link
Copy Markdown
Contributor

@regisb your build is failing because none of the unit test builds can run. I can replicate this locally - please investigate and fix. Thanks!

$ paver test_python
---> pavelib.tests.test_python
---> pavelib.prereqs.install_prereqs
---> pavelib.prereqs.install_ruby_prereqs
Ruby prereqs unchanged, skipping...
---> pavelib.prereqs.install_node_prereqs
Node prereqs unchanged, skipping...
---> pavelib.prereqs.install_python_prereqs
pip install -q --exists-action w -r requirements/edx/pre.txt
You are using pip version 6.0.8, however version 6.1.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
pip install -q --exists-action w -r requirements/edx/github.txt
You are using pip version 6.0.8, however version 6.1.1 is available.
You should consider upgrading via the 'pip install --upgrade pip' command.
  Could not find a tag or branch '96e1922348bfe6d99201b9512a9ed946c87b7e0b', assuming commit.
    Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-CD99La-build


Captured Task Output:
---------------------

---> pavelib.tests.test_python
---> pavelib.prereqs.install_prereqs
---> pavelib.prereqs.install_ruby_prereqs
---> pavelib.prereqs.install_node_prereqs
---> pavelib.prereqs.install_python_prereqs
pip install -q --exists-action w -r requirements/edx/pre.txt
pip install -q --exists-action w -r requirements/edx/github.txt

Build failed running pavelib.tests.test_python: Subprocess return code: 1

@sarina

sarina commented May 18, 2015

Copy link
Copy Markdown
Contributor

Also when I ran that command, I ended up with

    lms/static/sass/application-rtl.scss
    lms/static/sass/application.scss
    lms/static/sass/course-rtl.scss
    lms/static/sass/course.scss

in my working tree, which are no longer being compiled/used anymore. I'm surprised to see that, if you actually rebased on top of the latest version of master.

@singingwolfboy

Copy link
Copy Markdown
Contributor

@regisb the create-dev-env.sh script is not maintained anymore, and is almost certainly broken. It should probably be removed from the repository, but that another job for another pull request. Removing the distribute references from that script is fine, but it's probably still broken.

'distribute' is the ancestor of 'setuptools', and many packages require
setuptools to be installed.

Upgrade of MySQL-python from 1.2.4 to 1.2.5 is required because of an
incompatibility with setuptools>0.7.
@regisb
regisb force-pushed the openfun/get-rid-of-distribute branch from 3b934fc to 4ef12e4 Compare May 19, 2015 07:21
@regisb

regisb commented May 19, 2015

Copy link
Copy Markdown
Contributor Author

Tests are fixed after a rebase.

@openedx-webhooks openedx-webhooks added community manager review and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels May 19, 2015
@singingwolfboy

Copy link
Copy Markdown
Contributor

I tried building another sandbox, and got the same result as before: it still has setuptools installed at version 0.6. I found another reference to an old version of setuptools in the configuration repo, and I made a PR to update it there -- I'm currently building another sandbox that uses this PR for edx-platform and that PR for configuration.

Also, @regisb: it looks like setuptools version 16 just came out. As long as this PR hasn't been merged yet, can you update it to use setuptools 16, instead of 15.2?

@singingwolfboy

Copy link
Copy Markdown
Contributor

New sandbox still didn't work. After doing some more digging, I think I found the problem -- and it's a doozy.

We depend on NLTK at version 2.0.5 -- in fact, we actually upgraded from 2.0.4 to 2.0.5 specifically for this setuptools upgrade. However, NLTK's installation routine will automatically download and install distribute, if it detects that distribute is not installed. [ref 1, ref 2] It will actually uninstall an up-to-date version of setuptools and install distribute instead. Because Python packaging is a mess and no one knows how to do it properly, and at one point this was the accepted best practice.

We have two options. Either we could upgrade NLTK again and jump to version 3.0.2, which is a big, scary change with lots of backwards-incompatibilies, and therefore is very unlikely to happen. Or we could fork NLTK 2.0.5 and remove the distribute_setup.py file and all references to it, to prevent NLTK from replacing setuptools with distribute.

@feanil: how difficult would it be for us to fork NLTK 2.0.5?

@feanil

feanil commented May 19, 2015

Copy link
Copy Markdown
Contributor

Forking is easy but I'd rather not do it if we have no plan for getting back onto the version used upstream...

@singingwolfboy

Copy link
Copy Markdown
Contributor

@feanil: from my perspective, upgrading to NLTK 3 is a large effort, and one that will definitely affect courses running on edx.org. I think it's worth doing, but I don't think it's worth doing right now. On the other hand, we're really close with this distribute-to-setuptools upgrade, and I think it's worth forking NLTK to get this change in.

@sarina

sarina commented May 19, 2015

Copy link
Copy Markdown
Contributor

pardon if I'm missing something, but can we just install (or re-install) setuptools once nltk is done installing?

@singingwolfboy

Copy link
Copy Markdown
Contributor

@sarina Maybe? In my testing, when I tried to do something like that, I sometimes got weird errors involving the egg_info subcommand not being found. On the other hand, I just ssh'ed in to my sandbox server, and was able to use pip to switch between setuptools and distribute without any problem. It would still be very weird to install setuptools, and then reinstall it, but I suppose we can use comments to explain what's going on and why.

@feanil: is that option better or worse than forking NLTK?

@benpatterson

Copy link
Copy Markdown
Contributor

Reintroduce post.txt?

@feanil

feanil commented May 19, 2015

Copy link
Copy Markdown
Contributor

I think if we were going to do this, you would have to add it right after the NLTK requirement so that everything except for NLTK is installed using setuptools. My question in this case would be: Will NLTK still work correctly if we do this?

@regisb

regisb commented May 19, 2015

Copy link
Copy Markdown
Contributor Author

IMHO there wouldn't be much of an improvement over the current situation if we had to re-install setuptools over nltk; we'd probably face exactly the same problems as we do right now. So I'm in favor of forking. The fork is very limited in scope and can be documented clearly, e.g: in the README.

@singingwolfboy

Copy link
Copy Markdown
Contributor

@regisb: can you elaborate on exactly which problems we're facing right now? I'm all in favor of switching from distribute to setuptools, but I'm in favor of it simply because that means that we get to benefit from bugfixes (including security fixes) since then, and because some libraries that we want to use in the future may depend on the latest features of setuptools. I'm not aware of any problems that we're facing right now by sticking with distribute, but maybe you've hit some problems that I'm not aware of.

@singingwolfboy

Copy link
Copy Markdown
Contributor

Regarding @feanil's question: I'm fairly certain that the setuptools/distribute question is only relevant during install time. Once NLTK is installed, it shouldn't care about setuptools or distribute while NLTK is being used, so I believe that it should work correctly with setuptools installed. That being said, I haven't actually tested this.

@regisb

regisb commented May 21, 2015

Copy link
Copy Markdown
Contributor Author

@singingwolfboy I find that sometimes multiple versions of setuptools are installed concurrently. This causes weird errors, including the egg_info issue you mentioned. Basically, whenever the older setuptools is used, more recent setuptools options are not supported.
The whole point of this PR is that it's very difficult to handle dependencies that rely on recent setuptools whenever distribute has been installed earlier.

@singingwolfboy

Copy link
Copy Markdown
Contributor

I'm closing this PR in favor of #8161. I'm also altering OSPR-468 to point to that PR, instead of this one.

@regisb

regisb commented Jun 2, 2015

Copy link
Copy Markdown
Contributor Author

Ok, I'll start following #8161.

@regisb
regisb deleted the openfun/get-rid-of-distribute branch February 16, 2016 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants