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

746 blueapi does not call connect on all devices in a dodal beamline #751

Conversation

ZohebShaikh
Copy link
Contributor

@ZohebShaikh ZohebShaikh commented Dec 10, 2024

closes #746

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.20%. Comparing base (71d536a) to head (d6b40f7).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #751      +/-   ##
==========================================
+ Coverage   93.10%   93.20%   +0.09%     
==========================================
  Files          36       37       +1     
  Lines        2030     2074      +44     
==========================================
+ Hits         1890     1933      +43     
- Misses        140      141       +1     

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

docs/explanations/decisions/0005-connect-devices.md Outdated Show resolved Hide resolved
src/blueapi/core/context.py Outdated Show resolved Hide resolved
src/blueapi/core/context.py Outdated Show resolved Hide resolved
Comment on lines 131 to 138
def is_simulated_device(name, factory, **kwargs):
device = devices.get(name, None)
mock_flag = kwargs.get("mock", kwargs.get("fake_with_ophyd_sim", False))
return device is not None and (
isinstance(factory, DeviceInitializationController)
and (factory._mock or mock_flag) # noqa: SLF001
and isinstance(device, OphydV1Device | OphydV2Device)
)
Copy link
Contributor

Choose a reason for hiding this comment

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

What we're seeing here is a consequence of a discussion @DiamondJoseph and I had about DiamondLightSource/dodal#881.

The mock parameter of the API of DeviceInitializationController is meaningless unless connect_immediately=True, it is just ignored otherwise and you have to go digging through the factory to find out if it was set.

You can argue that it should actually be called mock_if_connect_immediately or similar.

This is confusing and makes this use case hard so it should be addressed. I can't think of a better way to do this at the moment so we will have to keep it but please make an issue on dodal explaining that you found this hard to implement with the DeviceInitializationController API and a corresponding issue in blueapi to simplify this code when we can.

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'll mention this in a ticket in dodal

src/blueapi/startup/example_devices.py Outdated Show resolved Hide resolved
src/blueapi/utils/connect_devices.py Outdated Show resolved Hide resolved
tests/system_tests/devices.json Outdated Show resolved Hide resolved
src/blueapi/utils/connect_devices.py Show resolved Hide resolved
Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

Almost there, thanks for bearing with

tests/unit_tests/core/fake_device_module.py Outdated Show resolved Hide resolved
@ZohebShaikh ZohebShaikh merged commit 211bb49 into main Dec 12, 2024
29 checks passed
@ZohebShaikh ZohebShaikh deleted the 746-blueapi-does-not-call-connect-on-all-devices-in-a-dodal-beamline branch December 12, 2024 15:28
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.

Blueapi does not call connect on all devices in a dodal beamline
2 participants