Skip to content

feat: add depth parameter to devices endpoints #912

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.

Sign up for GitHub

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

feat: add depth parameter to devices endpoints #912

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
22 changes: 21 additions & 1 deletion docs/reference/openapi.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,9 @@ components:
additionalProperties: false
description: Representation of a device
properties:
address:
title: Address
type: string
name:
description: Name of the device
title: Name
Expand All @@ -17,6 +20,7 @@ components:
required:
- name
- protocols
- address
title: DeviceModel
type: object
DeviceResponse:
Expand Down Expand Up @@ -340,7 +344,7 @@ components:
type: object
info:
title: BlueAPI Control
version: 0.0.10
version: 0.0.11
openapi: 3.1.0
paths:
/config/oidc:
Expand All @@ -361,13 +365,29 @@ paths:
get:
description: Retrieve information about all available devices.
operationId: get_devices_devices_get
parameters:
- description: Maximum depth of children to return, -1 for all
in: query
name: max_depth
required: false
schema:
default: 0
minimum: 0
title: Max Depth
type: integer
responses:
'200':
content:
application/json:
schema:
$ref: '#/components/schemas/DeviceResponse'
description: Successful Response
'422':
content:
application/json:
schema:
$ref: '#/components/schemas/HTTPValidationError'
description: Validation Error
summary: Get Devices
/devices/{name}:
get:
Expand Down
12 changes: 10 additions & 2 deletions src/blueapi/cli/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,11 +164,19 @@ def get_plans(obj: dict) -> None:

@controller.command(name="devices")
@check_connection
@click.option(
"-d",
"--max_depth",
type=click.IntRange(-1),
required=False,
help="Maximum depth of children to return: -1 for all",
default=0,
)
@click.pass_obj
def get_devices(obj: dict) -> None:
def get_devices(obj: dict, max_depth: int) -> None:
"""Get a list of devices available for the worker to use"""
client: BlueapiClient = obj["client"]
obj["fmt"].display(client.get_devices())
obj["fmt"].display(client.get_devices(max_depth))


@controller.command(name="listen")
Expand Down
11 changes: 9 additions & 2 deletions src/blueapi/cli/format.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

from blueapi.core.bluesky_types import DataEvent
from blueapi.service.model import (
DeviceModel,
DeviceResponse,
PlanResponse,
PythonEnvironmentResponse,
Expand Down Expand Up @@ -62,7 +63,7 @@ def display_full(obj: Any, stream: Stream):
print(indent(json.dumps(schema, indent=2), " "))
case DeviceResponse(devices=devices):
for dev in devices:
print(dev.name)
print(_format_name(dev))
for proto in dev.protocols:
print(f" {proto}")
case DataEvent(name=name, doc=doc):
Expand Down Expand Up @@ -124,7 +125,7 @@ def display_compact(obj: Any, stream: Stream):
print(f" {arg}={_describe_type(spec, req)}")
case DeviceResponse(devices=devices):
for dev in devices:
print(dev.name)
print(_format_name(dev))
print(
indent(
textwrap.fill(
Expand Down Expand Up @@ -164,6 +165,12 @@ def display_compact(obj: Any, stream: Stream):
FALLBACK(other, stream=stream)


def _format_name(device: DeviceModel) -> str:
if not device.address or device.address == device.name:
return device.name
return f"{device.name} @ {device.address}"


def _describe_type(spec: dict[Any, Any], required: bool = False):
disp = ""
match spec.get("type"):
Expand Down
6 changes: 3 additions & 3 deletions src/blueapi/client/client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,16 @@ def get_plan(self, name: str) -> PlanModel:
"""
return self._rest.get_plan(name)

@start_as_current_span(TRACER)
def get_devices(self) -> DeviceResponse:
@start_as_current_span(TRACER, "max_depth")
def get_devices(self, max_depth: int) -> DeviceResponse:
"""
List devices available

Returns:
DeviceResponse: Devices that can be used in plans
"""

return self._rest.get_devices()
return self._rest.get_devices(max_depth)

@start_as_current_span(TRACER, "name")
def get_device(self, name: str) -> DeviceModel:
Expand Down
6 changes: 4 additions & 2 deletions src/blueapi/client/rest.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,10 @@ def get_plans(self) -> PlanResponse:
def get_plan(self, name: str) -> PlanModel:
return self._request_and_deserialize(f"/plans/{name}", PlanModel)

def get_devices(self) -> DeviceResponse:
return self._request_and_deserialize("/devices", DeviceResponse)
def get_devices(self, max_depth: int) -> DeviceResponse:
return self._request_and_deserialize(
"/devices", DeviceResponse, params={"max_depth": max_depth}
)

def get_device(self, name: str) -> DeviceModel:
return self._request_and_deserialize(f"/devices/{name}", DeviceModel)
Expand Down
4 changes: 4 additions & 0 deletions src/blueapi/core/device_lookup.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
from typing import Any

from ophyd_async.core import DeviceVector

from .bluesky_types import Device, is_bluesky_compatible_device


Expand Down Expand Up @@ -28,6 +30,8 @@ def find_component(obj: Any, addr: list[str]) -> Device | None:
# Otherwise, we error.
if isinstance(obj, dict):
component = obj.get(head)
elif isinstance(obj, DeviceVector):
component = obj.get(int(head))
elif is_bluesky_compatible_device(obj):
component = getattr(obj, head, None)
else:
Expand Down
9 changes: 7 additions & 2 deletions src/blueapi/service/interface.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,14 @@ def get_plan(name: str) -> PlanModel:
return PlanModel.from_plan(context().plans[name])


def get_devices() -> list[DeviceModel]:
def get_devices(max_depth: int) -> list[DeviceModel]:
"""Get all available devices in the BlueskyContext"""
return [DeviceModel.from_device(device) for device in context().devices.values()]
return [
model
for device in context().devices.values()
for model in DeviceModel.from_device_tree(device, max_depth)
if model.protocols
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prevent any child devices that are not deserializable as have no protocols and so are not registered?

]


def get_device(name: str) -> DeviceModel:
Expand Down
14 changes: 12 additions & 2 deletions src/blueapi/service/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
Depends,
FastAPI,
HTTPException,
Query,
Request,
Response,
status,
Expand Down Expand Up @@ -54,7 +55,7 @@
from .runner import WorkerDispatcher

#: API version to publish in OpenAPI schema
REST_API_VERSION = "0.0.10"
REST_API_VERSION = "0.0.11"

RUNNER: WorkerDispatcher | None = None

Expand Down Expand Up @@ -231,9 +232,18 @@ def get_plan_by_name(
@start_as_current_span(TRACER)
def get_devices(
Copy link
Contributor

Choose a reason for hiding this comment

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

Must: Add another attribute to DeviceModel showing where the device is in the tree, so you get back something like

{
    "name": "x",
    "address": "stage.x"
    "protocols": []
}

The alternative is to return an actual tree structure and let the client work out for itself where the device is, but we need to provide that information somehow. I think a list of unique devices with tree address as an attribute is my preference.

If you do it that way then no, I don't think get_device should also return sub-devices. It just provides exactly that thing at the given address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need to do that? For ophyd-async devices the name is parent_child and can be used to reference the child device when passing it in as params.

Copy link
Contributor

Choose a reason for hiding this comment

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

Two reasons:

  1. The client can't actually reconstruct the tree from that information because of the differing python variable names from ophyd device names. For example your tree could be
stage (name="sample")
    - x (name="sample_x")

Blueapi will find x via a request for stage.x, not sample.x, so if I just have x in isolation I have no way to work that out.

  1. Even if that wasn't the case though I don't like the idea of an API that technically does allow you to reconstruct the tree, but only by doing some string manipulation, it seems messy and not easy to protect with versioning. We should tell the user about the structure and tell them in a structured way.

See also a stackexchange thread that I read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why does the client need to reconstruct the tree? I need to know what devices can be used for what plans. If I need to know that these two motors are related, my plan should accept and I should be passing their mutual parent.

I'm probably not seeing the big picture here but if I have table: Table, table_x: Motor and table_y: Motor, I can write a generic plan(x: Motor, y: Motor) and a specific related_plan(table: Table): yield from plan(table.x, table.y)

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider querying the server for its devices and then running a plan with said devices:

> blueapi controller devices
table
    ...
table_x
    ...
table_y
   ...
> blueapi run count '{"detectors": ["table_x"]}'
   Uh no, "table_x" not found!

The correct command is

-blueapi run count '{"detectors": ["table_x"]}'
+blueapi run count '{"detectors": ["table.x"]}'

But you have no way of knowing that from the client output unless you just happen to know to convention or (even worse) code it into your client. The name mismatch is also still an issue here. Finally, is the device tree information that we want to hide?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we preserve and parse ourselves the dot access of child devices, instead of registering the children of the device and allowing references to them by name? It means the call to blueapi uses different arguments than propagate downstream in documents: another name mismatch.

It's not that I want to hide the device tree, but it's outside of the scope of being able to run plans on child 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're saying don't accept table.x but do accept table_x? I guess the main problem is that when I'm running locally in my IPython terminal I type

[1]: table.children()
["x", "y"]
[2]: RE(bp.count(detectors=[table.x]))

Knowing about/inspecting the tree is inherently part of that user experience so it is a bigger leap to completely hide it/expect the user to enter something different and think of the devices in a different way.

Copy link
Contributor Author

@DiamondJoseph DiamondJoseph Apr 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

$ blueapi controller devices
table:
  protocols:
table_x:
  protocols:
    Movable:
    - float
table_y:
  protocols:
    Movable:
    - float
$ blueapi controller run count '{"detectors": ["table_x"]}'

Is it unreasonable to say "I think the user experience has already changed so much that swapping out . for _ doesn't really matter?" Or commit to supporting both. Either way, blueapi devices with some depth will be returning the Device.name, which blueapi considers special and unique, which uses _

Copy link
Contributor

Choose a reason for hiding this comment

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

No idea, I would suggest we ask some users

runner: Annotated[WorkerDispatcher, Depends(_runner)],
max_depth: Annotated[
int,
Query(
description="Maximum depth of children to return, -1 for all",
ge=0,
# https://github.com/fastapi/fastapi/discussions/13473
json_schema_extra={"description": None},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

fastapi/fastapi#13473

I'll make an issue for tracking this if this is the way we want to go

),
] = 0,
) -> DeviceResponse:
"""Retrieve information about all available devices."""
devices = runner.run(interface.get_devices)
devices = runner.run(interface.get_devices, max_depth)
return DeviceResponse(devices=devices)


Expand Down
59 changes: 57 additions & 2 deletions src/blueapi/service/model.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from typing import Annotated, Any

from bluesky.protocols import HasName
from ophyd import Device as SyncDevice
from ophyd_async.core import Device as AsyncDevice
from pydantic import Field
from pydantic.json_schema import SkipJsonSchema

Expand Down Expand Up @@ -34,16 +36,69 @@ class DeviceModel(BlueapiBaseModel):
protocols: list[ProtocolInfo] = Field(
description="Protocols that a device conforms to, indicating its capabilities"
)
address: str

@classmethod
def from_device(cls, device: Device) -> "DeviceModel":
name = device.name if isinstance(device, HasName) else _UNKNOWN_NAME
return cls(name=name, protocols=list(_protocol_info(device)))
return cls(name=name, protocols=list(_protocol_info(device)), address=name)

@classmethod
def from_device_tree(cls, root: Device, max_depth: int) -> list["DeviceModel"]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Should: There is a function in device_lookup.py that also traverses the device tree, could extract the walk logic into a generator and use it in both of these.

if isinstance(root, AsyncDevice):
return [
DeviceModel(
name=device.name,
protocols=list(_protocol_info(device)),
address=address,
)
for address, device in _from_async_device(
root, max_depth=max_depth
).items()
]
if isinstance(root, SyncDevice):
return [
DeviceModel(
name=device.name,
protocols=list(_protocol_info(device)),
address=address,
)
for address, device in _from_sync_device(
root, max_depth=max_depth
).items()
]
return [DeviceModel.from_device(root)]


def _from_async_device(root: AsyncDevice, max_depth: int) -> dict[str, AsyncDevice]:
depth = 0
devices: dict[str, AsyncDevice] = {root.name: root}
branches: dict[str, AsyncDevice] = {root.name: root}
while branches and (max_depth == -1 or depth < max_depth):
leaves: dict[str, AsyncDevice] = {}
for addr, parent in branches.items():
for suffix, child in parent.children():
leaves[f"{addr}.{suffix}"] = child
devices.update(leaves)
branches = leaves
depth += 1
return devices


def _from_sync_device(root: SyncDevice, max_depth: int) -> dict[str, SyncDevice]:
return {
root.name: root,
**{
k.dotted_name: k.item
for k in root.walk_signals()
if max_depth == -1 or len(k.ancestors) <= max_depth
},
}


def _protocol_info(device: Device) -> Iterable[ProtocolInfo]:
for protocol in BLUESKY_PROTOCOLS:
if isinstance(device, protocol):
if isinstance(device, protocol) and protocol is not AsyncDevice:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Prevent ProtocolInfo having "Device"

yield ProtocolInfo(
name=protocol.__name__,
types=[arg.__name__ for arg in generic_bounds(device, protocol)],
Expand Down
2 changes: 1 addition & 1 deletion tests/system_tests/test_blueapi_system.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ def test_get_non_existent_plan(client: BlueapiClient):


def test_get_devices(client: BlueapiClient, expected_devices: DeviceResponse):
retrieved_devices = client.get_devices()
retrieved_devices = client.get_devices(max_depth=0)
retrieved_devices.devices.sort(key=lambda x: x.name)
expected_devices.devices.sort(key=lambda x: x.name)

Expand Down
34 changes: 28 additions & 6 deletions tests/unit_tests/client/test_client.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,11 +35,26 @@
PLAN = PlanModel(name="foo")
DEVICES = DeviceResponse(
devices=[
DeviceModel(name="foo", protocols=[]),
DeviceModel(name="bar", protocols=[]),
DeviceModel(name="foo", protocols=[], address="foo"),
DeviceModel(name="bar", protocols=[], address="bar"),
]
)
DEVICE = DeviceModel(name="foo", protocols=[])
DEVICES_AND_CHILDREN = DeviceResponse(
devices=[
DeviceModel(name="foo", protocols=[], address="foo"),
DeviceModel(name="bar", protocols=[], address="bar"),
DeviceModel(name="foo-bar", protocols=[], address="foo.bar"),
]
)
DEVICES_AND_ALL_DESCENDENTS = DeviceResponse(
devices=[
DeviceModel(name="foo", protocols=[], address="foo"),
DeviceModel(name="bar", protocols=[], address="bar"),
DeviceModel(name="foo-bar", protocols=[], address="foo.bar"),
DeviceModel(name="foo-bar-baz", protocols=[], address="foo.bar.baz"),
]
)
DEVICE = DeviceModel(name="foo", protocols=[], address="foo")
TASK = TrackableTask(task_id="foo", task=Task(name="bar", params={}))
TASKS = TasksListResponse(tasks=[TASK])
ACTIVE_TASK = WorkerTask(task_id="bar")
Expand Down Expand Up @@ -67,11 +82,18 @@

@pytest.fixture
def mock_rest() -> BlueapiRestClient:
def get_devices(max_depth: int) -> DeviceResponse:
if max_depth == -1 or max_depth > 1:
return DEVICES_AND_ALL_DESCENDENTS
if max_depth == 1:
return DEVICES_AND_CHILDREN
return DEVICES

mock = Mock(spec=BlueapiRestClient)

mock.get_plans.return_value = PLANS
mock.get_plan.return_value = PLAN
mock.get_devices.return_value = DEVICES
mock.get_devices.side_effect = get_devices
mock.get_device.return_value = DEVICE
mock.get_state.return_value = WorkerState.IDLE
mock.get_task.return_value = TASK
Expand Down Expand Up @@ -121,7 +143,7 @@ def test_get_nonexistant_plan(


def test_get_devices(client: BlueapiClient):
assert client.get_devices() == DEVICES
assert client.get_devices(max_depth=0) == DEVICES


def test_get_device(client: BlueapiClient):
Expand Down Expand Up @@ -511,7 +533,7 @@ def test_get_plan_span_ok(exporter: JsonObjectSpanExporter, client: BlueapiClien

def test_get_devices_span_ok(exporter: JsonObjectSpanExporter, client: BlueapiClient):
with asserting_span_exporter(exporter, "get_devices"):
client.get_devices()
client.get_devices(max_depth=0)


def test_get_device_span_ok(exporter: JsonObjectSpanExporter, client: BlueapiClient):
Expand Down
Loading
Loading