-
Notifications
You must be signed in to change notification settings - Fork 34
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
set up file logger with timestamp #920
base: master
Are you sure you want to change the base?
Conversation
@@ -50,7 +50,7 @@ | |||
# Ensure segmentation logging information is included in this example's output | |||
root = logging.getLogger() | |||
root.setLevel(logging.ERROR) | |||
logging.getLogger('AFQ').setLevel(logging.INFO) | |||
logging.getLogger(__name__).setLevel(logging.INFO) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unless really necessary, it'd be better not to redefine the logger level in a file like that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an example, so its showing what the user can do. So I think it's good to show the getlogger and setlevel stuff. This file is not imported by any other file so it shouldn't affect anything else. That said I am not sure why there is this root logger thing.
Example log file generated with current config:
I think we should add subject/session for each entry. Also currently, if the pipeline works on a few subjects but not all in the |
I think this PR looks good to go as is, but there are a few changes we can do here or in another PR. We can remove these two lines from the example:
For the idea of printing subject/session for each log output, I think that is good and could be handled in a separate PR or posted as an issue. We could add a handler to the logger before running for a given subject, then remove it afterwards. Here is an example of adding a prefix to the logger object: https://stackoverflow.com/questions/28994448/how-to-add-a-prefix-to-an-existing-python-logging-formatter . I think we could do it in Line 431 in a44d570
(not sure how to handle the parallel case) For some subjects not completing with export_all, I am not sure what you mean. If any subject has a fatal error the whole pipeline should stop. @pierre-nedelec if you rebase this PR on master we can merge this PR. |
@pierre-nedelec : if you rebase this, we can merge this to include in the upcoming release. |
Pushing this to the next 1.2 release (which hopefully will be soon too!) |
Fixes part of #914 by adding the ability to log to file and have timestamps.
Next potential steps: