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

ENH: add restore functionality to snapshot restore page #97

Merged
merged 8 commits into from
Nov 12, 2024

Conversation

shilorigins
Copy link
Collaborator

@shilorigins shilorigins commented Nov 6, 2024

Description

  • Add "restore" button to restore page
  • Button opens dialog with all PVs and values in a table, plus accept button
    • PVs can be removed
    • Accept button writes all values in the table to the EPICS system
    • Accept button also turns on live data tracking, as a way of tracking restore progress

Motivation and Context

Restoring snapshots and other sets of values are a core feature of the application.

A dialog represents an action being configured and then activated, so I've used it to coordinate the restore action for now. Alternatives include filtering the RestorePage main PV table, but this could muddy the app's state in the mind of the user unless it's done well.

PVs can be removed from the restore dialog, but currently not added in. This covers cases where a whole or partial Snapshot needs to be restored, but not multiple Snapshots or cases where the user accidentally removes a PV. This is an acceptable first pass while the dialog / restore page continue to be developed.

An alternative is to replace the "Remove" push button with a radio button that marks the PV as active or inactive: inactive PVs would stay in the dialog table but only active PVs would be included in the restore. This is an easy way to make PVs re-includable, but may be visually busy for large snapshots. This method also lends itself to being used in the main table, obviating the need for a dialog at all.

How Has This Been Tested?

Interactively:

  1. run superscore demo
  2. right click "Accelerator Directorate" snapshot in the tree view and click "inspect values"
  3. click "Turn on Live Data"
  4. click "Restore" button
  5. click "Remove" button to un-stage PVs if desired
  6. click dialog's "Restore" button
  7. observe values changing in the RestorePage PV table

A second "linac" snapshot with different values has been added for comparing to the original snapshot.

I also added two test cases in test_page.py.

Where Has This Been Documented?

Screenshots (if appropriate):

Restore page before restore:
Screenshot 2024-11-07 at 09 39 52

Restore dialog:
Screenshot 2024-11-08 at 11 23 05

Restore page after restore:
Screenshot 2024-11-07 at 09 41 39

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@shilorigins shilorigins self-assigned this Nov 6, 2024
@shilorigins shilorigins added the GUI Graphical User Interface related issue label Nov 6, 2024
@shilorigins shilorigins force-pushed the devagr/restore-snapshot branch 4 times, most recently from 17e9d3b to 50d03fb Compare November 11, 2024 22:12
Dialog shows table of PV names and values to be restored. Rows
can be removed, but currently can't be added. Dialog appears to
only close when PVs are done being written.
If a PV appears in the demo fixture entries multiple times, the
IOC will hold the last value.
@shilorigins shilorigins force-pushed the devagr/restore-snapshot branch from 50d03fb to 687802d Compare November 11, 2024 22:38
@shilorigins shilorigins marked this pull request as ready for review November 11, 2024 22:52

@staticmethod
def prepare_attrs(entries: Iterable[Entry]) -> Mapping[str, pvproperty]:
def prepare_attrs(entries: Iterable[Entry], client: Client) -> Mapping[str, pvproperty]:
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I'm on board with requiring the IocFactory to also be passed a client, when all it's doing is gathering leaf entries. That means if we want to use this elsewhere, we need to construct a valid Client with a backend that holds the entries.

I know it's a bit repetitious, but here I think it could be justified

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't like it either. I don't mind re-implementing the gather method if we have to, but the entries being passed had UUID children that needed to be resolved. I can look into why the UUIDs were swapped in, but lazy loading feels like it's causing duplicate code in multiple places, and maybe we should revisit its implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed 100%. I think throughout the UI it's not unreasonable to expect the client to exist, and call its .fill() method or something similar. (Maybe we add a filled=True optional argument to Client.search() or something as well)

I think in the test suite unless elsewise specified it's ok to have all the Entry's be filled. I think if you ever use the Filestore backend you'll end up getting lazy Entry's

superscore/tests/test_page.py Outdated Show resolved Hide resolved
superscore/tests/test_page.py Show resolved Hide resolved
superscore/widgets/page/restore.py Outdated Show resolved Hide resolved
superscore/widgets/page/restore.py Show resolved Hide resolved
superscore/widgets/page/restore.py Show resolved Hide resolved
tangkong
tangkong previously approved these changes Nov 12, 2024
close() allows the garbage collector to manage cleanup, and the change
doesn't affect performance to the user
@shilorigins shilorigins merged commit f27ddc8 into pcdshub:master Nov 12, 2024
11 checks passed
@shilorigins shilorigins deleted the devagr/restore-snapshot branch November 12, 2024 23:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GUI Graphical User Interface related issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants