-
Notifications
You must be signed in to change notification settings - Fork 2
Enhance logging #346
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
Enhance logging #346
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enhances COMPASS logging to provide critical execution information that can be accessed when running on Kubernetes, where real-time log streaming is not available. The changes add version logging, execution parameter logging, processing step information, and improved exception handling with logging to file.
Key Changes
- Added
log_versions()function to log COMPASS and key dependency versions at INFO and DEBUG_TO_FILE levels - Added execution parameter and processing step logging via
_log_exec_info()and_check_enabled_steps()functions - Enhanced exception handling to log fatal errors during processing to file for debugging
- Introduced
[NOT PUBLIC API]docstring marker to exclude internal functions from Sphinx documentation
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| compass/utilities/logs.py | Added log_versions() function to log package versions; renamed _setup_logging_levels() to setup_logging_levels() with [NOT PUBLIC API] marker; added _get_version() helper |
| compass/scripts/process.py | Added logging of execution parameters, processing steps, and jurisdiction status; improved exception handling to log fatal errors; added _log_exec_info() and _check_enabled_steps() helper functions |
| compass/utilities/parsing.py | Added convert_paths_to_strings() utility function to convert Path objects to strings for JSON serialization |
| compass/init.py | Updated import to use renamed setup_logging_levels() function |
| docs/source/conf.py | Refactored autodoc skip logic into separate functions (_skip_pydantic_methods, _skip_builtin_methods, _skip_internal_api) for better organization and added support for [NOT PUBLIC API] marker |
| docs/source/dev/README.rst | Added documentation for the new [NOT PUBLIC API] docstring marker convention |
| .github/copilot-instructions.md | Updated guidelines to include [NOT PUBLIC API] marker usage and test file execution pattern |
| tests/python/unit/utilities/test_utilities_logs.py | Added test for log_versions() function; updated test imports for renamed function; added if __name__ block |
| tests/python/unit/scripts/test_process.py | New comprehensive test file covering execution logging, error handling, and processing step validation |
| tests/python/unit/test_version.py | Updated version regex to support additional setuptools_scm format with dirty date suffix; added if __name__ block |
| tests/python/unit/test_pb.py | Added if __name__ block for direct test execution |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #346 +/- ##
==========================================
+ Coverage 43.32% 51.55% +8.23%
==========================================
Files 45 45
Lines 4240 4300 +60
Branches 380 391 +11
==========================================
+ Hits 1837 2217 +380
+ Misses 2378 2052 -326
- Partials 25 31 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
castelao
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Thanks!
Somehow related. Although these extra log information would be great, I'm inclined to hold any container release until the end of the solar runs so we have consistency.
So you are suggesting we wait to merge this PR until the runs are done? |
No, I suggest we move ahead and merge it, but hold a few days for the next release. |
A lot of the existing logging assumed you could see the log streaming in real time on the command line. However, we don't have such a luxury when running on k8's, so a lot of critical execution information was not given to the user. In this PR, we aim to somewhat rectify this buy logging basic execution parameters to help debug any problems.