Correct return types to match current bluesky protocols #1248

NoxHarmonium · 2025-05-14T06:10:03Z

As per #1232 we have noticed that a lot of Ophyd objects such as Signal flag a lot of issues with type checkers when you use them with Bluesky plans, but they work fine at runtime.

This is due to a mismatch between the Ophyd types and the Bluesky protocols (https://github.com/bluesky/bluesky/blob/5f5d2c1db8ee5c52fbc9a5f9e4fb3513b2d8625e/src/bluesky/protocols.py).

This is my attempt at updating the types in Ophyd to keep the type checker happy. Sadly in a lot of the cases the fix was to make the types looser, rather than stricter. This is because we can't bring the Bluesky/event-model types into Ophyd (at least as far as I know, maybe we can?) so we would have to copy/paste heaps of types around and they might get out of sync. Instead I just made a lot of things Any so the type checker can infer/ignore things.

There were tests that were designed to make sure that the Ophyd objects remained compatible with the Bluesky protocols, but somewhere along the line they stopped working so I've given them an uplift and added pyright as a typechecker option since it is pretty popular now being part of VSCode via pylance.

There was one protocol (Subscribable) that I couldn't get Ophyd to conform too and that I'll need some extra guidance on (see the PR comments)

I have tested it with one of our beamline libraries, and it reduced the number of type errors we were flagging from 133 -> 0 🎉 . However, I am still a bit worried about changing the public API of base Ophyd objects like Device and Signal as people may be overriding them in different ways. I'd love your opinion on that.

I'll try and annotate the PR to help with the review.

Let me know what you think!

Before:

After:

- Also add pyright as a type checker to flush out any differences between pyright/mypy

NoxHarmonium · 2025-05-14T06:11:39Z

ophyd/__init__.py

@@ -1,5 +1,3 @@
-# type: ignore


I'm not sure why this type ignore statement was here, as well as the one at the top of device.py.
It was preventing mypy from traversing the submodules properly so I removed it. It didn't seem to break anything in the build, or when I imported it in to our beamline library so maybe its OK?

Removal of this wouldn't break anything other than type checking, which CI here doesn't do.

The question is whether it breaks type checking anywhere downstream using these methods... which I don't think it would since the methods aren't type hinted 🤔

NoxHarmonium · 2025-05-14T06:13:13Z

ophyd/device.py

 DEFAULT_CONNECTION_TIMEOUT = object()


-class OrderedDictType(Dict[A, B]):


I think this was here for compatibility with Python < 3.8
IIRC, it was the source of most of the type issues.
Since 3.8 is the minimum target now, I think we can just replace it with TypedDict

I think we can just replace it with dict by itself? We don't need the OrderedDict since insertion order would work fine.

NoxHarmonium · 2025-05-14T06:14:35Z

ophyd/utils/types.py

+from typing import List, Literal, Tuple, TypedDict
+
+
+class Hints(TypedDict, total=False):


This was the one type that I brought over from Bluesky because it was kind of small and I couldn't find a way around it. Typing the return type of hints() as Any or dict didn't seem to work.

Can we not use bluesky as a dependency and use the protocols like ophyd-async does?

@danielballan thoughts?

NoxHarmonium · 2025-05-14T06:15:56Z

ophyd/tests/test_bluesky_protocols.py

+@pytest.mark.skip()
+def test_subscribable(run):
    cmd = imports + "from bluesky.protocols import Subscribable;"
-    run(cmd + "foo: Subscribable = hw.signal1")


signal1 doesn't seem to exist. It may have been removed at some point (or maybe it never existed) but the type checker wasn't picking it up because --disallow-any-unimported wasn't on

NoxHarmonium · 2025-05-14T06:17:23Z

ophyd/tests/test_bluesky_protocols.py

    run(cmd + "foo: Flyable = sim.TrivialFlyer()")


-def test_hinted():


Replaced with test_hashints because Hinted doesn't exist anymore

NoxHarmonium · 2025-05-14T06:21:49Z

ophyd/tests/test_bluesky_protocols.py



-def test_subscribable():
+# TODO: Ophyd signature is incompatible with bluesky protocol


I have a feeling that Ophyd devices shouldn't conform to the Subscribable protocol as the concept of subscription is different between the two libraries.

The ophyd signature looks like this:
def subscribe(self, callback, event_type=None, run=True) -> int
and the Bluesky protocol looks like this:
def subscribe(self, function: Callback[T]) -> None

They seem to work differently in that Ophyd returns a cid when you subscribe which you can use as a token to unsubscribe. Bluesky doesn't give you a cid and unsubscribing works by passing the callback.

Hmmm I don't know if the cid is useful here?

It's not like it would be accessible from the private _temp_callback_ids.

I'm not really sure what we need event_type for here either:

https://github.com/bluesky/bluesky/blob/5f5d2c1db8ee5c52fbc9a5f9e4fb3513b2d8625e/src/bluesky/suspenders.py#L67-L68

@tacaswell

Now that I think about it, are the concepts even the same between Bluesky and Ophyd? In Ophyd a subscription is for watching for changes in device attributes, and in Bluesky a subscription is for handling events (e.g. with BestEffortCallback)

FWIW I know in our code base we have instances where we use event_type when subscribing to an Ophyd device to filter what events we want to receive and we store the CID to unsubscribe later.

NoxHarmonium · 2025-05-14T06:23:36Z

ophyd/device.py

+    # Placeholder required for devices to conform to the Bluesky Moveable protocol
+    # The actual implementation will be provided by sub classes
+    def set(self, *args, **kwargs) -> Any:
+        return super().set(*args, **kwargs)


These placeholder set() functions that just forward on to super() don't seem to break any tests so they seem OK but might need some extra scrutiny because they change the public API I suppose

Why is this needed? If set is just calling super's set, and it hasn't refined the return type at all, then what is it for? If we leave this out thensuper()'s set would be used instead anyway.

Good point. It made the type checker happy, but now that I look again, I don't think the base classes even have an implementation of set. Component doesn't even have a base class.

I'll have to have another look at this. Maybe adding set() to BlueskyInterface would make more sense.

evvaaaa

I'm fairly sure we have to use Dict[] from typing instead of my suggested dict[] in python 3.8.

Also my suggestions on using the protocols themselves are dependent on if we decide to include bluesky as a dependency.

evvaaaa · 2025-05-20T12:43:00Z

ophyd/__init__.py

@@ -1,5 +1,3 @@
-# type: ignore


Removal of this wouldn't break anything other than type checking, which CI here doesn't do.

The question is whether it breaks type checking anywhere downstream using these methods... which I don't think it would since the methods aren't type hinted 🤔

evvaaaa · 2025-05-20T12:51:57Z

ophyd/device.py

 DEFAULT_CONNECTION_TIMEOUT = object()


-class OrderedDictType(Dict[A, B]):


I think we can just replace it with dict by itself? We don't need the OrderedDict since insertion order would work fine.

evvaaaa · 2025-05-20T12:54:22Z

ophyd/device.py

        pass

-    def read(self) -> OrderedDictType[str, Dict[str, Any]]:
+    def read(self) -> OrderedDict[str, Any]:


I think we can use the TypedDict bluesky protocols here and for the rest of the Component methods, like ophyd-async is doing.

It would be great if we could just make bluesky a dependency like in ophyd-async. Then we could just use all the Bluesky types. I was assuming that we were trying to avoid coupling Ophyd to Bluesky but if not I'll re-do this PR. I suppose it might not be a breaking change as long as the version constraints were not too strict.

We can wait and see. I suppose the difference is that bluesky doesn't require ophyd-async 🤔

evvaaaa · 2025-05-20T12:56:46Z

ophyd/device.py

        return res

-    def describe_configuration(self) -> OrderedDictType[str, Dict[str, Any]]:
+    def describe_configuration(self) -> OrderedDict[str, Any]:


dict[str, dict[str, Any]] would be preferred, but we can use the DataKey TypedDict directly.

evvaaaa · 2025-05-20T13:02:49Z

ophyd/device.py

+    # Placeholder required for devices to conform to the Bluesky Moveable protocol
+    # The actual implementation will be provided by sub classes
+    def set(self, *args, **kwargs) -> Any:
+        return super().set(*args, **kwargs)


Why is this needed? If set is just calling super's set, and it hasn't refined the return type at all, then what is it for? If we leave this out thensuper()'s set would be used instead anyway.

evvaaaa · 2025-05-20T13:44:20Z

ophyd/tests/test_bluesky_protocols.py



-def test_subscribable():
+# TODO: Ophyd signature is incompatible with bluesky protocol


Hmmm I don't know if the cid is useful here?

It's not like it would be accessible from the private _temp_callback_ids.

I'm not really sure what we need event_type for here either:

https://github.com/bluesky/bluesky/blob/5f5d2c1db8ee5c52fbc9a5f9e4fb3513b2d8625e/src/bluesky/suspenders.py#L67-L68

@tacaswell

evvaaaa · 2025-05-20T13:48:13Z

ophyd/tests/test_bluesky_protocols.py

    run(cmd + "foo: Subscribable = hw.flyer1")


 if __name__ == "__main__":


I think we can remove this if __name__ == "__main__" section.

evvaaaa · 2025-05-20T13:49:59Z

ophyd/tests/test_bluesky_protocols.py

+# TODO: Ophyd signature is incompatible with bluesky protocol
+# (extra parameters, returns int instead of None, different parameter names)
+# Pyright is stricter and picks this up. Disabled for now
+@pytest.mark.skip()


Suggested change

# TODO: Ophyd signature is incompatible with bluesky protocol

# (extra parameters, returns int instead of None, different parameter names)

# Pyright is stricter and picks this up. Disabled for now

@pytest.mark.skip()

# TODO: ...

@pytest.mark.skip(reason="Ophyd signature is incompatible with bluesky protocol")

evvaaaa · 2025-05-20T13:53:00Z

ophyd/utils/types.py

+from typing import List, Literal, Tuple, TypedDict
+
+
+class Hints(TypedDict, total=False):


Can we not use bluesky as a dependency and use the protocols like ophyd-async does?

@danielballan thoughts?

setup.cfg

NoxHarmonium · 2025-05-22T23:13:29Z

I'll wait to hear the verdict on whether we should have bluesky as a runtime dependency (like ophyd-async) before I make any other changes.

NoxHarmonium added 4 commits May 14, 2025 15:42

Remove type ignores preventing mypy from following imports

e305db7

Tests now fail when Ophyd objects don't conform to protocols

162d6f0

- Also add pyright as a type checker to flush out any differences between pyright/mypy

Update types to conform to Bluesky interfaces

13a5452

Fix minor typo

64d5122

NoxHarmonium commented May 14, 2025

View reviewed changes

Remove duplicate check

6e3f85c

NoxHarmonium commented May 14, 2025

View reviewed changes

evvaaaa requested changes May 20, 2025

View reviewed changes

		DEFAULT_CONNECTION_TIMEOUT = object()


		class OrderedDictType(Dict[A, B]):

		from typing import List, Literal, Tuple, TypedDict


		class Hints(TypedDict, total=False):

		run(cmd + "foo: Flyable = sim.TrivialFlyer()")


		def test_hinted():



		def test_subscribable():
		# TODO: Ophyd signature is incompatible with bluesky protocol

		run(cmd + "foo: Subscribable = hw.flyer1")


		if __name__ == "__main__":

Correct return types to match current bluesky protocols #1248

Are you sure you want to change the base?

Correct return types to match current bluesky protocols #1248

Uh oh!

Conversation

NoxHarmonium commented May 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evvaaaa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NoxHarmonium May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

NoxHarmonium commented May 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

NoxHarmonium commented May 14, 2025 •

edited

Loading

NoxHarmonium May 22, 2025 •

edited

Loading

NoxHarmonium commented May 22, 2025 •

edited

Loading