ENH: comparisons and "atef check" command-line tool #23
Conversation
|
HXR EBD/FEE checkout helper PVs mapped to happi item + attribute: PVs not yet mapped to a happi item: |
|
Where does 'ready for beam' status fit into? |
|
For posterity, a quick script to load up the panels and correlate with whatrecord metadata: import json
import apischema
from atef import grafana
from typing import Tuple
def split_record_and_field(pvname) -> Tuple[str, str]:
"""Split REC.FLD into REC and FLD."""
record, *field = pvname.split(".", 1)
return record, field[0] if field else ""
happi_info = json.load(open("happi.json"))
json_doc = json.load(open("atef/tests/hxr_ebd_fee_checkout_helper.json"))
dashboard = apischema.deserialize(grafana.Dashboard, json_doc)
record_to_happi_item = happi_info["record_to_metadata_keys"]
happi_items = happi_info["metadata_by_key"]
for panel in dashboard.panels:
if isinstance(panel, grafana.RowPanel):
continue
try:
thresholds = panel.fieldConfig.defaults.thresholds
except AttributeError:
thresholds = None
targets = panel.targets_by_id
by_pv = {
split_record_and_field(target.target)[0]: target
for target in targets.values()
}
for pv, target in by_pv.items():
happi_item = record_to_happi_item.get(pv, None)
if not happi_item:
print(pv, "missing in happi")
else:
happi_item, *_ = happi_item
attrs = [
".".join((happi_item, info["signal"])).rstrip(".")
for info in happi_items[happi_item]["_whatrecord"]["records"]
if info["name"] == pv
]
steps = [(t.value, t.color) for t in thresholds.steps] if thresholds else None
print(target.target, "->", attrs[0], target.operator or "default operator", steps) |
|
CLI with Descriptions with attribute names still need some work (the warning above is missing which attribute failed, specifically). |
|
A little better, even if not 100% grammatical: |
|
Trees look decent and seem appropriate here: |
|
It's getting more difficult to focus on atef work recently, so I'm going to mark this as ready for review now. It's still really a work-in-progress, but there is enough here for feedback at least. Next steps:
|
klauer
changed the title
|
I can try to dive into a review here tomorrow |
| relative_module = f'.{module}' | ||
| return importlib.import_module(relative_module, 'atef.bin') | ||
| def _try_import(module_name): | ||
| return importlib.import_module(f".{module_name}", 'atef.bin') |
There was a problem hiding this comment.
What exactly is going on here? Why are we not importing these modules the normal way?
There was a problem hiding this comment.
This is the "easily support various subcommands through a simpleish interface" thing I used in a bunch of projects and added to the cookiecutter: https://github.com/pcdshub/cookiecutter-pcds-python/blob/master/%7B%7B%20cookiecutter.folder_name%20%7D%7D/%7B%7B%20cookiecutter.import_name%20%7D%7D/bin/main.py
The short of it is: add your module name to the list, add functions for creating the arg parser + main, and you're done. If it's not importable for some reason (optional dependencies? other import-related error?) it'll still allow you to use other subcommands.
There was a problem hiding this comment.
Alright, seems ok. I just found it really confusing/strange and I don't expect any linters to be able to pick up on typos in the COMMAND_TO_MODULE dict the same way they would for something much more straightforward e.g.
from . import check
from . import grafana
SUBMODULES = (check, grafana)
COMMANDS = {
module.__name__: module.build_arg_parser
for module in SUBMODULES
}
There was a problem hiding this comment.
I think that was the original implementation in pytmc, but then sometimes pyqt5 wasn't installed - which made every pytmc subcommand (even those that didn't require pyqt5) inaccessible.
I think I could be convinced the benefits of the static checker outweigh the benefits of the current implementation. Try/catch on import, don't add it to the commands dict. I'm only not sure about how to let the user know the reason that the subcommand isn't available without being too annoying.
It may be worth revisiting, but I think probably outside of the scope of the PR. (And in scope of 4ish other projects that use the same structure 😁 )
There was a problem hiding this comment.
Yeah, I mean, it confused me but if it does the thing it does the thing
|
|
||
| from . import serialization | ||
|
|
||
| Number = Union[int, float] |
There was a problem hiding this comment.
I'm surprised this isn't a built-in, it would get a lot of use...
There was a problem hiding this comment.
There is numbers.Number, but for whatever reason my static type checking tool was raising errors on valid inputs. It may have been on my end, and I'd be curious to hear what you see in your setup.
There was a problem hiding this comment.
I think I was also having trouble with that one last time I tried (in that big daq pr)
| success = 0 | ||
| warning = 1 | ||
| error = 2 | ||
| internal_error = 3 |
There was a problem hiding this comment.
I assume this is differentiating between "my device has an issue" and "my test has an issue", really good distinction
There was a problem hiding this comment.
Yes, exactly. I have notes elsewhere here about not knowing where "OK for beam" and "device seems OK" fits in, though.
There was a problem hiding this comment.
My two immediate thought threads on this are basically:
- Can we simply tag tests as "beam tests" and "device tests"?
- Can we include some sort of "consequence" enum in the failure information of a test?
not 100% sure here
There was a problem hiding this comment.
Hmm, I think the beam vs device test may be a good way to separate it out... I think we'll need to whiteboard this to really understand the implications of it all though.
| ReduceMethod.min: np.min, | ||
| ReduceMethod.max: np.max, | ||
| ReduceMethod.std: np.std, | ||
| }[self] |
There was a problem hiding this comment.
I've never considered this enum pattern before, it seems like it could be useful
|
|
||
|
|
||
| success = Result() | ||
| PrimitiveType = Union[str, int, bool, float] |
There was a problem hiding this comment.
This is another "why not in standard library" typing thing
| return f"{self.description} ({value_desc})" | ||
| return value_desc | ||
|
|
||
| def compare(self, value: PrimitiveType) -> bool: |
There was a problem hiding this comment.
Any thoughts on using dunder methods like __eq__ for things like this?
I'm not sure how I feel. Sometimes object == 5 feels nice, other times it gets confusing when comparing an object to a primitive.
There was a problem hiding this comment.
After reading on, it makes sense to keep this as a normal method, unless we wanted to go full confusion by enabling constructs like value in value_range
There was a problem hiding this comment.
An interesting thought, I think - but as your second comment notes, the grammar gets confusing for the non-equality-like variants. It could be named something else like "check" or "run" or...
| class ValueSet(Comparison): | ||
| """A set of values with corresponding severities and descriptions.""" | ||
| # Review: really a "value sequence"/list as the first ones have priority, | ||
| # but that sounds like a vector version of "Value" above; better ideas? |
There was a problem hiding this comment.
Maybe this one should be AnyValue and the next should be AnyPrimitive?
| cleanup: bool = True | ||
| ): | ||
| serialized_config = yaml.safe_load(open(filename)) | ||
| config = apischema.deserialize(ConfigurationFile, serialized_config) |
There was a problem hiding this comment.
Somehow I missed that this was possible- I'm giving apischema another look now. Very cool.
| """Clean up ophyd - avoid teardown errors by stopping callbacks.""" | ||
| dispatcher = ophyd.cl.get_dispatcher() | ||
| if dispatcher is not None: | ||
| dispatcher.stop() |
There was a problem hiding this comment.
Segfaults otherwise for short-lived sessions with trying-to-connect PVs 😬
I wonder if ophyd doesn't have an atexit hook for stopping the dispatcher - I could have sworn it did.
There was a problem hiding this comment.
ENH: comparisons and "atef check" command-line tool
Bullet points: