103 we should be using the logging module #224

bschroeter · 2023-12-20T01:03:28Z

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

…o be refactored to take advantage of this. #103

codecov · 2023-12-20T01:05:13Z

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 ⚠️
benchcab/fluxsite.py	74.07%	7 Missing ⚠️
benchcab/utils/repo.py	40.00%	6 Missing ⚠️
benchcab/main.py	25.00%	3 Missing ⚠️
benchcab/comparison.py	80.00%	2 Missing ⚠️

Additional details and impacted files

@@            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

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

ccarouge

Just a small change on how the log_level is set. I would do it directly within the argument parser instead of during parse_and_dispatch.

ccarouge · 2023-12-20T03:05:31Z

benchcab/main.py

+    # Intercept the verbosity flag to engage the logger
+    log_level = 'debug' if args.get('verbose', False) == True else 'info'


This could be handled in the parser definition directly:

args_subcommand.add_argument( "-v", "--verbose", help="Enable more detailed output in the command line.", action="store_const", default="info", const="debug", dest="log_level", )

or something like that because I haven't tried it out... I believe action isn't necessary but it doesn't hurt either.

See, this was my first thought, but then I saw that the verbose argument is passed around 150+ times throughout the code so there is a bit of a refactoring exercise involved to ensure things still work. This suggestion is probably related to, but a bit outside the scope of this ticket.

Should we raise this as a different Issue and PR to address this?

Yes, we can put it in a different issue.

ccarouge · 2023-12-20T03:38:04Z

benchcab/main.py

+    # We just need to instantiate this with the desired level
+    logger = get_logger(level=log_level)


Considering the suggestion above this would then need to change to:

Suggested change

# We just need to instantiate this with the desired level

logger = get_logger(level=log_level)

# We just need to instantiate this with the desired level

logger = get_logger(level=args.log_level)

See comment above :)

SeanBryan51

Looks good. Just a few suggestions:

Can you run black on your changes?
Can you run ruff on your changes and fix any small errors like unused imports etc?
Now that we have a logger, can you remove all occurrences of the verbose argument?

benchcab/utils/__init__.py

SeanBryan51 · 2023-12-20T23:41:13Z

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

…ow. #103

ccarouge · 2024-01-18T04:37:44Z

benchcab/benchcab.py

-# Copyright 2022 ACCESS-NRI and contributors. See the top-level COPYRIGHT file for details.
-# SPDX-License-Identifier: Apache-2.0
-


We shouldn't remove the copyright notice from this file. Can we reinstate please?

Sure thing, I think it was when I was trying to get ruff to work, for some reason it didn't like the double "#" comments. Typically a triple quote is used at the top of Python files but I will see how it goes.

ccarouge

Actually, I think this is ready. We will have to open an issue about the way log_level is set.
I've added a comment yesterday about a deleted copyright statement in a file that needs to be added. Please make sure to add that statement back first and then this can be merged.

…103

bschroeter · 2024-02-06T01:13:38Z

@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 · 2024-02-08T04:21:55Z

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

ccarouge

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.

ccarouge · 2024-02-08T04:45:28Z

benchcab/benchcab.py

-                pbs_config=config["fluxsite"]["pbs"],
-                verbose=verbose,


We want to keep the pbs_config argument here! That comes from #238

ccarouge · 2024-02-08T04:46:37Z

benchcab/benchcab.py

+        # TODO(Sean) we should archive revision numbers for CABLE-AUX
+        cable_aux_repo = SVNRepo(
+            svn_root=internal.CABLE_SVN_ROOT,
+            branch_path=internal.CABLE_AUX_RELATIVE_SVN_PATH,
+            path=internal.SRC_DIR / "CABLE-AUX",
+        )
+        cable_aux_repo.checkout(verbose=verbose)


We removed this in #230

ccarouge · 2024-02-08T04:48:41Z

benchcab/benchcab.py

        print("Running fluxsite tasks...")
-        if config["fluxsite"]["multiprocess"]:
-            ncpus = config["fluxsite"]["pbs"]["ncpus"]
+        try:
+            multiprocess = config["fluxsite"]["multiprocess"]
+        except KeyError:
+            multiprocess = internal.FLUXSITE_DEFAULT_MULTIPROCESS
+        if multiprocess:
+            ncpus = config.get("pbs", {}).get(
+                "ncpus", internal.FLUXSITE_DEFAULT_PBS["ncpus"]
+            )
            run_tasks_in_parallel(tasks, n_processes=ncpus, verbose=verbose)
        else:
-            run_tasks(tasks, verbose=verbose)
-        print("Successfully ran fluxsite tasks")
-        print("")
+            run_tasks(tasks)
+        self.logger.info("Successfully ran fluxsite tasks")


We want the version in main here but I believe the print at the top should call the logger.

ccarouge · 2024-02-08T04:50:01Z

benchcab/benchcab.py

        print("Running comparison tasks...")
-        if config["fluxsite"]["multiprocess"]:
-            ncpus = config["fluxsite"]["pbs"]["ncpus"]
+        try:
+            multiprocess = config["fluxsite"]["multiprocess"]
+        except KeyError:
+            multiprocess = internal.FLUXSITE_DEFAULT_MULTIPROCESS
+        if multiprocess:
+            try:
+                ncpus = config["fluxsite"]["pbs"]["ncpus"]
+            except KeyError:
+                ncpus = internal.FLUXSITE_DEFAULT_PBS["ncpus"]


We want the version in main. And we want to call self.logger.info instead of print on the first line.

ccarouge · 2024-02-08T04:58:02Z

benchcab/fluxsite.py

+        model : Model
+            Model.
+        met_forcing_file : str
+            Met forcinf file.


Suggested change

Met forcinf file.

Met forcing file.

ccarouge · 2024-02-08T05:01:27Z

benchcab/model.py

+        Parameters
+        ----------
+        repo : Repo
+            Respository.


Suggested change

Respository.

Repository.

ccarouge · 2024-02-08T05:18:03Z

benchcab/utils/singleton.py

Not used, remove?

ccarouge · 2024-02-08T05:18:36Z

benchcab/utils/__init__.py

 from importlib import resources
 from pathlib import Path

+import yaml
+
+# from benchcab.utils.singleton_logger import SingletonLogger


Remove instead of commented?

bschroeter · 2024-02-12T23:24:03Z

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.

…changes. #103

bschroeter · 2024-02-13T03:10:34Z

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?

ccarouge

@bschroeter There is one more clobbered conflict resolution. I think I have put the right code in the suggestion. Can you try it out before I give approval?

ccarouge · 2024-02-14T02:46:50Z

benchcab/benchcab.py

+        self.logger.debug("Running comparison tasks...")
+        try:
+            multiprocess = config["fluxsite"]["multiprocess"]
+        except KeyError:
+            multiprocess = internal.FLUXSITE_DEFAULT_MULTIPROCESS
+        if multiprocess:
+            try:
+                ncpus = config["fluxsite"]["pbs"]["ncpus"]
+            except KeyError:
+                ncpus = internal.FLUXSITE_DEFAULT_PBS["ncpus"]


Old version. We want:

Suggested change

self.logger.debug("Running comparison tasks...")

try:

multiprocess = config["fluxsite"]["multiprocess"]

except KeyError:

multiprocess = internal.FLUXSITE_DEFAULT_MULTIPROCESS

if multiprocess:

try:

ncpus = config["fluxsite"]["pbs"]["ncpus"]

except KeyError:

ncpus = internal.FLUXSITE_DEFAULT_PBS["ncpus"]

self.logger.debug("Running comparison tasks...")

if config["fluxsite"]["multiprocess"]:

ncpus = config["fluxsite"]["pbs"]["ncpus"]

Hi @ccarouge, I've committed the fix after running all tests again. Passing.

One thing to note, many of these remaining changes did not trigger failures in the tests. We may need to take a look at how robust our testing is to catch these in future.

…m:CABLE-LSM/benchcab into 103-we-should-be-using-the-logging-module

Fixes #103

ccarouge

Finally! Last small changes done:

clobbered merge conflicts
removed unused singleton.py

bschroeter added 3 commits December 18, 2023 11:31

Added baseline logging implementation, tests relying on stdout need t…

2f6da74

…o be refactored to take advantage of this. #103

Reorganised logger, updated most debugs to infos. Fixes #103

59e56fe

Added logger instantiation to CLI. Fixes #103

43deac8

bschroeter requested review from ccarouge and SeanBryan51 December 20, 2023 01:03

bschroeter linked an issue Dec 20, 2023 that may be closed by this pull request

We should be using the logging module #103

Closed

ccarouge requested changes Dec 20, 2023

View reviewed changes

SeanBryan51 requested changes Dec 20, 2023

View reviewed changes

benchcab/utils/__init__.py Show resolved Hide resolved

bschroeter added 3 commits December 21, 2023 11:48

Automatic application of ruff/black suggestions. Manual edits to foll…

63b41f8

…ow. #103

Manual ruff modifications. #103

023c4d4

Manual black modifications. #103

ff36e53

ccarouge reviewed Jan 18, 2024

View reviewed changes

ccarouge approved these changes Jan 18, 2024

View reviewed changes

bschroeter added 6 commits January 19, 2024 13:11

Merge branch 'main' into 103-we-should-be-using-the-logging-module

d26937b

Refactoring of verbosity

7686403

Black reformatting. #103

222ad43

Reverted to native logger singleton, less flexible but pickles. #103

1ba25b0

Applied black/ruff checks, ran integration tests successfully. Fixes #…

7d84532

…103

Merge update from main. Fixes #103

f618128

bschroeter requested a review from SeanBryan51 February 6, 2024 01:08

ccarouge requested changes Feb 8, 2024

View reviewed changes

bschroeter added 3 commits February 13, 2024 10:48

Updated again from main - further commits coming to handle clobbered …

44818a4

…changes. #103

Ruff checks, pytest passes. #103.

222d16d

Black reapplied. #103

9b49c13

bschroeter requested a review from ccarouge February 13, 2024 03:09

ccarouge reviewed Feb 14, 2024

View reviewed changes

abhaasgoyal and others added 3 commits February 15, 2024 09:49

Merge branch 'main' into 103-we-should-be-using-the-logging-module

7a3b741

Remaining clobbered code fix. #103.

b6bca5b

Merge branch '103-we-should-be-using-the-logging-module' of github.co…

3957aaf

…m:CABLE-LSM/benchcab into 103-we-should-be-using-the-logging-module

bschroeter requested a review from ccarouge February 15, 2024 00:06

Last clobbered merge changes.

b65ddbb

Fixes #103

ccarouge approved these changes Feb 15, 2024

View reviewed changes

bschroeter removed the request for review from SeanBryan51 February 15, 2024 05:02

ccarouge merged commit 3b40cd4 into main Feb 15, 2024
5 checks passed

ccarouge deleted the 103-we-should-be-using-the-logging-module branch February 15, 2024 05:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

103 we should be using the logging module #224

103 we should be using the logging module #224

bschroeter commented Dec 20, 2023

codecov bot commented Dec 20, 2023 •

edited

Loading

ccarouge left a comment

ccarouge Dec 20, 2023

bschroeter Dec 21, 2023

ccarouge Jan 18, 2024

ccarouge Dec 20, 2023

bschroeter Dec 21, 2023

SeanBryan51 left a comment

SeanBryan51 commented Dec 20, 2023

ccarouge Jan 18, 2024

bschroeter Jan 19, 2024

ccarouge left a comment

bschroeter commented Feb 6, 2024

ccarouge commented Feb 8, 2024

ccarouge left a comment

ccarouge Feb 8, 2024

ccarouge Feb 8, 2024

ccarouge Feb 8, 2024

ccarouge Feb 8, 2024

ccarouge Feb 8, 2024

ccarouge Feb 8, 2024

ccarouge Feb 8, 2024

ccarouge Feb 8, 2024

bschroeter commented Feb 12, 2024

bschroeter commented Feb 13, 2024

ccarouge left a comment

ccarouge Feb 14, 2024

bschroeter Feb 15, 2024

ccarouge left a comment

		# Intercept the verbosity flag to engage the logger
		log_level = 'debug' if args.get('verbose', False) == True else 'info'

		# We just need to instantiate this with the desired level
		logger = get_logger(level=log_level)

		# Copyright 2022 ACCESS-NRI and contributors. See the top-level COPYRIGHT file for details.
		# SPDX-License-Identifier: Apache-2.0

103 we should be using the logging module #224

103 we should be using the logging module #224

Conversation

bschroeter commented Dec 20, 2023

codecov bot commented Dec 20, 2023 • edited Loading

Codecov Report

ccarouge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeanBryan51 left a comment

Choose a reason for hiding this comment

SeanBryan51 commented Dec 20, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccarouge left a comment

Choose a reason for hiding this comment

bschroeter commented Feb 6, 2024

ccarouge commented Feb 8, 2024

ccarouge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bschroeter commented Feb 12, 2024

bschroeter commented Feb 13, 2024

ccarouge left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccarouge left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 20, 2023 •

edited

Loading