DM-33898: OCPS calibrations script: add Watcher alarms/warnings when verification fails

[plazas]

No description provided.

[tribeiro]

Here is an initial round of comments. Can you turn this into a draft PR while we work to rough details?

One thing I noticed is that we only have 1 level of alarm. Consider having different levels for WARNING, SERIOUS, etc..

[tribeiro]

please remove all references to "bias"

[tribeiro]

In the docstring above you said it was supposed to be "WARNING" alarm but here you set it so SERIOUS.

[plazas]

I adjusted the docstring to say SERIOUS.

[tribeiro]

I think we should use WARNING instead

[tribeiro]

A couple more comments..

[tribeiro]

repo is not defined outside this if statement, so if this never runs you will end up with undefined repo...

You probably want to check for this condition.

[plazas]

We know that the OCPS response contains this entry, but if not, I will raise an RuntimeError (we can't do anything if the response, for whatever reason, does not have a repo).

[tribeiro]

what if response_verify["parameters"]["environment"] is empty? you will never get a pass of the loop...

[plazas]

I added a check for its length, and issue an error if is empty.

[tribeiro]

remove "bias"

[tribeiro]

this was already set in line 118, you can probably remove the else statement

[tribeiro]

Let's use WARNING for now

[tribeiro]

missing an "f" in the string...

f"Job {job_id_verify} is not a recognizable cp_verify run."

[tribeiro]

please, remove commented code lines

[tribeiro]

There's a syntax error in you unit test file:

E     File "/home/jenkins/workspace/escope-and-Site_ts_watcher_PR-57/tests/rules/test_cp_verify_alarm.py", line 132
E       2022110300072: {'FAILURES': ['RXX_S00 C00 NOISE', 'RXX_S00 C00 PROCESSING', 'RXX_S00 C01
E                                                                                   ^
E   SyntaxError: unterminated string literal (detected at line 132)

[plazas]

There's a syntax error in you unit test file:

E     File "/home/jenkins/workspace/escope-and-Site_ts_watcher_PR-57/tests/rules/test_cp_verify_alarm.py", line 132
E       2022110300072: {'FAILURES': ['RXX_S00 C00 NOISE', 'RXX_S00 C00 PROCESSING', 'RXX_S00 C01
E                                                                                   ^
E   SyntaxError: unterminated string literal (detected at line 132)

Thanks. I now read the example strings from a file.

[tribeiro]

Please! Let's change this to WARNING

[plazas]

I'm so sorry, I changed the code but missed the docstring. I'll change it now.

[r-owen]

This is my first pass. The main thing that worries me is not running the butler query in a background thread, since the butler is not asyncio-friendly. I think your config schema may also need some tweaking.

Two more general questions:

• Do you want the operators to have to acknowledge these warnings? If not, I suggest waiting for an update Tiago requested to allow Watcher alarms to report a condition much like a log message, with no expectation that it will be acknowledged. These will be reported in a new LOVE window specifically intended for that purpose. I haven't started work on adding the new Watcher event to ts_xml yet, but hope to get it into the next ts_xml release.
• Is Warning the only severity you want? What if there are extra failures beyond the threshold you have now? (That said, it might be simpler to get this running in its simplest form first, then add extra severities later).

[r-owen]

If these are the only allowed values then please update the schema to only allow those values. Best to catch errors in the validation step.

I gently suggest making them lowercase to improve readability. You can cast them to uppercase internally when using them. But if all usage on the mountain is uppercase then it's fine to leave them as is.

[plazas]

Thanks! How can I specify that these values are the only ones allowed?
We use BIAS, DARK, or FLAT in other scripts such as https://github.com/lsst-ts/ts_externalscripts/blob/develop/python/lsst/ts/externalscripts/base_make_calibrations.py, which is the one this alarm is related to. I will leave then in uppercase if that's OK.

[r-owen]

See "enum" in jsonschema docs

[r-owen]

If the number of allowed values is fixed and fairly small, please specify them as an enum to catch errors in the validation step.

[r-owen]

Also

• You seem to be missing the type (integer)
• The description line above should be prefixed by description: or description: >-

[plazas]

I added the type and the prefix. Is there an example of the use of enum for cases like this?

[r-owen]

The jsonschema docs are very good

[plazas]

Found it, thanks!

[r-owen]

This call may run slowly and block the Watcher from doing anything else. Please run it in a thread, e.g. using run_in_executor. This salobj documentation for writing scripts shows how: https://ts-salobj.lsst.io/sal_scripts.html#run-method

You can also create a concurrent.futures.ThreadPoolExecutor and pass that as the first argument to run_in_executor, instead of None. I suggest that, because you'll be calling this a lot and it is more efficient to create one and keep using it.

Which brings up a question: how often will this run? Will it have enough time to process one message before the next one comes in? In other words: how many butlers will be running queries at the same time? Just curious. if you create your own thread pool executor you can limit this (e.g. if you specify n=1 then only one will run at a time, which could make processing your rule back up, but will not block the Watcher itself, n=3 would allow up to 3 to run at the same time...).

[plazas]

I have added run_in_executor and concurrent.futures.ThreadPoolExecutor when getting the verification statistics, thank you for the suggestions.

The idea is to run one of the instances of "base_make_calibrations.py" via LOVE every afternoon to take individual calibrations, and use these individual images to run cp_verify using existing standard calibrations (this is the current desired default as specified in DMTN-222, and it's called "external verification") for a given instrument. If the tests in cp_verify do not pass, the Watcher should tell the observer. I'm not quite sure how to answer your questions, though.

[plazas]

In addition, I'm now also using run_in_executor for the butler creation and calls.

[r-owen]

If creating butlers is still expensive, please cache the ones you create. Specifically I suggest:

• For the cache use a dict of key: butler, where the key is (repo, verify_collection) or (repo, job_id_verify), as you prefer. Together those seem to uniquely identify a butler.
• Create the empty cache in the __init__ method.
• Empty the cache (and close all butlers) in the stop method. You probably know this, but this is a nice way to clear a dict:

while mydict:
    key, value = mydict.popitem()
    # ... do something with value, such as close it

[plazas]

I'm not sure if this call will be expensive. A priori, it's just one butler call per calibration type (currently BIAS, DARK, and FLAT) to get the verification statistics dictionary.

[r-owen]

Please pretty-print both of your yaml data files.

[plazas]

I'd be happy to do so, but I can't find anything specific online to do this (i.e., to make a yaml file "pretty"). Did you have a package in mind or a particular standard/style that I should be following?

[plazas]

Chris Waters pointed me to pprint. I'll look into that.

[plazas]

Done :)

[r-owen]

Great! For the record, whenever I need this I search for "Yaml formatter" and pick one of the early hits.

This sort of thing is also useful for working with json schemas. Put the yaml through a yaml-to-json converter, then past that into a json schema validator (each of which is easily found with a web search). This shows any errors in the schema.

[r-owen]

I'm confused by this: name isn't a config parameter, and all three config parameters are missing. Shouldn't the arguments be:

• calibration_type
• ocps_index
• verification_threshold

[plazas]

Ah, thanks! This probably was copied from some other alarm test many months ago. I'll change the arguments and the dictionary:

    config_dict = dict(calibration_type =  calibration_type,
                       ocps_index = ocps_index,
                       verification_threshold=verification_threshold
                  )

[r-owen]

Please include verification_threshold. It will always be present in any case, since it has a default.

[r-owen]

This still needs to be done. It is getting long, so you may want to start listing these on separate lines, e.g.:

required:
  - calibration_type
  - ocps_index
  - verification_threshold

[plazas]

Added :) Sorry, I was (am) still working on implementing your comments!

[r-owen]

Seems like good progress, but more work needed. I think I found at least one bug (the way you run your job as a thread). Also please look through my earlier suggestions.

[r-owen]

This still needs to be done. It is getting long, so you may want to start listing these on separate lines, e.g.:

required:
  - calibration_type
  - ocps_index
  - verification_threshold

[r-owen]

Please say a bit more about this. I have no clue what it might look like. Likewise for verify_stats.

(Please also consider deleting the blank line between these arguments).

[plazas]

I will add a note that points to the yaml files in ts_watcher/tests/rules/data/cp_verify_alarm, which are examples of these responses. Would that suffice in this case?

[r-owen]

It might have to, given how long and confusing those are.

[r-owen]

This callback should take two arguments: (data, topic_callback), using exactly those names.

Also I see 3 different names for the OCPS result: result (the SAL field name), response (your variable) and response_verify (the argument to check_response). The code would be much clearer if you could be more consistent. But result or response are vague, so I suggest ocps_result. That would result in:

def __call__(self, data, topic_callback):
         # OCPS response after calling the pipetask
         ocps_result = json.loads(data.result)
         ....

and renaming the argument in check_response accordingly.

[plazas]

Thanks! I was using response... to try to match the way this is generated in https://github.com/lsst-ts/ts_externalscripts/blob/develop/python/lsst/ts/externalscripts/base_make_calibrations.py#L1549 , but I agree this is clearer.

[r-owen]

This all works, but this is a classic use case for defaultdict:

import collections
...
counter = collections.defaultdict(lambda: 0)  # or defaultdict(int) if you prefer, since int() == 0
for test in fail_count:
    counter[test] += 1

[plazas]

thanks!

[r-owen]

I suggest returning a dataclass with 3 named attributes. Advantages:

• Clarifies the code and makes it more robust, because you reference attributes by name, not position.
• Adds flexibility: you can add more fields later without breaking calling code.
• Simplifies early return if you specify defaults for some items -- you only need to pass the items you actually know to the constructor.

thresholds is another good candidate for a dataclass, depending on what you do with it. (And you can trivially turn a dataclass instance into a dict using vars(my_dataclass_instance))

[plazas]

I turned the return variables into a dataclass and I see your points. As for the thresholds dictionary, it's only used once to get statistics to report in the Alarm, so I'm not sure if there's much difference. I'll be happy to do it in this case too if you still think it's better.

[r-owen]

I gently suggest it, but it's OK as is.

[plazas]

I turned thresholds into a dataclass too.

[r-owen]

Please don't keep extracting entry["uri"]. Do it once at the beginning.

I also suggest a regex to simplify the code:

import re
...
match = re.search(r"(verify(Bias|Dark|Flat)Stats)", entry["uri"])
if match is None:
    raise ...
verify_stats_str = match.groups()[0]

Or if you prefer to avoid regexes, consider a loop such as:

uri = entry["uri"]
for substr in ("verifyBiasStats", "verifyDarkStats", "verifyFlatStats"):
    if substr in uri:
        verify_stats_string = substr
        break
else:
    raise ...

The else clause in the for statement is a slightly obscure Python feature. You could avoid it by initializing verify_stats_string to None and testing for that after the loop.