pcdshub / atef Goto Github PK

Expected Behavior

PV names should always display as-is in the terminal.

Current Behavior

If a PV names makes it into an output and it has specific emoji names associated with it, the emoji will be rendered in the text. This is because :GEM: and similar constructs are actually the normal (?) shortcuts for these.

Expected Behavior

The config GUI should have a consistent, readable look and feel on monitors with various DPI specifications.

Current Behavior

The GUI looks awful on the 4K monitor at my desk at SLAC. Inconsistent sizing and clipping.

Ref #16

• Procedure steps and groups thereof
• Procedure steps should be implemented as dataclasses with self-describing type hints
• Steps should be serializable
• Steps should be viewable as custom atef widgets
• Procedure should be viewable as a scrollable page of sorts (familiar to web browser users + typhos device displays)

For starters:

• Description: narrative - describe what to do
• Code: run some arbitrary code (locally? on queue server if possible?)
• Open a typhos display (by happi name / class+kwargs)
• Open a pydm display
• Show a device configuration / comparison (pass/fail criteria could be used to continue)
• Run a bluesky plan (with some/all arguments customizable)
• Group: group any and all of the above

To consider:

• Per-step check box for user to acknowledge step completion?

(Separate issues to be made for: report generation tools, qserver stuff, ...)

We should lock down which file format we want to use before deploying the beta
Currently I have picked json because it's faster than yaml to load and we're using a gui anyway
I did not consider other options

Expected Behavior

When we delete a configuration, checklist, or comparison, ask the user for confirmation first.

initial reaction is that there probably should be a confirmation dialog prior to deleting.
(If we had a bunch of time, undo/redo would be amazing, but... yeah)

Originally posted by @klauer in #57 (comment)

Current Behavior

To delete an item, you currently need to select and clear the name field, which will cause a delete button to appear. Clicking the delete button in this state will immediately delete the entire section of the tree, even if there are meaningful sub-items.

Possible Solution

Add a standard qt confirmation dialog.
Make this pop up either all the time or specifically when the underlying data structure is "non-empty".

Expected Behavior

The code for saving/loading files should not be embedded in the GUI
We need to be able to load these files in atef check

Current Behavior

The GUI defines its own save/load routines

Possible Solution

Refactor to a shared submodule

straightforward e.g.

from . import check
from . import grafana

SUBMODULES = (check, grafana)

COMMANDS = {
    module.__name__: module.build_arg_parser
    for module in SUBMODULES
}

Originally posted by @ZLLentz in #23 (comment)

Is there any way we can get access to the device list from the IdAndCompWidget/bridge in order to pre-fill the search widget again?

It's a totally different dataclass object, so I think we need to add an argument to IdAndCompWidget to accept the QDataclassList with the device names in it. And then we need to add that into the widget_args for the AtefItem that sets up the IdAndCompWidget

Originally posted by @ZLLentz in #70 (comment)

Additional thoughts on locking:

• Locking only applies with the top-level overview and not configs/comparisons
• Ideally, a lock would apply to everything underneath where it is configured in the tree
• Should locking be stored in a configuration file?
• Should there just be a read-only mode?
• I personally like how macOS does the lock thing which applies to the current page/descendents:

Originally posted by @klauer in #28 (comment)

Expected Behavior

Display no +/- range field for the equality checks of strings

Current Behavior

When entering a string this behaves appropriately, but when loading a string from a file it does not.

Expected Behavior

Every config widget should have a button to go "up" to the parent tree node.
Every config widget with children should have buttons to go "down" to that child note.

Current Behavior

No such behavior

Context

Ease of use for the GUI

Expected Behavior

Keyboard navigation should work

Current Behavior

Keyboard navigation does not work

Context

Bug:
Keyboard navigation doesn't work on the tree. Perhaps the wrong signal?

Originally posted by @klauer in #28 (comment)

Expected Behavior

Some of these files were scrapped and I've lost track of which ones. We can grep for their titles and delete the unused ones.

Expected Behavior

I should be able to remove configs, checklists, and comparisons using the GUI.

Current Behavior

No delete buttons yet.

Context

Required for GUI completeness.

Related comments:

I think this todo is still a thing, so issue creating time:

Add a way to delete configurations in the atef config gui.

Originally posted by @klauer in #28 (comment)

Expected Behavior

These widgets should initialize with the devices and components I selected last time

Current Behavior

They do not

Expected Behavior

If we raise an exception, don't immediately exit the GUI

Current Behavior

Undiagnosed buggy behavior causes the UI to crash, losing progress

See title
Gets long really fast

Thoughts on alarms

• Could atef verify that a device has passed NALMS checks - without reduplicating its effort?
• Would NALMS entries have to be structured in a way that tied it back to happi device names?
• Does atef need to be cognizant of alarm states in general? ophyd does not currently allow for easy access to such information, unfortunately

Based on #23 (comment)

Idea 1

• Checks may indicate "ready for beam" or "device is OK" or ... (other scenario)
• A failure on a check that's tagged "beam" could mean "device is not ready for beam"

Idea 2

• Consequences in addition to severity?

Expected Behavior

QDataclassBridge should be able to handle a diverse spectrum of annotation structures.

Current Behavior

QDataclassBridge is hard-coded for the cases that are relevant to the atef config gui

Possible Solution

From @klauer:

I think that better (?) way would just be to use typing.get_type_hints, get_origin, and get_args. But that doesn't have to happen in this PR.

e.g.,

In [12]: typing.get_type_hints(atef.check.Configuration)
Out[12]:
{'name': typing.Optional[str],
 'description': typing.Optional[str],
 'tags': typing.Optional[typing.List[str]],
 'checklist': typing.List[atef.check.IdentifierAndComparison]}

In [13]: tags = typing.get_type_hints(atef.check.Configuration)["tags"]

In [14]: typing.get_origin(tags)
Out[14]: typing.Union

In [15]: typing.get_args(tags)
Out[15]: (typing.List[str], NoneType)

Originally posted by @klauer in #28 (comment)

Context

The existing implementation is a code re-use and maintainability hazard

It would be a shame to lose a lot of effort in designing your perfect atef comparison configuration to a software bug or a failed X forwarding session or anything else for that matter.

I wonder if we should be periodically saving .cfg.wip when possible just in case of accidental closure/bug/etc. This is a fair amount of work, though, and could be annoying with nfs-related saving delays.

This is worth some further thought / discussion.

Originally posted by @klauer in #28 (comment)

Expected Behavior

Open file -> the whole tree is populated

Current Behavior

Open file -> the tree is only populated as you click on existing elements (each click generates the next layer), at some point it freezes

Possible Solution

1. Stop lazy loading the GUI elements? Each element is responsible for extending the tree widget slightly.
2. Look for places where we could get stuck in a bad loop

Expected Behavior

match_line_edit_text_width would be easier to use/parametrize/understand if the minimum width and offset elements were in units of "characters" instead of pixels.

Current Behavior

Units of pixels

Possible Solution

use QFontMetrics.averageFontWidth() as a metric for converting characters to pixels

Context

Just a thought:
Maybe 'minimum width in number of characters based on the metrics' would be a bit more intuitive than dealing in pixels here. I think then it'd be like getting the font metrics of a string with min xs ("x" * min)

Originally posted by @klauer in #61 (comment)

Expected Behavior

I should be able to serialize the subclasses of atef.check.Comparison using API schema

Current Behavior

For the instances used in the ATEF config GUI, we get this error:

ConfigurationFile(configs=[DeviceConfiguration(name='LFE Imagers', description='All checks associated with the LFE imagers.', tags=None, checklist=[IdentifierAndComparison(name='Rate checks', ids=['event_rate', 'image_rate'], comparisons=[Equals(name='check nonzero', description='Error if any of the rates are zero', invert=True, reduce_period=None, reduce_method=<ReduceMethod.average: 'average'>, string=None, severity_on_failure=<Severity.error: 2>, if_disconnected=<Severity.error: 2>, value=0, rtol=None, atol=None), None]), None, IdentifierAndComparison(name='Event code checks', ids=['event_code'], comparisons=[Equals(name='low rate', description='check that the event code is one of the sensible values', invert=False, reduce_period=None, reduce_method=<ReduceMethod.average: 'average'>, string=None, severity_on_failure=<Severity.error: 2>, if_disconnected=<Severity.error: 2>, value=0, rtol=None, atol=None), None]), None, IdentifierAndComparison(name='Removed checks', ids=['state.state'], comparisons=[Equals(name='check removed', description='fail if the imager is in the beam during pre-beam checkout', invert=False, reduce_period=None, reduce_method=<ReduceMethod.average: 'average'>, string=None, severity_on_failure=<Severity.error: 2>, if_disconnected=<Severity.error: 2>, value=0, rtol=None, atol=None), None]), None], devices=['im1l0', 'im2l0', 'im3l0', 'im4l0'])])
ERROR:atef.bin.config:Error serializing file
Traceback (most recent call last):
  File "C:\Users\zlentz\Documents\VSCode\atef\atef\bin\config.py", line 474, in serialize_tree
    return serialize(
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\utils.py", line 546, in wrapper
    return wrapped(*args, **kwargs)
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 470, in serialize
    return serialization_method_factory(
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 244, in method
    return wrapped_exclude_unset(
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 201, in method
    result[alias] = field_method(attr)
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 138, in method
    return [serialize_value(elt) for elt in obj]
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 138, in <listcomp>
    return [serialize_value(elt) for elt in obj]
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 365, in method
    return serialize_conv(converter(obj))
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 244, in method
    return wrapped_exclude_unset(
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 201, in method
    result[alias] = field_method(attr)
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 310, in method
    raise error or TypeError(
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 300, in method
    return alt_method(obj)
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 244, in method
    return wrapped_exclude_unset(
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 201, in method
    result[alias] = field_method(attr)
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 138, in method
    return [serialize_value(elt) for elt in obj]
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 138, in <listcomp>
    return [serialize_value(elt) for elt in obj]
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 244, in method
    return wrapped_exclude_unset(
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 201, in method
    result[alias] = field_method(attr)
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 138, in method
    return [serialize_value(elt) for elt in obj]
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 138, in <listcomp>
    return [serialize_value(elt) for elt in obj]
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\serialization\__init__.py", line 365, in method
    return serialize_conv(converter(obj))
  File "C:\Users\zlentz\Documents\VSCode\atef\atef\serialization.py", line 95, in <lambda>
    lambda obj: tagged_union(**{obj.__class__.__name__: obj}),
  File "C:\Users\zlentz\Miniconda3\envs\dev\lib\site-packages\apischema\tagged_unions.py", line 122, in __init__
    raise TypeError(f"{type(self)} has no tag {tag}")
TypeError: <class 'types.Comparison'> has no tag NoneType

Context

Needed for the GUI to be useful at all

Expected Behavior

Comparison listing: a summary in a tooltip or a widget to the side might be nice

Originally posted by @klauer in #71 (comment)

Expected Behavior

atef config my_config.json should open up that file immediately without any dialog prompt.

Current Behavior

no cli args

Context

ease of use QOL update
related comments:

atef config doesn't support a filename just yet on the command-line. That argument should go here.

Originally posted by @klauer in #28 (comment)

main is where your argparse arguments would go, when filename (or filenames, perhaps) is added to the command-line options

Originally posted by @klauer in #28 (comment)

Expected Behavior

Description textboxes shouldn't have a scrollbar at one line- it should be no scrollbar at all (expanding for each new line) or it should be scrollbar after N lines (N in 3-5?)

Current Behavior

I noticed this specifically when opening a display from a file:

• Right click a device configuration and add a checklist underneath it
• Right click a checklist and add a comparison underneath it
• Right click a checklist/comparison and copy/paste it elsewhere

Originally posted by @klauer in #28 (comment)

Expected Behavior

Rather than parsing strings from a single line edit in different ways depending on the type we want, it'd be more intuitive if the widget was swapped with a more directly helpful variant.

bool -> combobox with true/false
int -> spinbox
float -> doublespinbox
string -> as is, line edit

Current Behavior

Currently we have a line edit that is re-interpreted based on the user's desired type

Possible Solution

Create four widgets in the ui file, hide all but one and show the relevant one at all times. Connect each widget's value edit signal equivalent to a slot that updates the dataclass bridge. Update all four widgets when the dataclass bridge chases to the extent at which this is possible.

Context

Hmm, could we instead create an appropriate widget based on the selected data type?
bool -> checkbox, str -> line edit, int/float -> spinbox/double spinbox

Originally posted by @klauer in #61 (comment)

Steps to Reproduce (for bugs)

1. atef config
2. open
3. cancel
4. See screenshot

Current Behavior

Confusing on macOS - much less so on Windows with its attached file menu. More details below.

Expected Behavior

A crash has been reported on tag deletion

Current Behavior

This may or may not be the right widget for it, but I tried to delete the last tag and crashed with the following:

(atef) PC98125:atef klauer$ atef config
Traceback (most recent call last):
  File "/Users/klauer/Repos/atef/atef/bin/config.py", line 1197, in eventFilter
    if object.text():
AttributeError: 'QWidget' object has no attribute 'text'

Originally posted by @klauer in #28 (comment)

Expected Behavior

it'd be nice if double-click worked for selection of device names and component names. I thought we had made it this way but it isn't working for components for me.

Originally posted by @klauer in #71 (comment)

Expected Behavior

We should be able to copy/paste a bunch of PV config PVs at once.

Current Behavior

One at a time

Possible Solution

Multiline text box?

Context

It takes a long time to build out a PV config

Current Behavior

Some default sizing issues (worthy of an issue, probably); do we need more splitters perhaps? Screenshot below

Originally posted by @klauer in #71 (comment)

Expected Behavior

There should be a way to check historical data for a particular signal when you're selecting it to help you design your test.

Current Behavior

No such option

Context

My interpretation of something Alex mentioned during our quick 1-on-1 catchup meeting when I showed him some atef progress

Your Environment

There is some discussion here:

• #14
• #4

This came up again during our short demo. We should re-evaluate the feature.

Special consideration should be made as to where such information might be stored - long-term, should it be atef or another database?

Current Behavior

In the configuration GUI, you have to type the device name / component name directly and hope you have it correct.

Proposed solution

Prototype a happi search widget that allows for - in order of priority:

1. Filtering/finding of happi devices, at least by name (some parts may exist already: https://github.com/pcdshub/happi/blob/master/happi/qt/model.py and require customization)
2. Viewing and selecting device components by name
3. Viewing the current state of the device - or via ArchivedDevice, at a selected time

This can be prototyped here and moved elsewhere as appropriate. At least (1) can belong in happi itself.

Context

Configuration GUI discussions.

Expected Behavior

We should be able to add many devices at once

Current Behavior

The device selector doesn't support multi-selection yet

Context

It's kind of cumbersome to create a config of "all the valves"

Expected Behavior

No crashes

Current Behavior

Bug:

(atef) PC98125:atef klauer$ atef config
Traceback (most recent call last):
  File "/Users/klauer/Repos/atef/atef/bin/config.py", line 574, in show_selected_display
    widget = item.get_widget()
  File "/Users/klauer/Repos/atef/atef/bin/config.py", line 620, in get_widget
    self.widget_cached = self.widget_class(*self.widget_args)
  File "/Users/klauer/Repos/atef/atef/bin/config.py", line 1231, in __init__
    self.initialize_idcomp()
  File "/Users/klauer/Repos/atef/atef/bin/config.py", line 1235, in initialize_idcomp
    self.initialize_config_name()
  File "/Users/klauer/Repos/atef/atef/bin/config.py", line 817, in initialize_config_name
    self.bridge.name.changed_value.connect(self.name_edit.setText)
RuntimeError: wrapped C/C++ object of type QDataclassValueForstr has been deleted

I was clicking around, alt-tabbed back to the PR, and saw the crash above

Originally posted by @klauer in #28 (comment)

Some upstream archiver appliance enhancements will be required for control metadata to be propagated through the get_snapshot interface of archapp. Murali is working on this and has indicated that it may be ready (and deployed) sometime this month.

When that happens, revisit this code, and ensure that the control metadata is propagated appropriately to downstream consumers (primarily the mock epics.PV class)

Expected Behavior

Add a specific comparison for common "value is changing over time checks", to make it very clear in the reports for these sorts of checks what is going on.

Current Behavior

You can emulate this sort of check by reducing to the standard deviations and playing other small tricks.

Expected Behavior

I should be able to:

1. Select at2l0
2. Select a few components from at2l0
   And generate a configuration that has values pre-filled in with e.g., an equality check set.

With archiver integration, this could also be used to get values from a certain point in time.

Expected Behavior

All classes and methods should have descriptive docstrings with argument and return types.

Current Behavior

Some classes are missing this information.

Context

Required for code maintainability

Sizing issue for config_group.ui for tags:

Originally posted by @klauer in #28 (comment)

Add shortcuts for saving (ctrl-s), loading, and other common actions

Originally posted by @klauer in #28 (comment)

See #14

• Compare difference between two devices of the same underlying class - live devices or archived devices
• Simplest solution could be a tabular view of values.
• Multiple devices could be compared if desirable
• Non-archived values may be skipped (or may require additional consideration)

Expected Behavior

Some of the comparison reductions, particularly the ones that don't map nicely onto the original values such as stddev, should show up in the repr for the report output.

Current Behavior

The following output happens right now for a check that the standard deviation is greater than 1, which is misleading:

├── ✔ Success: im1l0.detector.image_counter > 1.0

Expected Behavior

It should be easy to save our value as the desired type

Current Behavior

I had some strange issues where my values got saved as "0.0" string type- I can't remember exactly what I was trying to do in these cases. I think I might have been trying to set 0 integers? Needs investigation.

Expected Behavior

We should have a widget set up for each comparison subclass in atef.check that can be used to modify the dataclass. This widget should be implemented in atef.widgets.config as appropriate, expecting a single QDataclassBridge as an initialization argument (alongside standard qt arguments like parent)

Checklist for how I expect the PRs to be split up:

• Equals/NotEquals
• ValueSet
• AnyValue
• AnyComparison
• Greater/GreaterOrEqual/Less/LessOrEqual
• Range

Current Behavior

There are only placeholder widgets at this time.

Context

Required for atef config GUI to be useful.