Skip to content

Distribute to setuptools - #8161

Merged
singingwolfboy merged 5 commits into
masterfrom
distribute-to-setuptools
Jul 9, 2015
Merged

Distribute to setuptools#8161
singingwolfboy merged 5 commits into
masterfrom
distribute-to-setuptools

Conversation

@singingwolfboy

Copy link
Copy Markdown
Contributor

Follow-on PR from #7465.

  • Replaces distribute with setuptools
  • Updates MySQL-python to 1.2.5 for setuptools compatibility
  • Uses a forked version of NLTK 2 that does not require distribute

@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch 2 times, most recently from 6e19ec4 to 4868a67 Compare May 22, 2015 18:52
@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@feanil I have this branch running on setuptools.m.sandbox.edx.org, and it works -- and I verified that setuptools 16.0 is really installed. I imagine that we need to change some things about this PR before it can be merged, though.

@sarina

sarina commented May 26, 2015

Copy link
Copy Markdown
Contributor

@singingwolfboy does this replace #7465? What's going on?

@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch from 4868a67 to 1b95f62 Compare May 30, 2015 07:21
@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@sarina yes, this is intended to replace #7465, although I don't think this PR is ready to be merged yet, since it depends on my fork of NLTK. To make this merge-ready, we should either make an NLTK fork and put it on the edx GitHub account, or upgrade to NLTK 3 (and deal with the backwards-incompatibilities). I'm waiting for some review & feedback from @feanil.

@sarina

sarina commented May 31, 2015

Copy link
Copy Markdown
Contributor

@singingwolfboy should #7465 be closed? Should an OSPR ticket be made for this, and be put for devops review?

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

I've altered OSPR-468 to point to this PR, instead of #7465.

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

Copy link
Copy Markdown
Contributor

@singingwolfboy could you give us an estimate of how much work it would take to rev up to NLTK3?

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@benpatterson: The work to upgrade to NLTK 3 can be divided up into two parts. The first part is identifying all the different places that we use NLTK in our various different projects, finding backwards-incompatible usages, and rewriting them to be compatible with NLTK 3. (Ideally, using try/except blocks to make it compatible with either NLTK 2 or 3.) Then we have to test all those changes, of course. Based on a cursory search of the codebase in my text editor, it looks like we use NLTK in one or two places in edx-platform and several places in ease. There are also several references to NLTK in edx-ora2, but it doesn't look like edx-ora2 is using NLTK directly -- it's probably declaring NLTK as a dependency because ease uses it.

That's the first part. The second part is identifying any courses running on edx.org where the course author is running custom Python code as part of the course, and contacting those course authors to determine if they are using NLTK. If so, we'll also need to inform them that NLTK is going to be upgraded to version 3, point them to the documentation with the backwards-incompatibilities, and ideally show them how to write code that is compatible with either NLTK 2 or 3.

I'm guessing that the first part will take a few days of work, mostly spent on testing. I have no idea how to estimate how much time and effort is involved in the second part.

@sarina sarina added the open-source-contribution PR author is not from Axim or 2U label Jun 2, 2015
@benpatterson

Copy link
Copy Markdown
Contributor

@singingwolfboy thanks for giving the details on it.

@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch from 1b95f62 to d18b9a6 Compare June 3, 2015 17:36
@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch 2 times, most recently from 57fd57b to a635bfe Compare June 15, 2015 15:49
@feanil

feanil commented Jun 18, 2015

Copy link
Copy Markdown
Contributor

jenkins test this please

@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch from a635bfe to 0ed878b Compare June 30, 2015 13:45
@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed awaiting prioritization labels Jul 6, 2015
@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch 3 times, most recently from 33ec032 to 4bea63f Compare July 6, 2015 15:07
@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch from 4bea63f to 3388c4b Compare July 6, 2015 20:46
@openedx-webhooks openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Jul 7, 2015
@feanil

feanil commented Jul 8, 2015

Copy link
Copy Markdown
Contributor

@singingwolfboy Brought up a sandbox and verified that at the end of the provision, it's the correct version. 🎆 I did a quick smoke test of the demo course as well and didn't see any issues. I think it's time. 👍

@sarina

sarina commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

@singingwolfboy @feanil what needs to happen next?

@feanil

feanil commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

@sarina we're trying to get ease and ora2 repos aligned before we can merge this.

@sarina

sarina commented Jul 9, 2015

Copy link
Copy Markdown
Contributor

Is there specific work we can point to on the JIRA ticket that indicates what it's blocked on?

@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch 2 times, most recently from 49620e9 to a673ce6 Compare July 9, 2015 16:00
regisb and others added 2 commits July 9, 2015 13:04
'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.
So that we can use setuptools instead of distribute
@singingwolfboy
singingwolfboy force-pushed the distribute-to-setuptools branch from a673ce6 to a31f685 Compare July 9, 2015 17:04
Comment thread requirements/edx/github.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.

This should be pinned instead of using -e

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.

Out of curiosity to me, what does the -e do?

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.

-e make pip install the dependency in "editable" mode: https://pip.pypa.io/en/stable/reference/pip_install.html#editable-installs I'm actually not sure what that means.

singingwolfboy added a commit that referenced this pull request Jul 9, 2015
@singingwolfboy
singingwolfboy merged commit da5ad4f into master Jul 9, 2015
@singingwolfboy
singingwolfboy deleted the distribute-to-setuptools branch July 9, 2015 19:00
@sarina sarina mentioned this pull request Jul 9, 2015
@regisb

regisb commented Jul 10, 2015

Copy link
Copy Markdown
Contributor

Thanks everyone for the great work!

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

@codref ansible no longer refers to post.txt as of two months ago: see https://github.com/edx/configuration/pull/1982 and https://github.com/edx/configuration/pull/1989. Can you make sure that you're updated to the latest version of the configuration repository, and let me know if you still get an error? If you do get an error, please post the traceback to a pastebin service (like Github Gist) and link to it, so that we can see what the problem is.

@codref

codref commented Jul 14, 2015

Copy link
Copy Markdown

Here you find the installation output with the error referring to a missing post.txt file.
The output came from a newly created ubuntu 12.04 virtual machine, I followed the steps described on the wiki (manual installation).

@singingwolfboy

Copy link
Copy Markdown
Contributor Author

We had to partially revert this pull request in #8929, because there was an unexpected interaction between adding python-saml to our requirements (from #8140) and upgrading setuptools. In the interest of not delaying the weekly release to edx.org, we reverted setuptools and put distribute back in -- but we are planning to do the setuptools upgrade once we can figure out why python-saml was unable to build under setuptools.

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

Labels

engineering review open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants