8000 Fix regressions from v2 (#212) by taldcroft · Pull Request #218 · sot/kadi · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix regressions from v2 (#212) #218

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Mar 29, 2022
Merged

Fix regressions from v2 (#212) #218

merged 1 commit into from
Mar 29, 2022

Conversation

taldcroft
Copy link
Member

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

  • Passes unit tests on MacOS
  • 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 sot/ska_testr#51)

cd ~/git/ska_testr
run_testr --include kadi

@javierggt
Copy link
Contributor
javierggt commented Mar 29, 2022 •
edited
Loading

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
Copy link
Member Author

@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
Copy link
Member Author

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

@javierggt
Copy link
Contributor

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
Copy link
Contributor
javierggt commented Mar 29, 2022 •
edited
Loading

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

javierggt
javierggt approved these changes Mar 29, 2022
View reviewed changes
Copy link
Contributor
@javierggt javierggt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked that testr passes

@javierggt javierggt merged commit 7d06516 into master Mar 29, 2022
This was referenced Mar 29, 2022
Closed
Merged
@javierggt javierggt mentioned this pull request Apr 21, 2022
Closed
@javierggt javierggt mentioned this pull request May 18, 2022
Closed
@javierggt javierggt mentioned this pull request Aug 3, 2022
Merged
@taldcroft taldcroft deleted the post-v2-regression-fixes branch September 14, 2022 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javierggt javierggt javierggt approved these changes

Assignees

@javierggt javierggt

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@taldcroft @javierggt
0