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

Update MX beamlines to use new device factory - part 1 #975

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

rtuck99
Copy link
Contributor

@rtuck99 rtuck99 commented Jan 7, 2025

Fixes

Also fixes the following issues:

  • diamond_filter was not being instantiated because python generic device types were not recognised as valid device factories.
  • device_factory did not support usage with sync v1 ophyd devices

See also mx-bluesky PR

Instructions to reviewer on how to test:

  1. Dodal connect i03, i04 should still work

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}

@rtuck99 rtuck99 requested a review from a team as a code owner January 7, 2025 16:12
Copy link

codecov bot commented Jan 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.54%. Comparing base (bbd76c6) to head (ac0ec40).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #975      +/-   ##
==========================================
+ Coverage   97.48%   97.54%   +0.06%     
==========================================
  Files         148      148              
  Lines        6238     6279      +41     
==========================================
+ Hits         6081     6125      +44     
+ Misses        157      154       -3     

☔ 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.

Looks good, but I feel like we should put the part which fixes device_factory for v1 devices into a separate PR.

Also, for this issue we might as well convert all of the devices in i04, i24 and i23 to use the new instantiation

@olliesilvester olliesilvester changed the title 846 update mx beamlines to use device factory Update MX beamlines to use new device factory Jan 8, 2025
Comment on lines 96 to 97
@device_factory()
@skip_device(lambda: BL == "s03")
Copy link
Contributor

Choose a reason for hiding this comment

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

Depending on how mx-bluesky/hyperion is handling this:

Suggested change
@device_factory()
@skip_device(lambda: BL == "s03")
@device_factory(skip = BL == "s03")

Copy link
Contributor

Choose a reason for hiding this comment

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

or skip = (BL == "s03") if it's any clearer what is meant

Copy link
Contributor Author

Choose a reason for hiding this comment

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

At this point, I think we are likely going to remove all this s03 stuff soon as I think we have pretty much given up on the simulator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There is a separate issue to remove the s03 system tests and I have updated that to remove s03 completely

Comment on lines 310 to 324
@device_factory()
@skip_device(lambda: BL == "s03")
def flux(wait_for_connection: bool = True, fake_with_ophyd_sim: bool = False) -> Flux:
def flux(mock: bool = False) -> Flux:
"""Get the i03 flux device, instantiate it if it hasn't already been.
If this is called when already instantiated in i03, it will return the existing object.
"""
return device_instantiation(
Flux,
"flux",
"-MO-FLUX-01:",
wait_for_connection,
fake_with_ophyd_sim,
wait=False,
fake=mock,
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to be mixing the two device factory types?

Comment on lines +143 to +144
) -> Callable[[Callable[[], T]], DeviceInitializationController[T]]:
def decorator(factory: Callable[[], T]) -> DeviceInitializationController[T]:
Copy link
Contributor

Choose a reason for hiding this comment

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

This has to be D, Ophyd-Async only device, the connection logic does not work for Ophyd devices.

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 have made modifications so that the device factory will now work with ophyd v1 devices.

Copy link
Contributor

Choose a reason for hiding this comment

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

So you have, if you can't tell that I stopped looking when I reached this point ;)

I thought MX only had the Eiger left on ophyd v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Plus a few other minor devices, s4_slits, etc. There is currently a GH ticket to move those across but the Eiger is the main thing, it will probably be a little while yet before that gets moved over to ophyd_async. I thought better to make a start on this.

Once they are moved over, we will be able to get rid of a whole bunch of code including the device_instantiation stuff.

@DiamondJoseph DiamondJoseph dismissed their stale review January 9, 2025 16:14

Fingers quicker than brain and eyes

@rtuck99 rtuck99 changed the title Update MX beamlines to use new device factory Update MX beamlines to use new device factory - part 1 Jan 9, 2025
@rtuck99 rtuck99 force-pushed the 846_update_mx_beamlines_to_use_device_factory branch from 9c828ca to 7ec1f74 Compare January 10, 2025 15:55
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.

3 participants