Skip to content

Update p45 to ophyd_async devices, device_factory#919

Merged
DominicOram merged 4 commits intomainfrom
update-p45
Mar 6, 2025
Merged

Update p45 to ophyd_async devices, device_factory#919
DominicOram merged 4 commits intomainfrom
update-p45

Conversation

@DiamondJoseph
Copy link
Contributor

Updates p45 device classes to be ophyd-async compatible, replaces use of device_instantiation with @device_factory.

Instructions to reviewer on how to test:

  1. Check status of IOCs on p45
  2. If IOCs on p45 in good shape, test connections with dodal connect

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}

@codecov
Copy link

codecov bot commented Nov 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.69%. Comparing base (c807a20) to head (d479ec0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #919   +/-   ##
=======================================
  Coverage   97.69%   97.69%           
=======================================
  Files         160      160           
  Lines        6635     6638    +3     
=======================================
+ Hits         6482     6485    +3     
  Misses        153      153           

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@DominicOram
Copy link
Contributor

I would be happy to review this but don't really use P45 so wouldn't know how to do Check status of IOCs on p45. I also think I can't connect to the PVs remotely so would be unsure how to test the connect? Could we add some docs (or a link to some if they already exist) on the basics of using P45 at the top of p45.py?

@DiamondJoseph
Copy link
Contributor Author

p45 has been mildly on fire (literally melted some of the server and some of the IOCs are unplugged) recently, so this is fairly low priority. I'm happy to close and maybe even delete the p45 beamline for now.

@DominicOram
Copy link
Contributor

DominicOram commented Mar 6, 2025

p45 has been mildly on fire (literally melted some of the server and some of the IOCs are unplugged) recently, so this is fairly low priority. I'm happy to close and maybe even delete the p45 beamline for now.

I would either merge this as is w/o necessarily being tested or remove p45 all together, up to you. Depends on if p45 is coming back soon or not. Either way, leaving the old style instantiation around is bad.

@DiamondJoseph DiamondJoseph requested a review from a team as a code owner March 6, 2025 16:13
@DiamondJoseph
Copy link
Contributor Author

Let's merge it, they're all simple devices and after rebasing it looks like it's just the DeviceFactory change which is pretty uncontroversial now.

@DominicOram
Copy link
Contributor

Let's merge it, they're all simple devices and after rebasing it looks like it's just the DeviceFactory change which is pretty uncontroversial now.

Ok, I will review it quickly now for obvious problems, without testing against the real thing

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

As discussed, hard to test properly with p45 not up but looks correct.

@DominicOram DominicOram merged commit f2862e8 into main Mar 6, 2025
19 checks passed
@DominicOram DominicOram deleted the update-p45 branch March 6, 2025 17:03
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