Unify LORIS-MRI logging #1191
Open
Unify LORIS-MRI logging #1191
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maximemulder
force-pushed
the
2024-09-27_new-env-log
branch
7 times, most recently
from
aa54f55
to
ef1632d
Compare
maximemulder
added
the
S-Medium
maximemulder
force-pushed
the
2024-09-27_new-env-log
branch
2 times, most recently
from
a157f00
to
5aae545
Compare
|
Tested using If you want to inspect the diff yourself, here is what I got: Main: This PR: I used the following scripts to uniformize the timestamps across the diffs: #!/bin/sh
cp main.txt main_copy.txt
cp pr.txt pr_copy.txt
sed -i -e 's/10-5-Wl4f_6/10-00-abcdef/g' main_copy.txt
sed -i -e 's/10-15-d5v5D4/10-00-abcdef/g' pr_copy.txt
sed -i -e 's/10h06m53s_hv0qp2_x/10h00m00s_abcdefgh/g' main_copy.txt
sed -i -e 's/10h08m15s_isxuj3lf/10h00m00s_abcdefgh/g' main_copy.txt
sed -i -e 's/10h16m19s_szr6cbl0/10h00m00s_abcdefgh/g' pr_copy.txt
sed -i -e 's/10h17m12s_yyfmqxed/10h00m00s_abcdefgh/g' pr_copy.txtThe changes that remain are mostly:
I also checked that the log file exists in the LORIS log directory and that everything was logged in the database. |
maximemulder
force-pushed
the
2024-09-27_new-env-log
branch
from
5aae545
to
451bc40
Compare
|
Tested it with Main: This PR: |
maximemulder
force-pushed
the
2024-09-27_new-env-log
branch
from
9e90906
to
7957846
Compare
cmadjar
requested changes
Nov 12, 2024
There was a problem hiding this comment.
@maximemulder testing this branch failed on a fresh DB install because there is no row yet in the notification_types table and it needs to be created if no rows are found.
|
|
||
| notification_session = Session(self.db_engine) | ||
| notification_type_name = f'PYTHON {self.script_name.replace("_", " ").upper()}' | ||
| notification_type = get_notification_type_with_name(self.db, notification_type_name) |
There was a problem hiding this comment.
This returns an error on a fresh DB install because there is no row yet for that kind of notification so if that happens, it needs to be created.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 LORIS-MRI:
Logclass. TheBasePipelineandImagingIOclasses build on top ofLogprovide better abstractions, with some amount of duplication between those two classes (some scripts simply useprintbut they are not the subject of this PR).BasePipelineclass is too coupled with the DCM2BIDS pipeline to be generalized.BasePipelineandImagingIOclasses 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.Envdataclass that stores generic information about a running script.lib.loggingmodule that handles log operations.lib.file_systemmodule that handles file system operations.make_envfunction that allows to build anEnvfrom aLorigGetOptobject (note that the environment does not directly depend onLorisGetOpt, it could also be built manually).Those abstractions aim to replace the
LogandImagingIOclasses, as well as theBasePipelinelogging 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 for simplicity, reusability and testability.Backwards compatibility
This PR is backwards compatible as the old abstractions are not replaced by this PR. They are however annotated as depcreated to let some time for external scripts to move to the old abstractions. All the uses of the old abstractions in LORIS-MRI are replaced by the new abstractions in this PR, so the deprecated files remain only for external scripts compatibility.
Ultimately (once the migration is finished), I would like the new logging module to take the
lib.lognamespace though (the same way I would like the new database module to use thelib.databasemodule).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 stdout and stderr to some files, and saving these files paths in the database. 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.