Consider makefile and pre-commit utilities for CI and local development

[Rocamonde]

Moving the discussion on https://github.com/HumanCompatibleAI/imitation/pull/534#discussion_r963202712 to its own issue:

Problem

We currently use ./ci/code_checks.sh as a script to do a variety of code checks that are required for CI as as manual pre-commit. There's a few problems with this script:

1. It is not modular. One cannot select what parts of the CI checks to run. The only way is to open the script and copy paste the commands on the terminal; then rely on scrolling through the history to run them again when needed. Checks have configuration params, such as what folders to check or to ignore, and they aren't easy to memorize or quick to type from scratch every time.
2. It doesn't automatically run before pushing a commit to remote. This means that additional "formatting" and "linting" commits are needed which add noise to the commit tree/diffs. It is also more likely that users will unintentionally commit things that have type errors or bugs without noticing, which complicates fixing them later down the line in e.g. a PR.

Solution

Using the Makefile and the pre-commit tools are potential solutions to this problem. Which/how to use them is the purpose of this PR.

Benefits of pre-commit: it is modular and once installed allows for automatic pre-commit checks. Benefits of Makefile: it allows adding actions that are not necessarily relevant for pre-commit checks, but only relevant in local development.

Possible alternative solutions

Modify the script ./ci/code_checks.sh to allow modularity (lot of work, not worth it) and then manually symlink the pre-commit hook every time (also not great).

[AdamGleave]

I agree with 1 but not 2. You can just add a symlink to make it run as a commit hook, same as with any other script.

[Rocamonde]

Sure, you're right. I mentioned that symlinking the file is a potential solution, but one has to do it manually every time you install the repo. You could automate the symlink command by adding it to the docs or putting it in a shell script, but I guess I found the pre-commit install approach "cleaner", even if it's doing exactly that under the hood.

Now I think about this, we could automate the symlink when you install the repo in development mode. What do you think about that?

[ernestum]

I vote for giving pre-commit a try. I think it looks interesting and it could replace ci/code_checks.sh with something far more concise while potentially checking for far more corner cases than we ever could think of.

[ernestum]

I played around with it and came up with the following .pre-commit-config.yaml:

# See https://pre-commit.com for more information
# See https://pre-commit.com/hooks.html for more hooks
repos:
-   repo: https://github.com/pre-commit/pre-commit-hooks
    rev: v4.3.0
    hooks:
    -   id: trailing-whitespace
    -   id: end-of-file-fixer
    -   id: check-toml
    -   id: check-added-large-files
    -   id: check-ast
-   repo: https://github.com/psf/black
    rev: 22.6.0
    hooks:
    -   id: black
    -   id: black-jupyter
-   repo: https://github.com/PyCQA/flake8
    rev: 4.0.1
    hooks:
    -   id: flake8
        additional_dependencies:
            - "flake8-blind-except==0.2.1"
            - "flake8-builtins~=1.5.3"
            - "flake8-commas~=2.1.0"
            - "flake8-debugger~=4.1.2"
            - "flake8-docstrings~=1.6.0"
            - "darglint~=1.8.1"
-   repo: https://github.com/PyCQA/isort
    rev: 5.10.1
    hooks:
    -   id: isort
-   repo: https://github.com/pre-commit/mirrors-mypy
    rev: v0.981
    hooks:
    -   id: mypy

What I liked so far:

1. we can remove black, flake8 etc. from the test requirements. They are no test requirements after all since we do not need them to run pytest. pre-commit automatically installs a fixed version of them (pinned by the rev: in the config file) in a separate environment.
2. It is really easy to update the revisions of all the tools to the newest version using the pre-commit autoupdate command. It pulls the newest version numbers and places them in the config file.
3. Found and fixed quite a number of little gotchas on the first run (mostly trailing white spaces)
4. Pretty output (even nicer with colors)

cleanup*
trim trailing whitespace.................................................Failed
- hook id: trailing-whitespace
- exit code: 1
- files were modified by this hook

Fixing experiments/README.md
Fixing docs/algorithms/bc.rst
Fixing docs/index.rst
Fixing docs/algorithms/preference_comparisons.rst
Fixing docs/algorithms/mce_irl.rst
Fixing runners/launch_docker-dev.sh
Fixing docs/_templates/autosummary/module.rst

fix end of files.........................................................Failed
- hook id: end-of-file-fixer
- exit code: 1
- files were modified by this hook

Fixing tests/generate_test_data.sh
Fixing .gitignore
Fixing docs/_static/css/custom.css
Fixing ci/code_checks.sh
Fixing docs/.gitignore

check yaml...............................................................Passed
check for added large files..............................................Passed
check python ast.........................................................Passed
black....................................................................Failed
- hook id: black
- files were modified by this hook

reformatted tests/algorithms/test_bc.py

All done! ✨ 🍰 ✨
1 file reformatted, 103 files left unchanged.

black-jupyter............................................................Passed
flake8...................................................................Failed
- hook id: flake8
- exit code: 1

src/imitation/rewards/reward_nets.py:217:89: E501 line too long (89 > 88 characters)
src/imitation/algorithms/preference_comparisons.py:462:89: E501 line too long (89 > 88 characters)
src/imitation/algorithms/base.py:194:89: E501 line too long (95 > 88 characters)

isort....................................................................Passed

5. Automatic support to run either on changed files or on all files.
6. Automatically excluded non-staged files (which often were an annoyance for me)
7. It is much faster. When modifying a random source file, pre-commit takes about 1.5s on my machine vs 16s for our custom script.

[AdamGleave]

Example of a working config in another project for reference (only visible to HumanCompatibleAI org members, sorry): https://github.com/HumanCompatibleAI/go_attack/blob/main/.pre-commit-config.yaml https://github.com/HumanCompatibleAI/go_attack/blob/main/.circleci/config.yml

[AdamGleave]

This has been added now.