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

[REF] Move logger management to new functions #750

Merged
merged 10 commits into from
Jul 22, 2021
Merged

Conversation

tsalo
Copy link
Member

@tsalo tsalo commented Jul 12, 2021

Closes #719.

Changes proposed in this pull request:

  • Add new functions to set up and tear down loggers, to be called at beginning and end of workflows, respectively.
  • Do not propagate REPORT and REFERENCES messages to the stream. This reduces clutter in the terminal.
  • Replace file-specific loggers with a general one (GENERAL).

The changes to the loggers doesn't show up in the CI, because pytest's logging is different from the logging set up in the workflows... but it looks good when run locally! Here's a snippet of my Terminal output from a local run:

INFO     tedana:tedana_workflow:463 Computing EPI mask from first echo
INFO     tedana:tedana_workflow:475 Computing T2* map
INFO     combine:make_optcom:225 Optimally combining data with voxel-wise T2* estimates
INFO     gscontrol:gscontrol_raw:50 Applying amplitude-based T1 equilibration correction

@tsalo tsalo marked this pull request as draft July 12, 2021 19:09
@codecov
Copy link

codecov bot commented Jul 12, 2021

Codecov Report

Merging #750 (bebeb13) into main (d449925) will increase coverage by 0.08%.
The diff coverage is 96.55%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #750      +/-   ##
==========================================
+ Coverage   92.61%   92.69%   +0.08%     
==========================================
  Files          27       27              
  Lines        2125     2121       -4     
==========================================
- Hits         1968     1966       -2     
+ Misses        157      155       -2     
Impacted Files Coverage Δ
tedana/decomposition/_utils.py 0.00% <0.00%> (ø)
tedana/workflows/parser_utils.py 94.73% <ø> (-1.27%) ⬇️
tedana/utils.py 97.38% <97.05%> (-0.12%) ⬇️
tedana/combine.py 91.42% <100.00%> (ø)
tedana/decay.py 97.24% <100.00%> (ø)
tedana/decomposition/ica.py 82.75% <100.00%> (ø)
tedana/decomposition/pca.py 86.66% <100.00%> (ø)
tedana/gscontrol.py 100.00% <100.00%> (ø)
tedana/io.py 93.04% <100.00%> (ø)
tedana/metrics/_utils.py 67.34% <100.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d449925...bebeb13. Read the comment docs.

It's unnecessary now that we're using a single LGR for general logging.
The issue is that pytest's handler is different from our own.
@tsalo tsalo marked this pull request as ready for review July 12, 2021 21:07
@emdupre
Copy link
Member

emdupre commented Jul 20, 2021

The output logs look reasonable in the latest artifacts ! Just confirming that when running locally:

INFO     GENERAL:tedana.py:367 Using output directory: /tmp/data/five-echo/TED.five-echo

updates to

INFO     tedana:tedana_workflow:367 Using output directory: /tmp/data/five-echo/TED.five-echo

is that right ?

Assuming so, is there anything specific you'd like feedback on here ?

@tsalo
Copy link
Member Author

tsalo commented Jul 20, 2021

@emdupre yeah, that looks right! There's nothing in particular that I'm worried about, so any review of any part of it would be appreciated.

Copy link
Collaborator

@eurunuela eurunuela left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Collaborator

@jbteves jbteves left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, this cleans up quite a bit!

@tsalo tsalo merged commit d5cbf86 into ME-ICA:main Jul 22, 2021
@tsalo tsalo deleted the logger-cleanup branch July 22, 2021 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move logger management into new functions
4 participants