Skip to content

Conversation

@garrettwrong
Copy link
Collaborator

Closes #212 .

Most of the competing logging implementation was removed in #283 , so this PR isn't too big, nice.

  • Converts logger assignments to __file__. We may have one specific place it would be reasonable to deviate from that convention that I am aware of (the tutorials), but it is not necessary the way I have implemented logging here (timestamped logfiles).
  • Convert to config file style from a a dictionary style. This moves configuration out of the source code.
  • Removes duplicated logging config in pytest.ini, also fixes the stdout out duplication under pytest -s.
  • Adds fileHandler with configurable filename.
  • Tee stream, >=DEBUG to file and >=INFO to console.
  • ISO datetime format in log, and close to it for filename (: presents a problem, replaced with -)
  • add levelname

One point of discussion is regarding the default log filename. This initial PR will drop a timestamped file name for each process where aspire package is imported. This is pretty common, but can be a little noisy. That is, if you are frantically developing and running tests, you might get many small files in your directory to rm *.log, instead of one big one...

Alternatively it is possible to turn off writing the file by default, and use a simple bool option like log_to_file in config.ini. This would shield users from the python logging configuration, which might seem complicated. (It isn't but certainly has nothing to do with CryoEM). This is easy to implement if desired.

As it stands the log filenames look like aspire-2020-10-08T14-50-13.456577.log, with a snippet example attached showing the new format.
ex.log

@garrettwrong garrettwrong added documentation Improvements or additions to documentation enhancement New feature or request cleanup labels Oct 8, 2020
@garrettwrong garrettwrong self-assigned this Oct 8, 2020
@garrettwrong
Copy link
Collaborator Author

I went through error, warning, and info messages. Made just a few adjustments (mainly errors that should also raise). I also updated the developer guide a little based on this PR.

The only point keeping this PR a draft is regarding the log file default. Mainly do we want multiple (individually timestamped) log files, a single log file, or no file as the default of ASPIRE. @janden do you have any preference? Currently the PR has the multiple timestamped setup. Thanks

@janden
Copy link
Collaborator

janden commented Oct 13, 2020

I would personally prefer to not log to file by default, but this will be very useful for bug reports. Timestamped log files I think are good for that reason too. Would a reasonable compromise be for log files to end up in a subdirectory (like logs/)?

@garrettwrong
Copy link
Collaborator Author

Sure I'll set that up, thanks.

@garrettwrong garrettwrong marked this pull request as ready for review October 13, 2020 17:48
@garrettwrong garrettwrong requested a review from janden as a code owner October 13, 2020 17:48
@garrettwrong garrettwrong merged commit 622dd97 into develop Oct 15, 2020
@garrettwrong garrettwrong mentioned this pull request Oct 15, 2020
@garrettwrong garrettwrong deleted the refresh_logging branch October 24, 2020 02:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cleanup documentation Improvements or additions to documentation enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants