Skip to content

Replace logging with loguru #467

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 22 commits into from
Apr 2, 2025
Merged

Replace logging with loguru #467

merged 22 commits into from
Apr 2, 2025

Conversation

lochhh
Copy link
Collaborator

@lochhh lochhh commented Mar 4, 2025

This PR supersedes #412

Description

What is this PR

  • Bug fix
  • Addition of a new feature
  • Other

Why is this PR needed?
This PR closes #337.

What does this PR do?

Custom loguru-based MovementLogger

This PR replaces the use of the logging module with loguru. It extends loguru's logger with a custom MovementLogger that overrides logger.error and logger.exception to return the Exception being logged. This means that we can raise e.g. err = ValueError("message") with

raise logger.error(err)

instead of

logger.error(err)
raise err

By default, the logger is configured to log to a rotating log file (at DEBUG level) and to stderr (at WARNING level). One can also turn off logging to stderr with logger.configure(console=False).

Standardised logging calls

All logging calls are now standardised such that we no longer have special syntaxes for logging warnings and errors:

  • instead of logging warnings and errors with the custom log_warning, log_error, we now use the same syntax (logger.<method>() to log at various levels, e.g. logger.error(...), logger.info(...)).

Single test log file

When running tests, messages are now logged to a single movement-test.log file configured at the beginning of each pytest session, instead of creating a new log file for each test.

Integration with warnings module

Messages emitted by the warnings module are also redirected to the logger to additionally be logged using logger.warning() by replacing warnings.showwarning with a custom showwarning.

print vs warnings vs logger

This PR also ensures the appropriate use of print, warnings.warn, logger.<method> according to the when to use logger/print/warnings guide . To distinguish between logger.warning() and warnings.warn(), the former is used when something is optional and we assign default values (e.g. individual names, keypoint names, confidence values); the latter is used when the input from the user is invalid and can be fixed by the user within movement (e.g. deprecated function calls, invalid fps number, single individual name provided as str instead of list[str]) or when applying filters to data containing excessive NaNs, which can potentially be addressed using movement's interpolate.

References

#337

How has this PR been tested?

Tests passing on CI and locally

Checklist:

  • The code has been tested locally
  • Tests have been added to cover all new functionality
  • The documentation has been updated to reflect any changes
  • The code has been formatted with pre-commit

Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (d2cd4e6) to head (0f20101).
Report is 3 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #467      +/-   ##
===========================================
+ Coverage   99.87%   100.00%   +0.12%     
===========================================
  Files          28        28              
  Lines        1559      1549      -10     
===========================================
- Hits         1557      1549       -8     
+ Misses          2         0       -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@lochhh lochhh force-pushed the refactor-logging branch 4 times, most recently from 80a692d to 93fbbc7 Compare March 11, 2025 12:31
@lochhh lochhh marked this pull request as ready for review March 11, 2025 14:55
@lochhh lochhh requested a review from a team March 11, 2025 14:55
Copy link
Member

@niksirbi niksirbi left a comment

Choose a reason for hiding this comment

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

Thanks @lochhh!

I tried using movement through Python console and jupyter notebooks, while also keeping an eye on the log file, and this PR seems to work as advertised. Also kudos for maintaining a somewhat backwards-consistent way for us to raise errors and warnings (the syntax is very similar to what we had before).

I think this is good to go, barring a few not-so-important comments I've left.
Also make sure to rebase to main so the changes in logging also apply to recently added stuff. One case which might be tricky is the deprecation warning I added in #455 (in filtering.py). Ideally we want to use the new syntax for it but also ensure that specifically a DeprecationWarning is raised.

@lochhh lochhh force-pushed the refactor-logging branch from 8dea9c1 to bda1c1e Compare March 28, 2025 11:18
@lochhh lochhh force-pushed the refactor-logging branch 2 times, most recently from 2e54f6a to 1ebca34 Compare March 28, 2025 17:01
@lochhh lochhh force-pushed the refactor-logging branch 6 times, most recently from 1afc664 to 8b2c391 Compare March 31, 2025 16:57
@lochhh lochhh force-pushed the refactor-logging branch from c40f836 to b1c89e5 Compare April 1, 2025 10:14
@lochhh
Copy link
Collaborator Author

lochhh commented Apr 1, 2025

Thanks @lochhh!

I tried using movement through Python console and jupyter notebooks, while also keeping an eye on the log file, and this PR seems to work as advertised. Also kudos for maintaining a somewhat backwards-consistent way for us to raise errors and warnings (the syntax is very similar to what we had before).

I think this is good to go, barring a few not-so-important comments I've left. Also make sure to rebase to main so the changes in logging also apply to recently added stuff. One case which might be tricky is the deprecation warning I added in #455 (in filtering.py). Ideally we want to use the new syntax for it but also ensure that specifically a DeprecationWarning is raised.

Thanks @niksirbi. warnings.warn() messages are now redirected to the logger. I think it's worth keeping both logger.warning() and warnings.warn() for different purposes, so I've replaced some print and logger.warning calls with warnings.warn (see updated PR description sections: Integration with warnings module, print vs warnings vs logger).

@lochhh lochhh requested a review from niksirbi April 1, 2025 17:51
@niksirbi
Copy link
Member

niksirbi commented Apr 1, 2025

Thanks @niksirbi. warnings.warn() messages are now redirected to the logger. I think it's worth keeping both logger.warning() and warnings.warn() for different purposes, so I've replaced some print and logger.warning calls with warnings.warn (see updated PR description sections: Integration with warnings module, print vs warnings vs logger).

Thanks @lochhh. I think you did a great job with this. I benefitted a lot from reading your updated PR description, because when to use logger/warnings/print was not entirely clear in my mind. I have only one final request to make: could you copy some of that information over to the contributing guide? It would be good to have a canonical place to point contributors to, when we inevitably have to explain this again.

Copy link

sonarqubecloud bot commented Apr 2, 2025

@lochhh
Copy link
Collaborator Author

lochhh commented Apr 2, 2025

Thanks @niksirbi. warnings.warn() messages are now redirected to the logger. I think it's worth keeping both logger.warning() and warnings.warn() for different purposes, so I've replaced some print and logger.warning calls with warnings.warn (see updated PR description sections: Integration with warnings module, print vs warnings vs logger).

Thanks @lochhh. I think you did a great job with this. I benefitted a lot from reading your updated PR description, because when to use logger/warnings/print was not entirely clear in my mind. I have only one final request to make: could you copy some of that information over to the contributing guide? It would be good to have a canonical place to point contributors to, when we inevitably have to explain this again.

Added a section describing this in Development guidelines

@niksirbi niksirbi added this pull request to the merge queue Apr 2, 2025
Merged via the queue into main with commit 956faf3 Apr 2, 2025
26 checks passed
@lochhh lochhh deleted the refactor-logging branch April 2, 2025 21:15
niksirbi added a commit that referenced this pull request Apr 8, 2025
* Replace `logging` with `loguru`

* Combine consecutive `logger.info` calls

* Configure console and file handler for logger

* Use default stderr handler

* Refactor logging and its tests

* Use loguru default format for logging

* Simplify tests logger setup

* Clean up log_warning calls

* Use custom MovementLogger

* Remove commented out code

* Fix logger repr test

* Apply test docstring suggestions from code review

Co-authored-by: Niko Sirmpilatze <[email protected]>

* Reduce number of log files kept at rotation

* Fix up rebase error

* Redirect warnings to logger

* Clarify caplog fixture

* Refactor tests

* Use default `enqueue=False`

* Emit and log warning in `_warn_about_nan_proportion`

* Emit warning when converting str to list[str]

* Emit warning when setting fps to None

* Add logging section to contributing guide

---------

Co-authored-by: Niko Sirmpilatze <[email protected]>
ShigrafS added a commit to ShigrafS/movement that referenced this pull request Apr 14, 2025
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.

Add console handler for logging
2 participants