Adding logging

[ehsteve]

This PR replaces all usage of print with messages to logs. It adds the ability to configure how logging works as well. To do this I had to add two new dependencies.

One thing I could not figure out is how to capture uncaught exceptions to the log file which is something many people want but is not made available through the standard logging module. There are work arounds, for example, the AstropyLogger does it.

Addresses issue #137.

[ddasilva]

Hi @ehsteve, happy holidays! Thanks for opening this pull request

I think this is a good idea but there are a couple design constraints I want to discuss. I think that libraries of this size should avoid managing their own logging settings (which 99% of the time would be a duplication of configuration), and that this should generally be left to the client code. And as a side-effect of that, I think we can make this simpler and cut library-level config files.

This is the design that I'd recommend:

import ccsdspy

ccsdspy.use_logging()    # tells the library to use logging module instead of print statements
configure_logger()   # my own function
# go on with doing my main program

I suspect the reason you configured logging in CCSDSPy was because we still need messages printed if no logging is configured. If no one calls use_logging(), we can fall back to print statements like the following, which can be colored by log level message. Adding colors will be a nice improvement for people who don't use logging. Here is an example of ccsdspy/logging.py:

import logging
from termcolor import cprint

logger = None

def use_logging():
    global logger
    logger = logging.getLogger("ccsdspy") 

def log_warn(msg):
    if logger:
       logging.warn(msg)
    else:
       cprint(msg, color="yellow")

def log_error(msg):
    if logger:
       logging.error(msg)
    else:
       cprint(msg, color="red")

And then in CCSDSPy code we would use it like ccsdspy.logging.log_warn("skipped a packet"), etc.

What do you think? Does anyone else want to weigh in? @tloubrieu-jpl @jmbhughes

[ehsteve]

@ddasilva thanks for the feedback. I'll admit that I don't have another model for how to do logging "correctly". As I mentioned, i am following the Astropy model (see their docs here). SunPy has also taken on this approach (see their docs here. And then I went ahead and implemented the same approach in our Space Weather SOC Python packages (see here). I do like this approach because it extends the standard python logger in ways that are generally better and adds features that really should be there.

I would argue that this package should provide a robust approach to logging (and completely move away from using print statements) because the real use-case for this package is in pipelines and i think this approach provides that but I'm open to other approaches. Though there is some benefit to using an approach which is already adopted by the community.

Would appreciate feedback from others that are using this in their pipelines!

[ehsteve]

Happy new year everyone!

[ddasilva]

Happy new year! If this is what astropy and sunpy do it does give it additional merit though it seems a bit much that programs that use both astropy, sunpy, and ccsdspy now have to ship with three additional config files. Let me think this over a bit and get input from others

[jmbhughes]

I agree that logging is better than print statements. If anyone wants to see print-statement-like output, then they can configure a logger to output to stdout. I favor @ehsteve's logging approach because it feels more Pythonic to me even if it is a bit more involved.

[ddasilva]

Thanks @jmbhughes !

@ehsteve I will solicit feedback from @tloubrieu-jpl when I meet with him on Thursday

[tloubrieu-jpl]

Hello,

The most simple way to log in python that I know, and that we apply in our projects is:

Have a logging.conf file such as https://github.com/CCSDSPy/SPaC-Kit/blob/main/src/spac_kit/parser/logger.conf
The default one can be empbedded as a resource with the source code of the package.

Initialize the configuration in the __init__.py file of the package:

conf_file_dir = os.path.dirname(os.path.abspath(__file__))
fileConfig(os.path.join(conf_file_dir, "logger.conf"))

You can also add an optional environment variable so that users can override the default embedded configuration file.

Then in the logged code:

    import logging
    ...
   # at the top of the file
   logger = logging.getLogger(__name__)

   # whenever needed
   logger.debug(...)
   logger.warn(...)
   logger.error(...)

That is what we used on the repository https://github.com/CCSDSPy/SPaC-Kit/tree/421e3593f26eb8f7bef10723a4a3e62cc8c393a4/src/spac_kit/parser

[ehsteve]

One thing that is currently missing in my implementation is supporting the validation messages which @Alrobbertz added a while back. He enables the output as a list. The current code does not enable this but the AstroPy logger class does so we could do the same and subclass the logger just like they do to enable this. They add context managers which is very nice (and again should probably exist in the python logger).

[ehsteve]

@tloubrieu-jpl thanks for reminding me about logger.conf. I totally forgot about that and I wonder why astropy and sunpy went a different route, probably just because they wanted to store configurations beyond just logging. This is a potential long term benefit if we want to enable ccsdspy to be configurable by users in the future.

[ddasilva]

One thing that is currently missing in my implementation is supporting the validation messages which @Alrobbertz added a while back. He enables the output as a list. The current code does not enable this but the AstroPy logger class does so we could do the same and subclass the logger just like they do to enable this. They add context managers which is very nice (and again should probably exist in the python logger).

Is it janky to just call logger.warn() in a for loop over the results? Astropy is a big dependency, which we used to have and eventually worked our way out of

[ddasilva]

Thanks @tloubrieu-jpl

@ehsteve I'm happy to let this pull request move forward with the design as is, though you can switch to logger.conf over yaml at your discretion. Currently, it needs to be cleaned up to allow the CI tests to pass, and some of the doc you copy and pasted from SunPy needs to have the references to SunPy changed to CCSDSPy.

When this is complete I will clone the repo, do some manual testing, and check through a locally built version of the docs.

I believe this warrants a minor version level increment but not a major version level. Do you concur?

[ehsteve]

@ddasilva thanks for feedback and signoff. I'll get to work finishing this up. Minor revision seems appropriate to me but if you want to give it more visibility than major would be okay to.

[codecov]

Codecov Report

❌ Patch coverage is 94.91525% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.14%. Comparing base (4a8f057) to head (36c8e83).
⚠️ Report is 70 commits behind head on main.

Files with missing lines	Patch %	Lines
ccsdspy/logger.py	94.44%	5 Missing ⚠️
ccsdspy/config.py	96.96%	2 Missing ⚠️
ccsdspy/utils.py	83.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #146      +/-   ##
==========================================
- Coverage   96.11%   95.14%   -0.97%     
==========================================
  Files           7        9       +2     
  Lines         721      948     +227     
==========================================
+ Hits          693      902     +209     
- Misses         28       46      +18     

☔ 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.

[ehsteve]

@ddasilva ready for your review.

[ddasilva]

Thanks! Can you give me push permission to your branch? This drops the coverage to 93% (from 96%) and I want to add some tests to keep it above 95%

[ehsteve]

@ddasilva i should not have to give you permission. The repo is already setup to give you permissions to edit any pull requests (see above on the right "Maintainers are allowed to edit this pull request.".

[ehsteve]

@ddasilva i realized just now that i actually made an API change. The validate() function now returns a list of log events and not a list of strings. It is easy to get the string from the log event with log_event.message.

[ehsteve]

This means we probably need a major revision update.

[ehsteve]

Ugh, and it probably means the documentation is out of date. @Alrobbertz ping so you can find this.

[ehsteve]

Added a commit to update the docs and doc string.

[ddasilva]

I added some additional tests for the ccsdspy/config.py and ccsdspy/logger.py modules, which brought the coverage back up to 96%. I also cleaned up the docs. In my docs changes I adjusted wording to be consistent with the other docs. I also removed the API autodoc for the config and decode modules; I don't think we need to show everyone the internals of these.

If we do a major release, that's okay. The last major release was three years ago. It's usually smart to break backwards compatibility as infrequently as possible, so it's worth thinking if there's anything else we want to include in this major release. Any thoughts?

[ehsteve]

@ddasilva i suggest we stop supporting older versions of python with this release. We say we supporting back to 3.6. It is becoming increasingly difficult to get some of these older python versions. For example, the oldest version of Python distributed by conda is 3.8.5. I'm pretty sure if we analyzed our dependencies we would not be able to run this in 3.6. We also only test 3.10 and above. I recommend we update the pyproject toml file to say we only support 3.10 and above. Maybe go to 3.9 but add testing for that version.

[ddasilva]

That's reasonable @ehsteve, I was thinking along similar lines. We can drop those older versions in this release for certain. When I get home I'll start a new issue to organize our plans for the release.

[ddasilva]

Are there any remaining todo's for this PR, or is it waiting on me?

[ehsteve]

Waiting on you I think.