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

Unify LORIS-MRI logging #1191

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

maximemulder
Copy link
Contributor

@maximemulder maximemulder commented Sep 27, 2024

Description of the PR

This PR is a rewrite of the logging and I/O modules of LORIS-MRI.

Problems of the current architecture

There are several problems with the current logging and I/O architecture of LORIS6MRI:

  1. It is fragmented. The main class seems to be the Log class, and the BasePipeline or ImagingIO classes that build on top of it to provide better abstractions, with some amount of duplication between those two classes (some scripts also use print but they are not the subject of this PR).
  2. The BasePipeline class is too coupled with the DCM2BIDS pipeline to be generalized.
  3. The BasePipeline and ImagingIO classes handle both logging and file system operations, which are two different responsibilities.
  4. The logging abstractions are verbose, notably because all logging functions require two keyword arguments is_error = 'Y' | 'N' and is_verbose = 'Y' | 'N'.
  5. The logging functions sometimes uses stdout to print errors, whereas they should use stderr.
  6. The code does not integrate the latest LORIS-MRI architecture improvements, namely static typing and the SQLAlchemy integration.

Description of the new architecture

This PR adds the following:

  1. An lib.env.Env dataclass that stores generic information about a running script.
  2. The lib.logging module that handles log operations.
  3. The lib.file_system module that handles file system operations.
  4. The make_env function that allows to build an Env from a LorigGetOpt object (note that the environment does not directly depend on this object, it could also be built manually).

Those abstractions aim to replace the Log and ImagingIO classes, as well as the BasePipeline logging methods. They are script-agnostic, remove the existing duplication, are statically typed, use the SQLAlchemy abstractions, and provide more convenient interfaces IMO. They also use simple functions instead of classes, which I think is better.

Backwards compatibility

Every use of the old abstractions have been replaced by the new ones in the repo. The old abstractions are not removed by this PR and but are deprecated, to let some time for existing scripts to move to the new abstractions. Note that I used lib.logging in this PR, but I'd like to steal the lib.log namespace in the future once the old abstraction is removed.

Final notes

I am not an expert in logging, but I am not a fan of how logging is done in LORIS-MRI (printing AND writing to a log file that does not differentiate stdout and stderr AND inserting into the database on every print). I much prefer how the LORIS process manager does it, which is redirecting . Nevertheless, the current behaviour is conserved in this PR as it is only an architectural change, and I believe this PR is an improvement no matter how LORIS-MRI logging evolves in the future.

@maximemulder maximemulder force-pushed the 2024-09-27_new-env-log branch 7 times, most recently from aa54f55 to ef1632d Compare September 30, 2024 16:31
@maximemulder
Copy link
Contributor Author

Tested with batch_uploads_imageuploader.pl (dcm2niix) successfully.

@cmadjar cmadjar added this to the 27.0 milestone Oct 4, 2024
@maximemulder maximemulder added the S-Medium Size: Medium. Moderately-sized pull requests that are not too complex but not trivial either label Oct 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Refactoring S-Medium Size: Medium. Moderately-sized pull requests that are not too complex but not trivial either
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants