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

Hyperion 1474: Save the Panda #702

Merged
merged 10 commits into from
Aug 7, 2024

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Jul 23, 2024

Part of DiamondLightSource/hyperion#1474

Instructions to reviewer on how to test:

  1. This should install a new cli command save-panda, that when run on a beamline workstation with the name of the panda device and an output filename, will save the device state as a yaml file

Checks for reviewer

  • Would the PR title make sense to a scientist on a set of release notes
  • If a new device has been added does it follow the standards
  • If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly
  • Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.33%. Comparing base (0df1ecd) to head (6c7f7a4).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #702      +/-   ##
==========================================
+ Coverage   94.24%   94.33%   +0.09%     
==========================================
  Files         110      111       +1     
  Lines        4399     4469      +70     
==========================================
+ Hits         4146     4216      +70     
  Misses        253      253              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

My main comment is I think this should be specifically for saving the panda, rather than a generic device save. Let me know if you disagree. Also, we should protect against overwriting a previous yaml file

src/dodal/devices/util/save_panda.py Show resolved Hide resolved
Copy link
Collaborator

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

Couple more comments, sorry. Also, CI is complaining about lines not being covered by tests

Comment on lines 50 to 68
module_name = module_name_for_beamline(beamline)
devices, exceptions = make_all_devices(
Copy link
Collaborator

Choose a reason for hiding this comment

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

We only need to create the panda device here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was being lazy - we don't seem to have a nice function for creating only one named device, only one for creating everything. I will create one.

src/dodal/devices/util/save_panda.py Show resolved Hide resolved
@rtuck99 rtuck99 force-pushed the hyperion_1474_panda_gridscan_smargon_speed branch from 24d0ade to c516272 Compare August 6, 2024 15:51
Copy link
Collaborator

@olliesilvester olliesilvester left a comment

Choose a reason for hiding this comment

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

This one spiralled a bit, sorry - looks good I think! Longer term it might be nice to have something similar to the dodal connect command, like dodal save_device args, but this is fine for now. Also, because we can't connect to PVA signals from the office network, this will only work on beamline machines.

@rtuck99 rtuck99 merged commit b88c415 into main Aug 7, 2024
18 checks passed
@rtuck99 rtuck99 deleted the hyperion_1474_panda_gridscan_smargon_speed branch August 7, 2024 14:59
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.

2 participants