Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions ophyd/__init__.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
# type: ignore
Copy link
Author

Choose a reason for hiding this comment

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

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?

Copy link

Choose a reason for hiding this comment

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

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 🤔


import logging
import os
import types
Expand Down
28 changes: 17 additions & 11 deletions ophyd/device.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
# type: ignore

from __future__ import annotations

import collections
Expand Down Expand Up @@ -31,6 +29,8 @@
Union,
)

from ophyd.utils.types import Hints

from .ophydobj import Kind, OphydObject
from .signal import Signal
from .status import DeviceStatus, StatusBase
Expand Down Expand Up @@ -86,10 +86,6 @@
DEFAULT_CONNECTION_TIMEOUT = object()


class OrderedDictType(Dict[A, B]):
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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

...


logger = logging.getLogger(__name__)


Expand Down Expand Up @@ -371,6 +367,11 @@ def sub_value(self, func):
"Value subscription decorator"
return self.subscriptions("value")(func)

# Placeholder required for components to conform to the Bluesky Moveable protocol
# The actual implementation will be provided by the cls passed into the Component
def set(self, *args, **kwargs) -> Any:
return super().set(*args, **kwargs)


class FormattedComponent(Component[K]):
"""A Component which takes a dynamic format string
Expand Down Expand Up @@ -562,7 +563,7 @@ def trigger(self) -> StatusBase:
"""
pass

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

Choose a reason for hiding this comment

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

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

Copy link
Author

@NoxHarmonium NoxHarmonium May 22, 2025

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

"""Read data from the device.

This method is expected to be as instantaneous as possible,
Expand All @@ -589,7 +590,7 @@ def read(self) -> OrderedDictType[str, Dict[str, Any]]:
"""
return OrderedDict()

def describe(self) -> OrderedDictType[str, Dict[str, Any]]:
def describe(self) -> OrderedDict[str, Any]:
"""Provide schema and meta-data for :meth:`~BlueskyInterface.read`.

This keys in the `OrderedDict` this method returns must match the
Expand Down Expand Up @@ -1456,7 +1457,7 @@ def read(self):
res.update(component.read())
return res

def read_configuration(self) -> OrderedDictType[str, Dict[str, Any]]:
def read_configuration(self) -> OrderedDict[str, Any]:
"""Dictionary mapping names to value dicts with keys: value, timestamp

To control which fields are included, change the Component kinds on the
Expand All @@ -1475,7 +1476,7 @@ def describe(self):
res.update(component.describe())
return res

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

Choose a reason for hiding this comment

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

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

"""Provide schema & meta-data for :meth:`~BlueskyInterface.read_configuration`

This keys in the `OrderedDict` this method returns must match the
Expand All @@ -1496,7 +1497,7 @@ def describe_configuration(self) -> OrderedDictType[str, Dict[str, Any]]:
return res

@property
def hints(self):
def hints(self) -> Hints:
fields = []
for _, component in self._get_components_of_kind(Kind.normal):
c_hints = component.hints
Expand Down Expand Up @@ -1667,6 +1668,11 @@ def _repr_info(self):
yield ("read_attrs", self.read_attrs)
yield ("configuration_attrs", self.configuration_attrs)

# 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)
Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.


class OphydAttrList(MutableSequence):
"""list proxy to migrate away from Device.read_attrs and Device.config_attrs"""

Expand Down
4 changes: 3 additions & 1 deletion ophyd/positioner.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from collections import OrderedDict
from typing import Any, Callable

from ophyd.utils.types import Hints

from .ophydobj import Kind, OphydObject
from .status import MoveStatus, StatusBase
from .status import wait as status_wait
Expand Down Expand Up @@ -255,7 +257,7 @@ def _repr_info(self):
yield ("timeout", self._timeout)

@property
def hints(self):
def hints(self) -> Hints:
if (~Kind.normal & Kind.hinted) & self.kind:
return {"fields": [self.name]}
else:
Expand Down
9 changes: 6 additions & 3 deletions ophyd/signal.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@
import time
import warnings
import weakref
from typing import Any, OrderedDict

import numpy as np

from ophyd.utils.types import Hints

from . import get_cl
from .ophydobj import Kind, OphydObject
from .status import Status
Expand Down Expand Up @@ -516,7 +519,7 @@ def value(self, value):
self.put(value)

@raise_if_disconnected
def read(self):
def read(self) -> OrderedDict[str, Any]:
Copy link

Choose a reason for hiding this comment

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

As before, we should use the bluesky protocols and dict here.

"""Put the status of the signal into a simple dictionary format
for data acquisition

Expand Down Expand Up @@ -547,7 +550,7 @@ def _infer_value_kind(self, inference_func):
else:
return inferred_kind

def describe(self):
def describe(self) -> OrderedDict[str, Any]:
"""Provide schema and meta-data for :meth:`~BlueskyInterface.read`

This keys in the `OrderedDict` this method returns must match the
Expand Down Expand Up @@ -625,7 +628,7 @@ def high_limit(self):
return self.limits[1]

@property
def hints(self):
def hints(self) -> Hints:
"Field hints for plotting"
if (~Kind.normal & Kind.hinted) & self.kind:
return {"fields": [self.name]}
Expand Down
19 changes: 13 additions & 6 deletions ophyd/sim.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,12 @@
from functools import partial
from tempfile import mkdtemp
from types import SimpleNamespace
from typing import Any

import numpy as np

from ophyd.utils.types import Hints

from .areadetector.base import EpicsSignalWithRBV
from .areadetector.paths import EpicsPathSignal
from .device import Component as Cpt
Expand Down Expand Up @@ -524,15 +527,15 @@ def position(self):

class SynAxisEmptyHints(SynAxis):
@property
def hints(self):
def hints(self) -> Hints:
return {}


class SynAxisNoHints(SynAxis):
readback = Cpt(_ReadbackSignal, value=0.0, kind="omitted")

@property
def hints(self):
def hints(self) -> Hints:
Copy link

Choose a reason for hiding this comment

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

If this raises an AttributeError then this should be type hinted None, or given no return type.

Copy link
Author

Choose a reason for hiding this comment

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

I guess we could also use NoReturn. If the type checker is happy with that I'll make the change.

raise AttributeError


Expand Down Expand Up @@ -761,7 +764,7 @@ class TrivialFlyer:
name = "trivial_flyer"
parent = None

def kickoff(self):
def kickoff(self) -> Any:
Copy link

Choose a reason for hiding this comment

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

Suggested change
def kickoff(self) -> Any:
def kickoff(self) -> NullStatus:

?

Copy link
Author

Choose a reason for hiding this comment

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

I thought I had to change that to Any because the type checker didn't think that Ophyd's version of BaseStatus conformed to Bluesky's Status protocol but I'll double check

return NullStatus()

def describe_collect(self):
Expand All @@ -773,7 +776,7 @@ def read_configuration(self):
def describe_configuration(self):
return OrderedDict()

def complete(self):
def complete(self) -> Any:
return NullStatus()

def collect(self):
Expand Down Expand Up @@ -902,9 +905,13 @@ def _scan(self):
with self._lock:
self._data.append(event)

def stop(self, *, success=False):
def stop(self, success=False):
pass

# Placeholder required for components to conform to the Bluesky Moveable protocol
def set(self, *args, **kwargs) -> Any:
return super().set(*args, **kwargs)

Comment on lines +911 to +914
Copy link

Choose a reason for hiding this comment

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

Perhaps this should just pass as well? This is a sim so wouldn't have a super() anyway right?

Copy link

Choose a reason for hiding this comment

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

Correction, it should still return a NullStatus.


class SynSignalWithRegistry(SynSignal):
"""
Expand Down Expand Up @@ -1677,5 +1684,5 @@ def hw(save_path=None):


# Dump instances of the example hardware generated by hw() into the global
# namespcae for convenience and back-compat.
# namespace for convenience and back-compat.
globals().update(hw().__dict__)
3 changes: 2 additions & 1 deletion ophyd/status.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from collections import deque
from functools import partial
from logging import LoggerAdapter
from typing import Any
from warnings import warn

import numpy as np
Expand Down Expand Up @@ -416,7 +417,7 @@ def _finished(self, success=True, **kwargs):
)
self.set_exception(exc)

def exception(self, timeout=None):
def exception(self, timeout=None) -> Any:
Copy link

Choose a reason for hiding this comment

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

Suggested change
def exception(self, timeout=None) -> Any:
def exception(self, timeout=None) -> None | Exception:

"""
Return the exception raised by the action.

Expand Down
Loading