PLAT-2060 Use pip-tools to manage requirements files #17825

jmbowman · 2018-03-29T00:38:27Z

This splits requirements into high-level direct dependency files managed by hand, and fully detailed requirements files generated by pip-compile. The detailed files are all recreated by the "make upgrade" target. Points worth noting:

There are separate configuration and edx-platform PRs for upgrading setuptools and pip and then eliminating the pre.txt requirements file; those should probably be merged before this.
pip-tools does not yet support non-editable GitHub URL installations, but I worked around this in a post-processing shell script. There are 3 different open pip-tools PRs to fix this, but all of them have known problems preventing them from being merged. We prefer non-editable installations when possible, because pip won't try to re-install them if the correct version is already installed.
pip-sync won't work with the output files due to the same issue with non-editable GitHub URLs. We can still use pip install with them as normal, we just don't gain the benefit of automatically uninstalling any packages which were removed from the requirements.
pip-compile writes some of devstack's global pip configuration data to the generated files; I also handled this in the shell script for now.
pip-compile currently outputs absolute paths for local editable packages; until this is fixed upstream, I worked around it in the post-processing script also.
Indirect dependencies resulting from packages installed via GitHub don't have the source indicated in the output files. This is annoying, but better than the situation before (where we often weren't even capturing the fact that we depended on these).
The file reference removed from openedx.yaml is marked as obsolete in the OEP.
I removed the version pins in the input files for packages which are already current; this should help us keep them that way.
None of the existing requirements files still referenced by the configuration repo were removed (although some of them are now empty placeholders to be cleaned up later).

Packages which were deliberately upgraded:

celery 3.1.18 -> 3.1.25 (minimum requirement for edx-celery-utils)
click 3.3 -> 6.7 (edx-lint and pip-tools both require >=6)
cryptography 1.9 -> 2.1.4 (minimum requirement for pyOpenSSL)
execnet 1.4.1 -> 1.5.0 (indirect dependency of tox, removed it from input requirements)
fs 2.0.17 -> 2.0.18 (minimum requirement of new fs-s3fs version)
fs-s3fs 0.1.5 -> 0.1.8 (includes a patch I submitted to allow use with the latest version of six, which we were already using)
oauthlib 1.0.3 -> 2.0.1 (minimum requirement for django-oauth-toolkit)
py 1.4.34 -> 1.5.3 (indirect dependency of pytest and tox, removed it from input requirements)
PyJWT 1.4.0 -> 1.5.2 (minimum requirement for djangorestframework-jwt)
python-dateutil 2.1 -> 2.4.0 (minimum requirement for Faker, via factory-boy)
pytz 2016.7 -> 2016.10 (minimum requirement for django-ses)
requests-oauthlib 0.4.1 -> 0.6.1 (minimum requirement for social-auth-core)
rules 1.1.1 -> rules 1.3 (claims to fix some Django 1.10 and 1.11 issues)

Packages with recent releases which hadn't been pinned:

backports.functools-lru-cache 1.4 -> 1.5 (via astroid, pylint)
cffi 1.11.4 -> 1.11.5 (via cryptography)
Faker 0.8.10 -> 0.8.12 (via factory-boy)
httplib2 0.10.3 -> 0.11.1 (via oauth2, zendesk)
parsel 1.3.1 -> 1.4.0 (via scrapy)
pbr 3.1.1 -> 4.0.0 (via stevedore)
python-swiftclient 3.4.0 -> 3.5.0 (via ora2)
queuelib 1.4.2 -> 1.5.0 (via scrapy)
text-unidecode 1.1 -> 1.2 (via Faker)
virtualenv 15.1.0 -> 15.2.0 (via tox)
voluptuous 0.10.5 -> 0.11.1 (via ora2)
w3lib 1.18.0 -> 1.19.0 (via scrapy)

edx-status-bot · 2018-03-29T01:34:43Z

Your PR has finished running tests.

edx-status-bot · 2018-03-29T03:23:33Z

Your PR has finished running tests.

edx-status-bot · 2018-03-29T17:39:38Z

Your PR has finished running tests.

edx-status-bot · 2018-03-29T19:36:10Z

Your PR has finished running tests.

edx-status-bot · 2018-03-29T20:31:27Z

Your PR has finished running tests.

doctoryes · 2018-03-30T17:47:18Z

Makefile

+	pip-compile --upgrade -o requirements/edx/development.txt requirements/edx/development.in
+	scripts/post-pip-compile.sh requirements/edx/base.txt requirements/edx/coverage.txt \
+	    requirements/edx/development.txt requirements/edx/paver.txt requirements/edx/testing.txt \
+	    requirements/edx-sandbox/shared.in requirements/edx-sandbox/base.in


Were these edx-sandbox files supposed to be the .txt versions?

Yes, good catch. Fixing.

pwnage101 · 2018-03-30T18:26:18Z

Makefile

+	pip-compile --upgrade -o requirements/edx-sandbox/base.txt requirements/edx-sandbox/base.in
+	pip-compile --upgrade -o requirements/edx/base.txt requirements/edx/base.in
+	pip-compile --upgrade -o requirements/edx/testing.txt requirements/edx/testing.in
+	pip-compile --upgrade -o requirements/edx/development.txt requirements/edx/development.in


Are these all just in a specific order which guarantees that no compilation is performed with an old compiled .txt file? In general I thought that no .in file would install a .txt file, but maybe we want to do that for performance reasons?

Right, this sequence compiles them such that the ones which are included in other files get built before the ones that include them. The reason for including ".txt" instead of ".in" files is to ensure consistency of dependency versions across different contexts (so pip-tools doesn't resolve to one version for production and a different one for tests after constraints from additional dependencies are imposed). I'm currently making an edit to the draft OEP to explain that reasoning.

There needs to be a comment here pointing at that explanation. This looks easy to screw up.

bmedx · 2018-03-30T18:38:05Z

Makefile

+	    requirements/edx-sandbox/shared.in requirements/edx-sandbox/base.in
+	# Let tox control the Django version for tests
+	grep "^django==" requirements/edx/base.txt > requirements/edx/django.txt
+	sed '/^django==/d' requirements/edx/testing.txt > requirements/edx/testing.tmp


I think we've run into problems with these lines in other repos with "django" vs "Django" and had to do case-insensitive searches

One of the changes in this PR was to force-lowercase django itself, which I think solves the problem.

pip-compile always lower-cases the package names, but it doesn't hurt to make it case-insensitive in case somebody edits it manually for some reason. Changing.

bmedx · 2018-03-30T18:46:31Z

common/lib/chem/setup.py

@@ -8,6 +8,6 @@
        "pyparsing==2.0.7",
        "numpy==1.6.2",
        "scipy==0.14.0",
-        "nltk==3.2.5",
+        "nltk",


I think we need to bump the version of chem for this, otherwise existing virtualenvs might not install the new version and get the loosened requirement?

The generated requirements file still pins it. I want to gradually get these local apps out of the business of pinning anything. The only potential problem I can see down the road is if we upgrade nltk later and it breaks existing virtualenvs that still remember the earlier pin here...but with devstack images now getting routinely rebuilt and pulled, that seems less likely than before (and a good reason to yank this sooner rather than later). Am I still missing any subtleties of the issue with this?

Probably just me being traumatized from the ways this has broken recently. It's probably fine if nobody tries to touch that version any time soon. Seems like it would be safer to force the version so we know the right thing is there, but I don't feel strongly about it.

Why unpin nltk but not the other requirements?

n/m, "because nltk is at the latest version"

edx-status-bot · 2018-03-30T21:08:44Z

Your PR has finished running tests.

nedbat · 2018-04-03T10:16:16Z

Makefile

-clean:
+help: ## display this help message
+	@echo "Please use \`make <target>' where <target> is one of"
+	@perl -nle'print $& if m{^[a-zA-Z_-]+:.*?## .*$$}' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m  %-25s\033[0m %s\n", $$1, $$2}'


How can it be that make doesn't provide this functionality natively?? :(

Yeah, I originally pulled this from an upstream Django cookiecutter template and it's been spreading throughout our repos. The closest built-in feature that GNU make has is make --print-data-base, which omits the target description comments and includes piles of usually irrelevant cruft.

Just because, I tried my hand at a python replacement. It's longer...

@python -c 'import fileinput,re; ms=filter(None, (re.search("([a-zA-Z_-]+):.*?## (.*)$$",l) for l in fileinput.input())); print("\n".join(sorted("\033[36m {:25}\033[0m {}".format(*m.groups()) for m in ms)))' $(MAKEFILE_LIST)

But how did perl get in there in the first place? It's just a grep:

@grep -E '^[a-zA-Z_-]+:.*?##' $(MAKEFILE_LIST) | sort | awk 'BEGIN {FS = ":.*?## "}; {printf "\033[36m %-25s\033[0m %s\n", $$1, $$2}'

Which is shorter than the original, and just to continue the golfing:

@grep '^[a-zA-Z]' $(MAKEFILE_LIST) | sort | awk -F ':.*?## ' 'NF==2 {printf "\033[36m %-25s\033[0m %s\n", $$1, $$2}'

And honestly, I'm not a fan of the terminal coloring, so why not just:

@grep '^[a-zA-Z]' $(MAKEFILE_LIST) | sort | awk -F ':.*?## ' 'NF==2 {printf " %-26s%s\n", $$1, $$2}'

BTW, you should add "help" to the .PHONY target, I think.

Looks like that works, switching it is fine with me. Feel like submitting a PR upstream? https://github.com/pydanny/cookiecutter-djangopackage/blob/master/%7B%7Bcookiecutter.repo_name%7D%7D/Makefile#L16

Fingers crossed! :) pydanny/cookiecutter-djangopackage#378

nedbat · 2018-04-03T11:01:11Z

Makefile

+	pip install -qr requirements/edx/development.txt --exists-action w
+
+upgrade: ## update the pip requirements files to use the latest releases satisfying our constraints
+	pip install -q pip-tools


I don't want to get into an infinite loop here, but shouldn't we pin the version of pip-tools?

It is pinned in requirements/edx/development.txt, which is installed by the requirements target. I could depend on that target here, but it's a little slow even in the case where everything's already up to date because of all the editable requirements; that gets annoying if you have to keep running this to resolve version conflicts. I'll try splitting that and its dependencies out into a separate file.

nedbat · 2018-04-03T11:02:15Z

Makefile

+	pip-compile --upgrade -o requirements/edx/development.txt requirements/edx/development.in
+	scripts/post-pip-compile.sh requirements/edx/base.txt requirements/edx/coverage.txt \
+	    requirements/edx/development.txt requirements/edx/paver.txt requirements/edx/testing.txt \
+	    requirements/edx-sandbox/shared.txt requirements/edx-sandbox/base.txt


Let's put these each on their own line so we can see the list better.

Actually, I think I'm going to switch this to scripts/pip-compile.sh, which calls pip-compile and then does the post-processing on the output. That should simplify this make target a lot and make it less likely to forget something when updating it.

nedbat · 2018-04-03T11:02:59Z

Makefile

+	scripts/post-pip-compile.sh requirements/edx/base.txt requirements/edx/coverage.txt \
+	    requirements/edx/development.txt requirements/edx/paver.txt requirements/edx/testing.txt \
+	    requirements/edx-sandbox/shared.txt requirements/edx-sandbox/base.txt
+	# Let tox control the Django version for tests


There isn't a better way to do this?

Not that we've found yet. The requirements file we give tox can't have Django pinned to a specific version (it would conflict with the versions it assigns itself), but everything else should be. The "django.txt" file is here so Jenkins doesn't need to install all the development requirements, just the testing ones (since it isn't using tox for most runs yet, but installs the same test requirements file).

nedbat · 2018-04-03T11:20:48Z

openedx/core/djangoapps/user_api/tests/test_views.py

@@ -2590,7 +2590,7 @@ def _assert_time_zone_is_valid(self, time_zone_info):
        self.assertIn(time_zone_name, common_timezones_set)
        self.assertEqual(time_zone_info['description'], get_display_time_zone(time_zone_name))

-    @ddt.data((ALL_TIME_ZONES_URI, 436),
+    @ddt.data((ALL_TIME_ZONES_URI, 439),
              (COUNTRY_TIME_ZONES_URI, 28))


A comment here about when this number might have to change?

Sure, adding.

nedbat · 2018-04-03T11:29:21Z

requirements/edx-sandbox/local.txt

-common/lib/chem
-common/lib/sandbox-packages
-common/lib/symmath
+# Placeholder for code which hasn't yet been updated to no longer use this file.


What code hasn't been updated? Ansible?

Yes, a few different places in the configuration repo. After that's updated I plan to come back and remove these now-unnecessary files.

nedbat · 2018-04-03T11:44:11Z

requirements/edx/github.in

+#   * The "-e" prefix is primarily for the benefit of pip-compile, which does
+#     not yet support non-editable GitHub URLs.  If a VERSION is correctly
+#     specified as shown above, the "-e" prefix will be stripped from the
+#     output file by the post-pip-compile.sh script.


We should mention that this is the opposite of previous advice.

edx-status-bot · 2018-04-04T18:47:04Z

Your PR has finished running tests.

edx-status-bot · 2018-04-09T21:44:12Z

Your PR has finished running tests.

edx-pipeline-bot · 2018-04-10T18:55:39Z

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production on Wednesday, April 11, 2018.

edx-pipeline-bot · 2018-04-11T14:12:37Z

EdX Release Notice: This PR has been deployed to the production environment.

jmbowman force-pushed the jmbowman/PLAT-2060 branch from 251de85 to f0d2d31 Compare March 29, 2018 02:31

jmbowman force-pushed the jmbowman/PLAT-2060 branch from f0d2d31 to 8436d0b Compare March 29, 2018 16:55

jmbowman force-pushed the jmbowman/PLAT-2060 branch from 8436d0b to ddcef1c Compare March 29, 2018 18:59

jmbowman force-pushed the jmbowman/PLAT-2060 branch from ddcef1c to d3f1678 Compare March 29, 2018 19:47

jmbowman requested review from nedbat, pwnage101, doctoryes and bmedx March 30, 2018 16:02

doctoryes reviewed Mar 30, 2018

View reviewed changes

pwnage101 reviewed Mar 30, 2018

View reviewed changes

bmedx reviewed Mar 30, 2018

View reviewed changes

jmbowman force-pushed the jmbowman/PLAT-2060 branch from d3f1678 to 93873ed Compare March 30, 2018 20:15

doctoryes approved these changes Apr 2, 2018

View reviewed changes

nedbat reviewed Apr 3, 2018

View reviewed changes

jmbowman force-pushed the jmbowman/PLAT-2060 branch from 93873ed to 60c3a23 Compare April 4, 2018 18:02

jmbowman mentioned this pull request Apr 5, 2018

Move requirements of local packages to the global requirements #17625

Closed

PLAT-2060 Use pip-tools to manage requirements files

432347b

jmbowman force-pushed the jmbowman/PLAT-2060 branch from 60c3a23 to 432347b Compare April 9, 2018 21:07

jmbowman merged commit cce91d6 into master Apr 9, 2018

jmbowman deleted the jmbowman/PLAT-2060 branch April 9, 2018 22:51

feanil mentioned this pull request Apr 10, 2018

Revert "PLAT-2060 Use pip-tools to manage requirements files" #17914

Merged

jmbowman restored the jmbowman/PLAT-2060 branch April 10, 2018 17:50

PLAT-2060 Use pip-tools to manage requirements files #17825

PLAT-2060 Use pip-tools to manage requirements files #17825

Conversation

jmbowman commented Mar 29, 2018 • edited Loading

edx-status-bot commented Mar 29, 2018

edx-status-bot commented Mar 29, 2018

edx-status-bot commented Mar 29, 2018

edx-status-bot commented Mar 29, 2018

edx-status-bot commented Mar 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-status-bot commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nedbat Apr 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edx-status-bot commented Apr 4, 2018

edx-status-bot commented Apr 9, 2018

edx-pipeline-bot commented Apr 10, 2018

edx-pipeline-bot commented Apr 11, 2018

jmbowman commented Mar 29, 2018 •

edited

Loading

nedbat Apr 4, 2018 •

edited

Loading