Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

LORIS-MRI tooling configuration #1170

Merged
merged 25 commits into from
Sep 13, 2024

Conversation

maximemulder
Copy link
Contributor

@maximemulder maximemulder commented Aug 30, 2024

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
Copy link
Contributor Author

maximemulder commented Sep 4, 2024

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 maximemulder force-pushed the 2024-08-26_add-pyproject branch from 5cf4673 to 2d639f8 Compare September 6, 2024 13:56
@maximemulder
Copy link
Contributor Author

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 maximemulder marked this pull request as ready for review September 6, 2024 17:15
@maximemulder
Copy link
Contributor Author

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.

@laemtl laemtl added the A-CI Area: CI. Issues and pull requests related to CI, including automated tests and static checks label Sep 13, 2024
@cmadjar cmadjar merged commit 72634dd into aces:main Sep 13, 2024
9 checks passed
@cmadjar cmadjar added this to the 27.0 milestone Sep 13, 2024
maximemulder added a commit to maximemulder-test-org/Loris-MRI that referenced this pull request Sep 13, 2024
* LORIS-MRI tooling configuration (aces#1170)

* add pyproject.toml

* keep flake8 (for now)

* double type checking

* Use Pyright as main type checker

* remove comment

* test

* test

* change configuration

* add python environment variables

* try factorization

* test

* test

* test

* test

* remove temporary things

* test ruff error

* ruff output github

* further test errors

* test pyright output github

* test

* test

* test

* change comment

* remove test errors

* test

---------

Co-authored-by: Maxime Mulder <[email protected]>

* update docker install (aces#1171)

Co-authored-by: Maxime Mulder <[email protected]>

* add RB override tables for the MRI side (aces#1173)

* Remove php 8.1 and 8.2 from the LORIS tests to keep only test on PHP 8.3 (aces#1175)

* remove php 8.1 and 8.2 testing

* update to 8.3 in Dockerfile.test.php8

* ADD minc tools install in gitactions (aces#1179)

* todo install loris mri

* test

* run pytest in gitaction (aces#1172)

* test

* ttt

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* test

* Delete test_example.py

* rename

* done

* Delete test/Dockerfile.test.py

* keep flake8 (for now)

* double type checking

* Use Pyright as main type checker

* test

* test

* change configuration

* add python environment variables

* try factorization

* test

* test

* test

* remove temporary things

* test ruff error

* ruff output github

* further test errors

* test pyright output github

* test

* test

* test

* change comment

* remove test errors

* test

* test

* test

* test

* test

* test

* test

* test cache

* test

* disable unused workflow

* test

* test

* test

* update repo owner in action

* try add permissions

* permissions test 2

* permissions check 3

* test

* other test

* use correct username

* change login

* try remove permission

---------

Co-authored-by: Maxime Mulder <[email protected]>
Co-authored-by: Cécile Madjar <[email protected]>
Co-authored-by: Shen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-CI Area: CI. Issues and pull requests related to CI, including automated tests and static checks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants