Fixes ignored --loglevel by adding force=True when configuring with basicConfig. #800

Lucas-Steinmann · 2023-02-01T15:29:27Z

Setting force=True removes any existing handlers from the root logger before configuring it.
If not specified and any handlers have been set before the call to basicConfig, then the changes are not applied.
During the import process, there are other calls to logging.basicConfig. Specifically, when tensorflow is not installed, then this line calls log.debug, which in turn calls log.basicConfig.

Since the call to basicConfig in datumaro/cli/__main__.py is the only deliberate call to basicConfig, it should be forced and overwrite any other inadvertent config changes.

Summary

Resolves #627

Fixes bug, which prevented setting of log level when calling CLI tool.

How to test

Before this patch is applied setting --logleveldid nothing (atleast if tensorflow is not installed, haven't tested with tf).
To test it just check if calling datum --loglevel debug <any command> works before and after applying this test.

Checklist

I submit my changes into the develop branch
I have added description of my changes into CHANGELOG
I have updated the documentation accordingly
I have added tests to cover my changes
I have linked related issues

License

I submit my code changes under the same MIT License that covers the project.
Feel free to contact the maintainers if that's a concern.
I have updated the license header for each file (see an example below)

# Copyright (C) 2021 Intel Corporation
#
# SPDX-License-Identifier: MIT

Setting force=True removes any existing handlers from the root logger before configuring it. If not specified, the changes are not applied. During the import process there are other calls to logging.basicConfig. Specificially, when tensorflow is not installed, then datumaro/components/extractor_tfds.py:26 calls log.debug, which in turn calls log.basicConfig. Since the call to basicConfig in datumaro/cli/__main__.py is the only deliberate call to basicConfig, it should be forced and overwrite any other inadvertent config changes.

vinnamkim

Hi @Lucas-Steinmann, thank you for contributing to this project. It looks good to me but there is a formatting problem. Would you please fix it?

vinnamkim

Some failures are found in our unit tests.

By introducing force=True to log.basicConfig in a previous commit, log capturing handlers which have been added by the unit tests are removed. This commit implements a less drastical way to configure logging. Instead of removing all handlers, this simply sets the log level and overwrites the formatter. In doing so the CapturingHandlers of the unit tests are preserved, but the log level as well as the format is still set. This is a compromise, since it tries to handle cases where logging is already configured but still does some changes to the logging behaviour. It would be laborious to anticipate every possible previous log configuration. Hence, this commit only tries to fix the special case to make unit tests similar to tests/cli/test_video::VideoTest.test_can_display_video_import_warning_in_import compatible.

Lucas-Steinmann · 2023-02-03T00:12:02Z

I've fixed the formatting and I've revisited the tests.

The main problem is that the tests overwrite the logging configuration.
I guess unittests's assertLog was not written with the target in mind to run integration tests.
This is not meant as a critique, since I don't have a better idea on how to check the logging behavior.

I've tried to find a compromise, by not deleting handlers and instead only overwriting log level and formatters.
I'm not super happy with this, since this now tries to support some kind of prior modification to the logging configuration,
but cannot anticipate all possible prior modifications.

codecov-commenter · 2023-02-03T00:53:51Z

Codecov Report

Base: 84.01% // Head: 83.95% // Decreases project coverage by -0.07% ⚠️

Coverage data is based on head (f636c59) compared to base (15c5926).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #800      +/-   ##
===========================================
- Coverage    84.01%   83.95%   -0.07%     
===========================================
  Files          174      174              
  Lines        22956    22927      -29     
  Branches      4985     4971      -14     
===========================================
- Hits         19287    19248      -39     
- Misses        2546     2552       +6     
- Partials      1123     1127       +4

Flag	Coverage Δ
macos-11_Python-3.10	`83.86% <100.00%> (-0.06%)`	⬇️
macos-11_Python-3.7	`83.55% <100.00%> (-0.06%)`	⬇️
macos-11_Python-3.8	`83.79% <100.00%> (-0.06%)`	⬇️
macos-11_Python-3.9	`83.81% <100.00%> (-0.06%)`	⬇️
ubuntu-20.04_Python-3.10	`83.87% <100.00%> (-0.06%)`	⬇️
ubuntu-20.04_Python-3.7	`83.56% <100.00%> (-0.07%)`	⬇️
ubuntu-20.04_Python-3.8	`83.81% <100.00%> (-0.06%)`	⬇️
ubuntu-20.04_Python-3.9	`83.82% <100.00%> (-0.06%)`	⬇️
windows-2019_Python-3.10	`83.83% <100.00%> (-0.06%)`	⬇️
windows-2019_Python-3.7	`83.52% <100.00%> (-0.07%)`	⬇️
windows-2019_Python-3.8	`83.75% <100.00%> (-0.06%)`	⬇️
windows-2019_Python-3.9	`83.77% <100.00%> (-0.06%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
datumaro/cli/__main__.py	`88.77% <100.00%> (+0.47%)`	⬆️
datumaro/plugins/data_formats/cvat/base.py	`91.47% <0.00%> (-4.62%)`	⬇️
datumaro/plugins/splitter.py	`88.47% <0.00%> (-0.23%)`	⬇️
datumaro/plugins/data_formats/imagenet_txt.py	`86.56% <0.00%> (ø)`
datumaro/plugins/data_formats/mpii/mpii_mat.py	`79.06% <0.00%> (ø)`
...envino_plugin/samples/ssd_face_detection_interp.py	`10.63% <0.00%> (ø)`
...vino_plugin/samples/ssd_person_detection_interp.py	`10.63% <0.00%> (ø)`
...ino_plugin/samples/ssd_vehicle_detection_interp.py	`10.63% <0.00%> (ø)`
...gin/samples/ssd_mobilenet_coco_detection_interp.py	`11.76% <0.00%> (ø)`
...amples/ssd_person_vehicle_bike_detection_interp.py	`10.20% <0.00%> (ø)`

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

vinnamkim · 2023-02-03T01:47:19Z

I've fixed the formatting and I've revisited the tests.

The main problem is that the tests overwrite the logging configuration. I guess unittests's assertLog was not written with the target in mind to run integration tests. This is not meant as a critique, since I don't have a better idea on how to check the logging behavior.

I've tried to find a compromise, by not deleting handlers and instead only overwriting log level and formatters. I'm not super happy with this, since this now tries to support some kind of prior modification to the logging configuration, but cannot anticipate all possible prior modifications.

It seems that the previous implementation (force=true) removes an existing logging handler and create a new one. Therefore, self.assertLogs() stores a reference of the handler which will be removed by (force=true) subsequently. I think that this implementation not to remove the handler but modify it with our purpose is better than before.

vinnamkim

LGTM.

Lucas-Steinmann added 2 commits February 1, 2023 16:09

Add changes to changelog.

c1d294c

Lucas-Steinmann changed the title ~~Adds force=True when configuring loglevel.~~ Fixes ignored --loglevel by adding force=True when configuring with basicConfig. Feb 1, 2023

vinnamkim approved these changes Feb 2, 2023

View reviewed changes

vinnamkim added the BUG Something isn't working label Feb 2, 2023

vinnamkim suggested changes Feb 2, 2023

View reviewed changes

Lucas-Steinmann added 2 commits February 2, 2023 23:51

Formatting.

a9adb44

Fix formatting in markdown.

f636c59

vinnamkim approved these changes Feb 3, 2023

View reviewed changes

vinnamkim merged commit 0a96e0c into open-edge-platform:develop Feb 3, 2023

vinnamkim added this to the 1.0.0 milestone Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes ignored --loglevel by adding force=True when configuring with basicConfig. #800

Fixes ignored --loglevel by adding force=True when configuring with basicConfig. #800

Uh oh!

Lucas-Steinmann commented Feb 1, 2023 •

edited

Loading

Uh oh!

vinnamkim left a comment

Uh oh!

vinnamkim left a comment

Uh oh!

Lucas-Steinmann commented Feb 3, 2023

Uh oh!

codecov-commenter commented Feb 3, 2023 •

edited

Loading

Uh oh!

vinnamkim commented Feb 3, 2023 •

edited

Loading

Uh oh!

vinnamkim left a comment

Uh oh!

Uh oh!

Fixes ignored --loglevel by adding force=True when configuring with basicConfig. #800

Fixes ignored --loglevel by adding force=True when configuring with basicConfig. #800

Uh oh!

Conversation

Lucas-Steinmann commented Feb 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How to test

Checklist

License

Uh oh!

vinnamkim left a comment

Choose a reason for hiding this comment

Uh oh!

vinnamkim left a comment

Choose a reason for hiding this comment

Uh oh!

Lucas-Steinmann commented Feb 3, 2023

Uh oh!

codecov-commenter commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

vinnamkim commented Feb 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vinnamkim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Lucas-Steinmann commented Feb 1, 2023 •

edited

Loading

codecov-commenter commented Feb 3, 2023 •

edited

Loading

vinnamkim commented Feb 3, 2023 •

edited

Loading