[MRG] Reduce logging for missing imports #2129
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2129 +/- ##
==========================================
+ Coverage 98.34% 98.38% +0.04%
==========================================
Files 77 77
Lines 13435 13435
==========================================
+ Hits 13212 13218 +6
+ Misses 223 217 -6 ☔ View full report in Codecov by Sentry. |
src/pydicom/pixels/utils.py
Outdated
| except Exception as exc: | ||
| LOGGER.exception(exc) | ||
| if config.debugging: | ||
| LOGGER.warning(exc) |
There was a problem hiding this comment.
Why not just use LOGGER.debug() here? Generally, I wonder why we need that config.debugging at all, if it just means the logging level is DEBUG.
There was a problem hiding this comment.
Mostly because you suggested warning 😛. Technically config.debugging just enables extra logging (especially in frequently used code), not necessarily debug logging.
One thing I'd like to do for 3.1 is improve the overall logging.
There was a problem hiding this comment.
Mostly because you suggested warning
Did I? Silly me...
Technically config.debugging just enables extra logging (especially in frequently used code), not necessarily debug logging.
Hm, isn't that what log levels are for? We don't use info at all, I think...
One thing I'd like to do for 3.1 is improve the overall logging.
🚀
| debugging = False | ||
|
|
||
|
|
||
| # force level=WARNING, in case logging default is set differently (issue 103) |
There was a problem hiding this comment.
I wanted to ask how you dug up a ten-year-old issue before I noticed that the code just had been moved :)
src/pydicom/pixels/utils.py
Outdated
| module = importlib.import_module(package_name, "__version__") | ||
| return tuple(int(x) for x in module.__version__.split(".")) >= minimum_version | ||
| except Exception as exc: | ||
| if config.debugging: |
There was a problem hiding this comment.
Still the same question: why do you need to check for config.debugging here (or elsewhere)?
There was a problem hiding this comment.
It's probably not as useful here, but in the data_element_generator code especially it's used to prevent logging in heavily used parts of the code base, presumably for efficiency.
There was a problem hiding this comment.
Thanks, that makes some sense, though I would still argue that this could be just debug logging.
There was a problem hiding this comment.
It's probably not as useful here, but in the
data_element_generatorcode especially it's used to prevent logging in heavily used parts of the code base, presumably for efficiency.
This. Could probably do it by checking the debug level as well, but actually calling LOGGING.debug() would be an "expensive" function call regardless of logging level. An if statement is much faster.
There was a problem hiding this comment.
Ah, I see. Actually, checking the debug level ist the first thing that the logger does, but it is of course more expansive than the if statement. Also, we would have to make sure, that we don't do anything expansive in the log message that will evaluated during the call.
So this is actually an optimization that can be used in critical code paths.
Describe the changes
pixelsplugin imports are missing.Tasks