Fix regressions from v2 (#212)

taldcroft commented 2 years ago

Description

This fixes some regression (ska_testr) problems introduced by #212.

Use absolute rather than relative imports. When I moved scripts into the scripts subpackage those relative imports were broken. Another reason to avoid relative imports in most (but not all) situations.
Reverted the name of the command-line script from kadi_update_cmds_v1 back to the original kadi_update_cmds. This should fix the error in ska_testr where it failed to find the expected script.
Logging was doing unexpected things because of the addition of a new top-level logger with the name kadi. Modules that initialized a logger with __name__ were generating output from two loggers. For some reason that is actually still a mystery to me, when I did a local install test of kadi the top-level logger was getting a log-level of DEBUG and generating unexpected logging output. I'd still like to track that down, but using new logger names that don't start with kadi. fixes this.

Testing

[x] Passes unit tests on MacOS
[x] Functional testing

Functional testing

Installed into a test environment, copied the current (but a few days out of date) kadi data (cmds v1, v2 and events) into a local test directory (not the git repo), and successfully ran:

kadi_update_cmds
kadi_update_cmds_v2
kadi_update_events

Then ran ska_testr in the test env and got a PASS (with https://github.com/sot/ska_testr/pull/51)

cd ~/git/ska_testr
run_testr --include kadi

javierggt commented 2 years ago

The reason why you get the double output is that loggers with name starting in kadi. propagate messages up the hierarchy. From the docs:

Child loggers propagate messages up to the handlers associated with their ancestor loggers. Because of this, it is unnecessary to define and configure handlers for all the loggers an application uses. It is sufficient to configure handlers for a top-level logger and create child loggers as needed. (You can, however, turn off propagation by setting the propagate attribute of a logger to False.)

And I don't know why the DEBUG level is set to the top-level kadi logger.

taldcroft commented 2 years ago

@javierggt - yes, I understood the propagation. It was only the fact that the level was at DEBUG that makes no sense. I looked pretty hard through the code trying to find where that might happen and failed. Another piece of this mystery is that it only seemed to happen when I ran this in an installed environment via kadi_update_events. If I ran from the repo via python -m kadi.scripts.update_events then the logger level was at the default WARNING.

I'm pretty sure there is a reason but I haven't found it.

taldcroft commented 2 years ago

My plan is to come back later with less time pressure and rework all the logging to do it consistently and correctly.

javierggt commented 2 years ago

Is this PR supposed to fix these errors?

'warning' matched at test_regress.log:2217 :: /proj/sot/ska/jgonzalez/git/kadi/kadi/cmds/cmds.py:17: FutureWarning: kadi.cmds is deprecated and no longer tested, use kadi.commands instead
'warning' matched at test_regress.log:2218 :: warnings.warn('kadi.cmds is deprecated and no longer tested, use kadi.commands instead',
'error' matched at test_regress.log:2241 :: AttributeError: 'LazyVal' object has no attribute '_val'
'error' matched at test_regress.log:2266 :: raise IOError("``%s`` does not exist" % (filename,))
'error' matched at test_regress.log:2267 :: OSError: ``/export/jgonzale/ska_testr/outputs/logs/Linux_2022-03-29T18-39-49_8baabe2_kady.cfa.harvard.edu/kadi/cmds.h5`` does not exist

javierggt commented 2 years ago

I'm running again. The scripts were not installed.

sot / kadi