CABLE-LSM / benchcab

Tool for evaluation of CABLE land surface model
https://benchcab.readthedocs.io/en/latest/
Apache License 2.0
2 stars 4 forks source link

103 we should be using the logging module #224

Closed bschroeter closed 9 months ago

bschroeter commented 11 months ago

Added changes from yesterday's discussion. Also updated the CLI interface to engage the log level from the verbosity flag.

codecov[bot] commented 11 months ago

Codecov Report

Attention: 72 lines in your changes are missing coverage. Please review.

Comparison is base (7c8fa5a) 59.61% compared to head (b65ddbb) 60.76%. Report is 2 commits behind head on main.

Files Patch % Lines
benchcab/benchcab.py 22.85% 54 Missing :warning:
benchcab/fluxsite.py 74.07% 7 Missing :warning:
benchcab/utils/repo.py 40.00% 6 Missing :warning:
benchcab/main.py 25.00% 3 Missing :warning:
benchcab/comparison.py 80.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #224 +/- ## ========================================== + Coverage 59.61% 60.76% +1.15% ========================================== Files 30 30 Lines 2199 2238 +39 ========================================== + Hits 1311 1360 +49 + Misses 888 878 -10 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

SeanBryan51 commented 11 months ago

Also, I realise the integration test will fail due to #223. I will fix this today so that we have working integration tests.

bschroeter commented 9 months ago

@SeanBryan51, OK. Massive update with merges and reversion to the native logger class. We had to do this because there is some python magic behind the scenes that will enable the native logger to pickle, whereas subclassing the native logger loses this ability. Many hours chasing that down.

This means that we lose the ability for multi-line logging in a single call (which also means we retain the code location information in the log), and we have to call an explicit log call for each line, which is technically the proper practice and will enable us to later use a log-scanning framework to extract events from log lines.

I've updated from main, merged, applied a bunch of changes suggested from ruff and black and ran the integration and pytest suites through to success. I think we're good to go on this.

We should look at adding black and ruff to the GitHub actions in another PR, if we haven't already.

Let me know if you have any questions.

ccarouge commented 9 months ago

@bschroeter There is a minor conflict now with main since the merge of #232. Since Sean is away, I'll try and review.

bschroeter commented 9 months ago

I've spotted a few errors while resolving merge conflicts. But otherwise only minor typos.

If you don't mind resubmitting for review once this is solved, I want to make sure I'm not missing anything else.

Thanks Claire - looks like the merge from main didn't pick up some of the recent changes. I'm working through these and getting an update PR ready for you.

bschroeter commented 9 months ago

OK @ccarouge, I've gone back through everything and manually applied all of the changes that got clobbered over the last week. I think it should all be ok now...

I've run all tests and integration and it seems to pass.

Coverage drops, but otherwise I think this is ready?