Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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:
Log
class, and theBasePipeline
orImagingIO
classes that build on top of it to provide better abstractions, with some amount of duplication between those two classes (some scripts also useprint
but they are not the subject of this PR).BasePipeline
class is too coupled with the DCM2BIDS pipeline to be generalized.BasePipeline
andImagingIO
classes handle both logging and file system operations, which are two different responsibilities.is_error = 'Y' | 'N'
andis_verbose = 'Y' | 'N'
.Description of the new architecture
This PR adds the following:
lib.env.Env
dataclass that stores generic information about a running script.lib.logging
module that handles log operations.lib.file_system
module that handles file system operations.make_env
function that allows to build anEnv
from aLorigGetOpt
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
andImagingIO
classes, as well as theBasePipeline
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 thelib.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.