LORIS-MRI tooling configuration

maximemulder commented 2 months ago

PR Description

This PR adds a tooling configuration to LORIS-MRI via pyproject.toml (PEP 518).

This PR configures three tools :

Ruff, a Python linter
Pyright, a Python type checker
Pytest, a Python testing library

Thoughts and things to discuss at the next meeting :

Location of the `pyproject.toml` file

Usually, Python projects have the following architecture

src/
    foo.py
tests/
    test_foo.py
pyproject.toml

We do not use this architecture, so the two locations to place pyproject.toml would either be in the root directory, or in the python directory. I propose to use the root directory, because the tools read the pyproject.toml file to get their configuration when running them from the command line, and having the file in the root directory allows to run these tools from there instead of checking out other directories, which is more pratical IMO.

Ruff

Ruff is a Python linter and formatter aimed at replacing other tools like Flake8 and Black. It is still relatively new but is seeing a lot of adoption, including in several of our dependencies like Pydicom or scikit-learn.

The advantages of Ruff over Flake8 are these ones:

Speed, Ruff lints our entire codebase in less than a second.
IDE integration, Ruff has a first-party VSCode extension.
Configuration, Ruff uses the standard pyproject.toml file for its configuration, whereas Flake8 uses its own configuration file or requires an extension to use pyproject.toml.
Better lints, Ruff includes many lints natively, whereas Flake8 requires plugin for some lints (like sorting imports for instance).
Ruff as a more active development than Flake8 according to their commit history.

Pyright

Pyright is a static type checker for Python developed by Microsoft. It is an alternative to Mypy, the official community Python type checker. Both of these type checkers are good, and can in fact be used simultaneously. The advantages of Pyright are the following:

It is used by Pylance, the VSCode Python language server.
It has a better default configuration for our project IMO (return type inference !).
It is faster than Mypy (not important but still noticeable IMO).

Pytest

Pytest is a popular Python testing library, which we will use for our unit and integration tests. This PR does not add any test but adds it to our dependencies and provides a place to configure the tool.

Dependencies in `pyproject.toml`

pyproject.toml also also allows to configure a project dependencies (replacing requirements.txt). However, installing dependencies via pyproject.toml also requires to "build" the project (which is mostly copying the source files and putting a version on it as far as I understand it), which we do not really have any use for now IMO. There are some workarounds (Poetry notably) but they complicate the setup IMO. So I did not touch the dependencies management in this PR.

Dev dependencies vs prod dependencies

As is currently the case, all the tools are specified in the requirements.txt file, meaning there is no separation between the development and the production dependencies. This could be changed in the future but I do not think it is worth the (relatively simple but still existing) complexity for now.

Follow-up work

Replace Flake8 by Ruff in GitHub actions and remove the Flake8 dependency (EDIT: Done in this PR).

maximemulder commented 2 months ago

The strict configuration will use Pyright for now, but here is the equivalent Mypy configuration in case we want to change (or use both !).

[tool.mypy]
files = [
    "python/lib/db",
    "python/lib/exception",
]
strict = true

maximemulder commented 2 months ago

OKAY, I think this is done ! Not only does this PR configure the tooling, but it also adds GitHub actions related to this tooling. I made sure the outputs for Ruff and Pyright are formatted so that GitHub recognizes the errors, and shows them directly in the code. Deprecated calls are reported as warnings as we discussed. The dev and normal dependencies are not separated from the normal ones but this may be a subject for another PR (not urgent at all IMO).

Since our current Flake8 action checked for both Python 3.11 and 3.12, I did the same here. I can change that if you want a simplified workflow (it is nice that it may be theoretically more exhaustive and runs in parallel, but it also means errors can be reported twice).

maximemulder commented 2 months ago

Actually, I may have thought of a better way to do things this week-end (by containerizing the actions in a Docker image), let me see if I can succeed in another PR before merging.

aces / Loris-MRI