1395231 - Improve requirements.txt format

Status: RESOLVED FIXED

(Webtools Graveyard :: Pontoon, enhancement, P5)

Description

[[DEACTIVATED] Adrian Gaudebert]

I hate requirements.txt files, and especially, I hate them now that we use --require-hashes, because we add a lot of stuff in there without expressing which ones we actually use, and which ones are required by other dependencies. So, I would like to change that. All it takes is to make some formatting in that file, to differentiate top-level modules from the rest, and try to group them so that it's easy to see all that can be removed when a top-level dependency is removed.

I've tried a few different formats and this is what I like best: have a single requirements.txt file with "blocks" for each top-level dependency that has subsequent requirements. Top-level imports that do not require anything can be put at the top of the file in a single block.

Here's an example:

> # No-requirements dependencies.
> py-bcrypt==0.4 \
>     --hash=sha256:5fa13bce551468350d66c4883694850570f3da28d6866bb638ba44fe5eabda78
> https://github.com/mathjazz/silme/archive/v0.9.3.zip#egg=silme==0.9.3 \
>     --hash=sha256:9aa618f068ed71ecc8820dd7538aa39e499ed4ce61a0074edbcfcf1008da1d1c
> 
> # ----------------------------------------------------------------------------------
> Django==1.11.4 \
>     --hash=sha256:6fd30e05dc9af265f7d7d10cfb0efa013e6236db0853c9f47c74c585587c5a57 \
>     --hash=sha256:abe86e67dda9897a1536a727ed57dbefb5a42b41943be3b116fe3edab4c07bb2
> 
> # Required by Django
> pytz==2017.2 \
>     --hash=sha256:c883c2d6670042c7bc1688645cac73dd2b03193d1f7a6847b6154e96890be06d \
>     --hash=sha256:03c9962afe00e503e2d96abab4e8998a0f84d4230fa57afe1e0528473698cdd9
> 
> # ----------------------------------------------------------------------------------
> requests==2.18.4 \
>     --hash=sha256:6a1b267aa90cac58ac3a765d067950e7dbbf75b1da07e895d1f594193a40a38b \
>     --hash=sha256:9c443e7324ba5b85070c4a818ade28bfabedf16ea10206da1132edaa6dda237e
> 
> # Required by requests
> urllib3==1.22 \
>     --hash=sha256:06330f386d6e4b195fbfc736b297f58c5a892e4440e54d294d7004e3a9bbea1b \
>     --hash=sha256:cc44da8e1145637334317feebd728bd869a35285b93cbb4cca2577da7e62db4f
> idna==2.6 \
>     --hash=sha256:8c7309c718f94b3a625cb648ace320157ad16ff131ae0af362c9f21b80ef6ec4 \
>     --hash=sha256:2c6a5de3089009e3da7c5dde64a141dbc8551d5b7f6cf4ed7c2568d0cc520a8f
> chardet==3.0.2 \
>     --hash=sha256:6ebf56457934fdce01fb5ada5582762a84eed94cad43ed877964aebbdd8174c0 \
>     --hash=sha256:4f7832e7c583348a9eddd927ee8514b3bf717c061f57b21dbe7697211454d9bb
> certifi==2017.7.27.1 \
>     --hash=sha256:54a07c09c586b0e4c619f02a5e94e36619da8e2b053e20f594348c0611803704 \
>     --hash=sha256:40523d2efb60523e113b44602298f0960e900388cf3bb6043f645cf57ea9e3f5

Pros:
-----

* it will make maintenance of the requirements files much, much easier;
* during reviews, it's super easy to see what top-level dependencies are added, and what is only requirements of those deps;
* removing a top-level dependency is going to be much easier, as all it's sub-requirements will be identified;
* by switching to that format, we might find a few dependencies that we do not need anymore -- less dependencies is better!

Cons:
-----

* it requires a bit more work to add requirements to the project, as after you have used hashin, you'll need to edit the file to make the formatting correct;
* we might make mistakes in our formatting, leading to devs thinking something isn't used. Luckily that is mitigated by the fact that we do test our app. :)

Comment 1

[[DEACTIVATED] Adrian Gaudebert]

CC'ing mathjazz, Pike and jotes for opinions on this.

Comment 2

[Matjaz Horvat [:mathjazz]]

I like the idea.

Shall we also format dependencies of dependencies differently?

Comment 3

[Axel Hecht [:Pike]]

Any suggestion for star dependencies? Aka, several things depending on six, or the like?

Also, is that a real problem for more than, say, six?

Comment 4

[Jarek Śmiejczak [:jotes]]

I like the overall idea. It's just too easy to sweep deps under the carpet and at some point it may be hard to maintain.
I think that some parts of your approach can be automated (probably not now but in the future).
At work I use https://github.com/naiquevin/pipdeptree - that's a tool in the similar space, maybe we could create the new tool that will restructure requirements.txt file automatically.

Comment 5

[[DEACTIVATED] Adrian Gaudebert]

I've started working on this, verifying all our dependencies, making a fresh requirements.txt file with only the top-level dependencies, slowly adding all sub-deps. In the end, I have passing tests and a seemingly working webapp, and all those requirements I have been able to remove: 

certifi==14.05.14
setuptools==36.2.4
pyasn1==0.1.7
mercurial==4.1.2
chardet==2.3.0
newrelic==2.50.0.39
configparser==3.5.0b2
blessings==1.6
py-bcrypt==0.3

That makes me quite suspicious that I might be doing something wrong, though. I would _expect_ that `mercurial` for example would be required by the sync app? And what about `py-bcrypt`? Don't we use `newrelic` also? This is very weird to me.

Matjaz, do you mind taking a quick look and letting me know if you know for sure that one of those is actually needed for some part of the app to work?

Comment 6

[[DEACTIVATED] Adrian Gaudebert]

(In reply to Axel Hecht [:Pike] from comment #3)
> Any suggestion for star dependencies? Aka, several things depending on six,
> or the like?
> 
> Also, is that a real problem for more than, say, six?

I think we can afford not worrying about this. For one, six is actually a direct dependency of Pontoon. Furthermore, with that new format, the library would be "attached" to one requirement. If we were to remove that requirement, with all its dependencies, building the docker environment will very quickly tell you that the dependency is actually still needed, and you can then re-add it, attached to another library.

I don't think there's a good answer to that problem anyway. I think we should try it for now, and see how it works out for us. I'll also show it to other teams to get their opinions. I'm pretty sure other people have put some thoughts into this.

Comment 7

[[DEACTIVATED] Adrian Gaudebert]

(In reply to Matjaz Horvat [:mathjazz] from comment #2)
> Shall we also format dependencies of dependencies differently?

I am basically just adding them under their parent dependency. It might become a hassle in some specific cases, but the "deeper" we have at the moment is two levels (ie, one top-level requirement that has a dependency that has a dependency). I think that's fine for now.

Comment 8

[Matjaz Horvat [:mathjazz]]

mercurial we need during sync

newrelic I also believe we need:
https://github.com/mozilla/pontoon/commit/2421d9b048c984f3ae557d9c024235d6d87e940f

certifi, chardet, pyasn1 were all added as part of peep, which we no longer use, so we might be good without them:
https://github.com/mozilla/pontoon/commit/ae7b918f8d026d74640583fa0e51613484da36e3#diff-b4ef698db8ca845e5845c4618278f29a

setuptools is apparently a dependency of bleach:
https://github.com/mozilla/pontoon/pull/655#pullrequestreview-53315669

configparser was used for handling .ini files, which is now covered by silme, so we should be able to remove it

blessings were needed for tests only, maybe a req for factory_boy; if tests pass, we should be good without it:
https://github.com/mozilla/pontoon/commit/2617fbfda4daeabcd28380bf377c3e36f2c72d92

py-bcrypt comes from playdoh, I'm not sure we need it:
https://github.com/mozilla/pontoon/commit/c2bc0d493da03fbc581d6ea49c1a5ee7ad88267c#diff-b4ef698db8ca845e5845c4618278f29a

--

I suggest we deploy your branch with update to the requirements file to stage and:
- test the app
- sync all projects *with the no-commit flag*

Comment 9

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/pontoon/pull/732

Comment 10

[[github robot]]

Commit pushed to master at https://github.com/mozilla/pontoon

https://github.com/mozilla/pontoon/commit/25028697d4d8d4f30a03c7aad353d7af18cc4232
Fixes bug 1395231 - Format requirements files. (#732)

* Fixes bug 1395231 - Format requirements files.

* Move django_nose app to the dev settings.

* Moved django-nose back to prod settings.

* Moved nose to base requirements.