Logging

[TeaganKing]

This PR should address #82 by adding logging capabilities to CUPiD instead of using print statements. This should accomplish the following three tasks:

• update print statements to logger statements
• add log file to .gitignore
• move ploomber output to log file instead of printed to console (moved to new issue Move ploomber output to log file instead of printing to console #129)

It is useful to implement logging instead of using print statements because log files can be retrieved in the future (whereas print statements are not saved) and this reduces the amount of printing to the console that can get overwhelming for users.

All Submissions:

• Have you followed the guidelines in our Contributor's Guide (including the pre-commit check)?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

New Feature Submissions:

1. Have you lint your code locally prior to submission?

Changes to Core Features:

• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you successfully tested your changes locally?

[TeaganKing]

This page provides some examples of basic pipelines that can be used to set up logging for ploomber; however, this calls dag in a slightly different manner and will require a bit more looking into.

[TeaganKing]

Given that within the CESM workflow, log files such as cupid.o#### and cupid.e#### where #### is a timestamp will be automatically generated, we can remove the filename from the logger instantiation.

• It would be useful to set up logging levels in the configuration file so that users can choose what amount of output they want printed to the console.

[TeaganKing]

I'd like to test this a bit more thoroughly to make sure it didn't break cupid-run or timeseries generation. Thus, the testing might be on hold until HPC systems are back up, but this should be ready for testing/review next week.

[TeaganKing]

The timeseries generation and cupid-run seem to work as expected, so this is ready for review.

[mnlevy1981]

This looks great! There are a couple of in-line comments, plus I wanted to note that I grepped for print (only in the cupid/ subdirectory) and found two more occurrences that we may want to address:

$ cd cupid
$ git grep -n print
timeseries.py:87:        print(f"\n  No time series files requested for {component}...")
util.py:55:        print(f"ERROR: {config_path} not found")

[mnlevy1981]

If you change line 104 to

log_level = control["computation_config"].get("log_level", "info")

Then you don't need the if statement at 105 or the entire else block from 122 - 126 (maybe move the # default level is info if log level is not set in config comment to above setting log_level?)

[mnlevy1981]

Thinking about this more, should we use

        if log_level == "debug":
            logging.basicConfig(
                level=logging.DEBUG,
            )
        elif log_level == "info":
            logging.basicConfig(
                level=logging.INFO,
            )
        elif log_level == "warning":
            logging.basicConfig(
                level=logging.WARNING,
            )
        elif log_level == "error":
            logging.basicConfig(
                level=logging.ERROR,
            )
        else:

and trap the case when log_level is not one of the four expected strings?

[mnlevy1981]

This function is called from create_time_series(), which uses logger prior to calling in... so I think we want logger to be a required argument. This will let us remove

if logger is None:
    logger = cupid.util.setup_logging("config.yml")

at lines 370-372 (and I think also the import cupid.util at lines 17 & 18)

[TeaganKing]

Thanks for this review, @mnlevy1981 !

I addressed those comments, except for the one print statement in cupid/util.py-- the get_control_dict function is needed prior to logging implementation (because logging uses info from the config file). Thus, I think this one would have circular implementation if both functions call one another.

[mnlevy1981]

Did you see my follow-up comment about this (#127 (comment))? What happens if someone puts log_level: all_messages (or even just log_level: DEBUG) in config.yml? none of these if statements will evaluate to true, so we won't call logging.basicConfig() at all... I think we want to catch that and raise an exception.

[TeaganKing]

Oh! I think I marked it as resolved and then the response was hidden. Sorry about that.

I added an 'else' statement back in and also included the all-caps versions of the log levels.

[mnlevy1981]

This looks great, thanks!