Error in read_generic_cols() with graphviper.utils.logger

[FedeMPouzols]

When loading troublesome colums that produce read errors (such as SPECTRAL_WINDOW/ASSOC_NATURE) a warning/debug message is printed with different level depending on whether the issue happens for a "known troublesome" column or an unexpected column. This got broken after PR #119. Test datasets can produce errors like:

File "/home/fedemp/ws_xradio_pointing/venv_xradio_python_38/lib/python3.8/site-packages/xradio/vis/_vis_utils/_ms/_tables/read.py", line 349, in read_generic_table
    mcoords, mvars = read_generic_cols(infile, tb_tool, timecols, ignore)
  File "/home/fedemp/ws_xradio_pointing/venv_xradio_python_38/lib/python3.8/site-packages/xradio/vis/_vis_utils/_ms/_tables/read.py", line 469, in read_generic_cols
    level = logger.WARNING
AttributeError: module 'graphviper.utils.logger' has no attribute 'WARNING'

A simple fix could be:

diff --git a/src/xradio/vis/_vis_utils/_ms/_tables/read.py b/src/xradio/vis/_vis_utils/_ms/_tables/read.py
index 71a1aac..4f7c321 100644
--- a/src/xradio/vis/_vis_utils/_ms/_tables/read.py
+++ b/src/xradio/vis/_vis_utils/_ms/_tables/read.py
@@ -464,12 +466,11 @@ def read_generic_cols(
                     ]
                 )
             except Exception as exc:
-                level = logger.WARNING
+                msg = f"{infile}: failed to read data for column {col}: {exc}"
                 if col in known_misbehaving_cols:
-                    level = logger.DEBUG
-                logger.log(
-                    level, f"{infile}: failed to read data for column {col}: {exc}"
-                )
+                    logger.debug(msg)
+                else:
+                    logger.warning(msg)
                 data = []

         if len(data) == 0:

@jrhosk: is there any plan to have these logger.log(), logger.WARNING, logger.INFO, etc. in graphviper.utils.logger, or its interface will stay simpler than Python's logging?

[jrhosk]

Maybe I am misunderstanding but warning is there as logger.warning()

... I'll look at the code later but I think everything is there already, it's just how it is accessed is at a different level.

[jrhosk]

I see what you are asking for I think. I hadn't imagined it being used that way, e.g. having conditional messages based on the logging level. I can add those features if it is important to have. I had thought messages would just be classified as warning (), error(), ect without having a condition.

[FedeMPouzols]

@jrhosk: not necessarily asking for it. As in the small diff above, it is easy to get around log() not being available, and probably it is better to handle that type of conditional logging level with explicit calls to debug(), warning(), error() etc. as needed.

Something like log() and the logging levels would be more needed I guess if at some point we wanted to define different levels. And I guess that is not really needed for now, just wondering if there was any plans.

[jrhosk]

Alright, I understand. It can be added, yes. I'll plan on working on it in a few weeks when I get some things off my plate. Thanks for pointing it out.

[jrhosk]

@FedeMPouzols You can assign this to me if you would like. I'll fix it in the next week or so.

[jrhosk]

@FedeMPouzols I have added the logger.log() and logger.exception() options to the graphviper logger. They should be in the main branch as soon as JW reviews the changes. The logger keywords are already there since the graphviper logger is just a wrapper on the logging module, a logger instance doesn't give access to those keywords anyway you have to use logging.WARNINGThe bugged code just needs to have logger.WARNING changed to logging.WARNING. I have added all the keyword in now though so logger.WARNING for instance should work. It should work as is now. Let me know if there is an issue after the logger changes are merged.