Skip to content
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.

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

Add email notifications for validation violations #302

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

taldcroft
Copy link
Member

@taldcroft taldcroft commented Nov 23, 2023

Description

This PR adds email notification to aca@cfa.harvard.edu for violations.

As part of this I did some refactoring to use smaller, more separable, processing functions. The first step was moving the mail validation script processing function from validate.py (implements validator functionality) to validate_states.py (does the processing to produce a report page).

Closes #271

Required dependencies

This requires:

  • Quaternion 4.2.0
  • ska_helpers 0.13.0

Interface impacts

Email alert notification for any kadi command states validation violations.

Testing

Unit tests

  • Mac
(ska3-perf) ➜  kadi git:(validate-finish-line) git rev-parse HEAD                        
6adf5f2fa31b847cd469c0c9d0e8fe68a81e0312
(ska3-perf) ➜  kadi git:(validate-finish-line) pytest                                    
=========================================== test session starts ===========================================
platform darwin -- Python 3.10.8, pytest-7.2.1, pluggy-1.0.0
rootdir: /Users/aldcroft/git, configfile: pytest.ini
plugins: timeout-2.1.0, anyio-3.6.2
collected 219 items                                                                                       

kadi/commands/tests/test_commands.py .............................................................. [ 28%]
.............                                                                                       [ 34%]
kadi/commands/tests/test_states.py ......................x......................................... [ 63%]
....x.......................                                                                        [ 76%]
kadi/commands/tests/test_validate.py ....................                                           [ 85%]
kadi/tests/test_events.py ..........                                                                [ 89%]
kadi/tests/test_occweb.py ......................                                                    [100%]

================================ 217 passed, 2 xfailed in 91.58s (0:01:31) ================================

Independent check of unit tests by [REVIEWER NAME]

  • [PLATFORM]:

Functional tests

On kady in ska3 2023.7, with the two git directories checked out at master corresponding to the versions shown above. The off_nom_roll violations are found because this processing does not include the improvements to ska_sun and chandra_aca.planets.

Output: https://icxc.cfa.harvard.edu/aspect/test_review_outputs/kadi/pr302/

env PYTHONPATH=/home/aldcroft/git/Quaternion:/home/aldcroft/git/ska_helpers \
  python -m kadi.scripts.validate_states --email taldcroft@cfa.harvard.edu
...
2023-12-03 07:06:14,274 get_plot_html: Creating HTML for state sun_pos_mon
2023-12-03 07:06:14,290 main: kadi validate_states processing found state violation(s):

    name             start                  stop        
------------ --------------------- ---------------------
off_nom_roll 2023:326:07:37:24.629 2023:326:08:31:44.128
off_nom_roll 2023:328:23:57:25.428 2023:329:00:06:25.603
off_nom_roll 2023:329:00:38:51.053 2023:329:00:50:16.778
off_nom_roll 2023:329:01:09:07.353 2023:329:04:49:08.328
off_nom_roll 2023:335:13:30:00.478 2023:335:13:31:30.678
   pcad_mode 2023:326:20:05:54.867 2023:326:20:07:23.017

See:https://cxc.harvard.edu/mta/ASPECT/validate_states/

2023-12-03 07:06:14,327 send_mail: Sent mail to ['taldcroft@cfa.harvard.edu']

Email:
image

@taldcroft taldcroft changed the title WIP: Validate finish line Improve kadi state accuracy and get validation across the finish line Nov 26, 2023
@taldcroft taldcroft marked this pull request as draft November 26, 2023 20:37
Base automatically changed from flatten-namespace-package-names to master November 29, 2023 20:26
@taldcroft taldcroft force-pushed the validate-finish-line branch from 7f7489e to 1494b5b Compare December 1, 2023 16:27
@taldcroft taldcroft changed the base branch from master to improve-off-nom-roll-state-accuracy December 1, 2023 16:27
@taldcroft taldcroft changed the title Improve kadi state accuracy and get validation across the finish line Add email notifications Dec 2, 2023
@taldcroft taldcroft changed the title Add email notifications Add email notifications for validation violations Dec 3, 2023
@taldcroft taldcroft marked this pull request as ready for review December 3, 2023 11:56
@taldcroft taldcroft force-pushed the validate-finish-line branch from aa29263 to 6adf5f2 Compare December 3, 2023 12:10
@taldcroft taldcroft changed the base branch from improve-off-nom-roll-state-accuracy to fix-spm-off-nom-roll-violations December 3, 2023 12:10
@taldcroft taldcroft requested a review from jeanconn December 3, 2023 12:10

logger = logging.getLogger(__name__)
logger = logging.getLogger("kadi")
Copy link
Member Author

Choose a reason for hiding this comment

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

This makes the logger use the same output format as the rest.

@@ -68,15 +205,22 @@ def main(args=None):
maude.conf.cache_msid_queries = True
fetch.CACHE = True

if opt.in_work:
conf.include_in_work_command_events = True
kadi.commands.conf.include_in_work_command_events = opt.in_work
Copy link
Member Author

Choose a reason for hiding this comment

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

In the unlikely event that a user has set the configuration in a file, the code now overrides that. I.e. the command line option wins.

opt : argparse.Namespace
Command-line options
"""
from acdc.common import send_mail
Copy link
Contributor

Choose a reason for hiding this comment

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

acdc is presently a non-fsds package. One of the reasons we let it go there was the idea that this acdc.common send mail import was also only used by other non-fsds packages. So I see this new import in kadi validation as suggesting one of several paths:

  1. Follow through and copy/move this send_mail into something like ska_helpers
  2. Put acdc back into ska3-flight
  3. Put this kadi validation and trending into a separate non-fsds package instead of kadi proper
  4. Leave it like this and leave acdc in non-fsds packages but realize the validation reporting then depends on a package outside of flight (practically, the validation is likely going to be run from HEAD ska3/flight which will have acdc installed).

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Let's go with 4 right now. Tests should still pass without acdc installed. I have been bothered by having this utility living in acdc so this will get fixed in a reasonable time scale.

action="append",
dest="emails",
default=[],
help='Email address for notification (multiple allowed, use "TEST" for testing)',
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I don't know if it is useful to document in here that this TEST thing is a feature of acdc.common.send_mail where it just logs the output.

Copy link
Member Author

Choose a reason for hiding this comment

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

It will be documented when that function gets a new home in ska_helpers.

@jeanconn
Copy link
Contributor

jeanconn commented Dec 5, 2023

Though I should have asked before I approved -- why does this need ska_helpers 0.13.0?

@taldcroft
Copy link
Member Author

Though I should have asked before I approved -- why does this need ska_helpers 0.13.0?

I think it was the coerce_types thingy.

@jeanconn
Copy link
Contributor

jeanconn commented Dec 5, 2023

Makes sense but is in 0.12.0 (0.13.0 just has a test update so I was trying to figure out if if you were calling out a different package or different release).

Base automatically changed from fix-spm-off-nom-roll-violations to master December 7, 2023 15:33
@taldcroft taldcroft merged commit 4664263 into master Dec 7, 2023
2 checks passed
@taldcroft taldcroft deleted the validate-finish-line branch December 7, 2023 15:33
This was referenced Jan 9, 2024
@javierggt javierggt mentioned this pull request Feb 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add logging of validation violations
2 participants