Skip to content

MAINT: fix interaction with external logging #2611

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

Merged
merged 3 commits into from
Jul 2, 2018
Merged

Conversation

mgxd
Copy link
Member

@mgxd mgxd commented Jun 6, 2018

Fixes #2569 .

Changes proposed in this pull request

  • remove call to logging.basicConfig
  • scope sub-loggers (interface, workflow, etc) within nipype ancestor

@codecov-io
Copy link

codecov-io commented Jun 6, 2018

Codecov Report

Merging #2611 into master will increase coverage by 0.03%.
The diff coverage is 94.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2611      +/-   ##
==========================================
+ Coverage   67.59%   67.63%   +0.03%     
==========================================
  Files         339      339              
  Lines       42814    42855      +41     
  Branches     5288     5296       +8     
==========================================
+ Hits        28941    28984      +43     
+ Misses      13191    13187       -4     
- Partials      682      684       +2
Flag Coverage Δ
#smoketests 50.77% <92%> (ø) ⬆️
#unittests 65.12% <93.15%> (+0.03%) ⬆️
Impacted Files Coverage Δ
nipype/utils/config.py 66.84% <0%> (ø) ⬆️
nipype/algorithms/mesh.py 30.39% <100%> (ø) ⬆️
nipype/interfaces/dipy/anisotropic_power.py 41.66% <100%> (ø) ⬆️
nipype/pipeline/engine/workflows.py 79.08% <100%> (ø) ⬆️
nipype/interfaces/cmtk/cmtk.py 18.65% <100%> (ø) ⬆️
nipype/algorithms/metrics.py 58.35% <100%> (ø) ⬆️
nipype/interfaces/elastix/registration.py 52.25% <100%> (ø) ⬆️
nipype/interfaces/afni/base.py 62.9% <100%> (ø) ⬆️
nipype/pipeline/plugins/oar.py 54.54% <100%> (ø) ⬆️
nipype/pipeline/plugins/slurm.py 16.88% <100%> (ø) ⬆️
... and 54 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 cd5567b...765521c. Read the comment docs.

@@ -513,7 +513,7 @@ def _fetch_bucket(self, bucket_name):

# Init variables
creds_path = self.inputs.creds_path
iflogger = logging.getLogger('interface')
iflogger = logging.getLogger('nipype.interface')
Copy link
Member

Choose a reason for hiding this comment

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

I would actually remove this line, as well as the others (L590, L658), as they're overriding a file-level variable, but not changing it.

Copy link
Member Author

Choose a reason for hiding this comment

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

nice catch, fixed.

@effigies
Copy link
Member

effigies commented Jun 8, 2018

Is there some way to test this automatically? Or at least post a demonstration that @jondeaton's replicating code now works as expected?

@mgxd
Copy link
Member Author

mgxd commented Jun 8, 2018

@effigies any recommended way to check logging output? I mocked up an example with unittest.Testcase.assertLogs but was passing for both pre/post fix.

@jondeaton I tested this with your minimal example

import logging
from nipype.interfaces.ants import N4BiasFieldCorrection
logger = logging.getLogger('root')
logger.addHandler(logging.StreamHandler())
logger.warning("This should really only be appearing once...")

but perhaps you would like to check out this branch and verify this fix.

@effigies
Copy link
Member

effigies commented Jun 8, 2018

@effigies any recommended way to check logging output? I mocked up an example with unittest.Testcase.assertLogs but was passing for both pre/post fix.

No, sorry, I don't have any experience here. It would just be nice to have some confirmation.

@effigies effigies merged commit f79581e into nipy:master Jul 2, 2018
effigies added a commit that referenced this pull request Jul 2, 2018
#2598 did not incorporate #2611's move to hierarchical loggers.

This is likely to be a breaking change for a lot of downstream users, so we should make a particular note of it in the release notes.
@effigies effigies added this to the 1.1.0 milestone Aug 30, 2018
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.

3 participants