From 2638a1bbdd229fd9b306e1088faecd68f7ba7981 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Mon, 19 Oct 2026 19:43:21 +0100 Subject: [PATCH 01/17] Vararg support without the fallout --- docs/reference/openapi.yaml | 14 +- example-services | 2 +- src/blueapi/config.py | 2 +- src/blueapi/core/context.py | 21 ++- src/blueapi/service/interface.py | 1 + src/blueapi/service/model.py | 8 +- src/blueapi/worker/task.py | 98 ++++++++--- tests/system_tests/test_blueapi_system.py | 18 +- tests/unit_tests/service/test_interface.py | 9 +- tests/unit_tests/worker/test_task_worker.py | 186 +++++++++++++++++++- 10 files changed, 312 insertions(+), 47 deletions(-) diff --git a/docs/reference/openapi.yaml b/docs/reference/openapi.yaml index e0cd8282df..c17734bbe2 100644 --- a/docs/reference/openapi.yaml +++ b/docs/reference/openapi.yaml @@ -236,6 +236,11 @@ components: additionalProperties: false description: Task that will run a plan properties: + args: + description: Positional arguments to plan + items: {} + title: Args + type: array metadata: additionalProperties: true description: Any metadata to apply to all runs within this task @@ -247,7 +252,7 @@ components: type: string params: additionalProperties: true - description: Values for parameters to plan, if any + description: Keyword arguments to plan, if any title: Params type: object required: @@ -278,6 +283,11 @@ components: additionalProperties: false description: Request to run a task with related info properties: + args: + description: Positional arguments to plan + items: {} + title: Args + type: array instrument_session: description: Instrument session associated with this task title: Instrument Session @@ -449,7 +459,7 @@ info: name: Apache 2.0 url: https://www.apache.org/licenses/LICENSE-2.0.html title: BlueAPI Control - version: 1.5.0 + version: 1.6.0 openapi: 3.1.0 paths: /api/v1/devices: diff --git a/example-services b/example-services index 9e0faf7cbe..3c0274b244 160000 --- a/example-services +++ b/example-services @@ -1 +1 @@ -Subproject commit 9e0faf7cbea7e40ea63d857f046f61675b35b907 +Subproject commit 3c0274b244d28bc6e197f9e3610355cc7c0cc5d3 diff --git a/src/blueapi/config.py b/src/blueapi/config.py index 9c339c71f5..f65dd65f0a 100644 --- a/src/blueapi/config.py +++ b/src/blueapi/config.py @@ -335,7 +335,7 @@ class ApplicationConfig(BlueapiBaseModel): """ #: API version to publish in OpenAPI schema - REST_API_VERSION: ClassVar[str] = "1.5.0" + REST_API_VERSION: ClassVar[str] = "1.6.0" LICENSE_INFO: ClassVar[dict[str, str]] = { "name": "Apache 2.0", diff --git a/src/blueapi/core/context.py b/src/blueapi/core/context.py index 76ce74e647..f8a96b0d6a 100644 --- a/src/blueapi/core/context.py +++ b/src/blueapi/core/context.py @@ -439,7 +439,7 @@ def _type_spec_for_function( Returns: Mapping of {name: (type, default)} to be used by pydantic for deserialising - function arguments + function arguments """ args = signature(func).parameters types = get_type_hints(func) @@ -451,7 +451,16 @@ def _type_spec_for_function( f"Type annotation is required for '{name}' in '{func.__name__}'" ) - no_default = para.default is Parameter.empty + match para.kind: + case Parameter.VAR_POSITIONAL: + arg_type = tuple[arg_type, ...] + case Parameter.VAR_KEYWORD: + arg_type = dict[str, arg_type] + + no_default = para.default is Parameter.empty and para.kind not in ( + Parameter.VAR_POSITIONAL, + Parameter.VAR_KEYWORD, + ) if ( isclass(arg_type) and (issubclass(arg_type, BaseModel) or is_dataclass(arg_type)) @@ -462,9 +471,15 @@ def _type_spec_for_function( info = FieldInfo(default_factory=default_factory) else: _type = self._convert_type(arg_type, no_default) + match para.default: case Parameter.empty: - info = FieldInfo(default_factory=None) + if para.kind is Parameter.VAR_POSITIONAL: + info = FieldInfo(default_factory=tuple) + elif para.kind is Parameter.VAR_KEYWORD: + info = FieldInfo(default_factory=dict) + else: + info = FieldInfo(default_factory=None) case None: info = FieldInfo(default_factory=lambda: None) case _: diff --git a/src/blueapi/service/interface.py b/src/blueapi/service/interface.py index a767c4043c..7df4745570 100644 --- a/src/blueapi/service/interface.py +++ b/src/blueapi/service/interface.py @@ -220,6 +220,7 @@ def submit_task( ] task = Task( name=task_request.name, + args=task_request.args, params=task_request.params, metadata=metadata, ) diff --git a/src/blueapi/service/model.py b/src/blueapi/service/model.py index dbaa1d9651..13a98b188d 100644 --- a/src/blueapi/service/model.py +++ b/src/blueapi/service/model.py @@ -64,8 +64,14 @@ class TaskRequest(BlueapiBaseModel): """ name: str = Field(description="Name of plan to run") + args: list[Any] = Field( + description="Positional arguments to plan", + default_factory=list, + exclude_if=lambda args: not args, + ) params: Mapping[str, Any] = Field( - description="Values for parameters to plan, if any", default_factory=dict + description="Values for parameters to plan, if any", + default_factory=dict, ) instrument_session: str = Field( description="Instrument session associated with this task", diff --git a/src/blueapi/worker/task.py b/src/blueapi/worker/task.py index 6dbbaa0202..a0f2f09ce5 100644 --- a/src/blueapi/worker/task.py +++ b/src/blueapi/worker/task.py @@ -1,6 +1,7 @@ import logging from collections.abc import Mapping from typing import Any, Self, TypedDict +from inspect import Parameter, signature from pydantic import BaseModel, Field, TypeAdapter, ValidationError @@ -16,57 +17,98 @@ class Task(BlueapiBaseModel): """ name: str = Field(description="Name of plan to run") + args: list[Any] = Field( + description="Positional arguments to plan", + default_factory=list, + exclude_if=lambda args: not args, + ) params: Mapping[str, Any] = Field( - description="Values for parameters to plan, if any", default_factory=dict + description="Keyword arguments to plan, if any", + default_factory=dict, ) metadata: dict[str, Any] = Field( description="Any metadata to apply to all runs within this task", default_factory=dict, ) - def prepare_params(self, ctx: BlueskyContext) -> Mapping[str, Any]: + def prepare_params( + self, ctx: BlueskyContext + ) -> tuple[list[Any], Mapping[str, Any]]: + """ + Checks the configured plan parameters against context + + Args: + ctx: Context holding plans and devices + + Returns: + tuple[list[Any], Mapping[str, Any]]: The prepared *args and **kwargs + parameters for the plan. + """ try: - model = _lookup_params(ctx, self) - # Re-create dict manually to avoid nesting in model_dump output - return {field: getattr(model, field) for field in model.__pydantic_fields__} + return self._inner_prepare_params(ctx) except ValidationError as ve: raise TaskParameterError.from_validation_error(ve) from None + except TypeError as te: + raise TaskParameterError(str(te), []) from None + + def _inner_prepare_params( + self, ctx: BlueskyContext + ) -> tuple[list[Any], Mapping[str, Any]]: + plan = ctx.plans[self.name] + func = ctx.plan_functions[self.name] + + sig = signature(func) + bound = sig.bind_partial(*self.args, **self.params) + + # Only validate explicitly supplied arguments. This allows Pydantic's + # default/default_factory to provide injected defaults. + adapter = TypeAdapter(plan.model) + validated = adapter.validate_python(bound.arguments) + + args: list[Any] = [] + kwargs: dict[str, Any] = {} + + for name, parameter in sig.parameters.items(): + supplied = name in bound.arguments + + if supplied: + value = getattr(validated, name) + + match parameter.kind: + case Parameter.POSITIONAL_ONLY: + args.append(value) + case Parameter.VAR_POSITIONAL: + args.extend(value) + case Parameter.POSITIONAL_OR_KEYWORD: + if name in self.params: + kwargs[name] = value + else: + args.append(value) + case Parameter.KEYWORD_ONLY: + kwargs[name] = value + case Parameter.VAR_KEYWORD: + kwargs.update(value) + else: + # Let the generated Pydantic model provide the default. + kwargs[name] = getattr(validated, name) + + return args, kwargs def do_task(self, ctx: BlueskyContext) -> None: LOGGER.info( f"Asked to run plan {self.name} with {self.params} and " f"metadata {self.metadata} for all runs" ) - - func = ctx.plan_functions[self.name] - prepared_params = self.prepare_params(ctx) + plan = ctx.plan_functions[self.name] + prepared_args, prepared_kwargs = self.prepare_params(ctx) ctx.run_engine.md.update(self.metadata) - result = ctx.run_engine(func(**prepared_params)) + result = ctx.run_engine(plan(*prepared_args, **prepared_kwargs)) if isinstance(result, tuple): # pragma: no cover # this is never true if the run_engine is configured correctly return None return result.plan_result -def _lookup_params(ctx: BlueskyContext, task: Task) -> BaseModel: - """ - Checks plan parameters against context - - Args: - ctx: Context holding plans and devices - plan: Plan object including schema - params: Parameter values to be validated against schema - - Returns: - Mapping[str, Any]: _description_ - """ - - plan = ctx.plans[task.name] - model = plan.model - adapter = TypeAdapter(model) - return adapter.validate_python(task.params) - - class InvalidTaskParameter(TypedDict): loc: tuple[str | int, ...] msg: str | None diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 5ee1639971..9feda470c6 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -542,15 +542,26 @@ def test_delete_current_environment(client: BlueapiClient): @pytest.mark.parametrize( "task,user", [ - ( + pytest.param( TaskRequest( name="count", - params={"detectors": ["det"], "num": 3}, + args=[["det"], 3], instrument_session=VALID_INSTRUMENT_SESSION[User.alice], ), User.alice, + id="all_args", ), - ( + pytest.param( + TaskRequest( + name="count", + args=[["det"]], + params={"num": 3}, + instrument_session=VALID_INSTRUMENT_SESSION[User.alice], + ), + User.alice, + id="mixed_args_params", + ), + pytest.param( TaskRequest( name="spec_scan", params={ @@ -577,6 +588,7 @@ def test_delete_current_environment(client: BlueapiClient): instrument_session=VALID_INSTRUMENT_SESSION[User.bob], ), User.bob, + id="all_params", ), ], ) diff --git a/tests/unit_tests/service/test_interface.py b/tests/unit_tests/service/test_interface.py index ef44a558d0..db4e687600 100644 --- a/tests/unit_tests/service/test_interface.py +++ b/tests/unit_tests/service/test_interface.py @@ -195,12 +195,15 @@ def test_get_device(context_mock: MagicMock): assert interface.get_device("non_existing_device") +@pytest.mark.parametrize("args,kwargs", (([], {"repeats": 42}), ([42], {}))) @patch("blueapi.service.interface.context") -def test_submit_task(context_mock: MagicMock): +def test_submit_task(context_mock: MagicMock, args, kwargs): context = BlueskyContext() - context.register_plan(my_plan) + context.register_plan(my_second_plan) task = TaskRequest( - name="my_plan", + name="my_second_plan", + args=args, + params=kwargs, instrument_session=FAKE_INSTRUMENT_SESSION, ) context_mock.return_value = context diff --git a/tests/unit_tests/worker/test_task_worker.py b/tests/unit_tests/worker/test_task_worker.py index f6fe50c0da..796f633b17 100644 --- a/tests/unit_tests/worker/test_task_worker.py +++ b/tests/unit_tests/worker/test_task_worker.py @@ -34,6 +34,7 @@ WorkerState, ) from blueapi.worker.event import TaskResult, TaskStatusEnum +from blueapi.worker.task import TaskParameterError _SIMPLE_TASK = Task(name="sleep", params={"time": 0.0}) _LONG_TASK = Task(name="sleep", params={"time": 1.0}) @@ -724,7 +725,7 @@ def injected_device_plan( yield from () context.register_plan(injected_device_plan) - params = Task(name="injected_device_plan").prepare_params(context) + _, params = Task(name="injected_device_plan").prepare_params(context) assert params["dev"] == fake_device @@ -840,7 +841,7 @@ def test_injected_composite_devices_are_found( context: BlueskyContext, ): context.register_plan(injected_device_plan) - params = Task(name="injected_device_plan").prepare_params(context) + _, params = Task(name="injected_device_plan").prepare_params(context) assert params["composite"].fake_device == fake_device assert params["composite"].second_fake_device == second_fake_device @@ -861,7 +862,7 @@ def test_injected_composite_with_pydantic_dataclass( second_fake_device: FakeDevice, ): context.register_plan(injected_dataclass_device_plan) - params = Task(name="injected_dataclass_device_plan").prepare_params(context) + _, params = Task(name="injected_dataclass_device_plan").prepare_params(context) assert params["composite"].fake_device == fake_device assert params["composite"].second_fake_device == second_fake_device @@ -872,7 +873,7 @@ def test_injected_composite_with_standard_dataclass( second_fake_device: FakeDevice, ): context.register_plan(injected_standard_dataclass_device_plan) - params = Task(name="injected_standard_dataclass_device_plan").prepare_params( + _, params = Task(name="injected_standard_dataclass_device_plan").prepare_params( context ) assert params["composite"].fake_device == fake_device @@ -887,7 +888,9 @@ def test_plan_module_with_composite_devices_can_be_loaded_before_device_module( context_without_devices.register_plan(injected_device_plan) context_without_devices.register_device(fake_device) context_without_devices.register_device(second_fake_device) - params = Task(name="injected_device_plan").prepare_params(context_without_devices) + _, params = Task(name="injected_device_plan").prepare_params( + context_without_devices + ) assert params["composite"].fake_device == fake_device assert params["composite"].second_fake_device == second_fake_device @@ -949,3 +952,176 @@ def test_worker_event_task_id(): def test_worker_event_no_task_id(): event = WorkerEvent(state=WorkerState.IDLE, task_status=None) assert event.task_id is None + + +def test_task_worker_passes_positional_args( + context: BlueskyContext, +) -> None: + def positional_plan(value: int) -> MsgGenerator: + yield from () + + context.register_plan(positional_plan) + + task = Task(name="positional_plan", args=[42]) + args, kwargs = task.prepare_params(context) + + assert args == [42] + assert kwargs == {} + + +def test_task_worker_passes_multiple_positional_args( + context: BlueskyContext, +) -> None: + def positional_plan(first: int, second: int) -> MsgGenerator: + yield from () + + context.register_plan(positional_plan) + task = Task(name="positional_plan", args=[1, 2]) + args, kwargs = task.prepare_params(context) + + assert args == [1, 2] + assert kwargs == {} + + +def test_task_worker_passes_positional_and_keyword_args( + context: BlueskyContext, +) -> None: + def mixed_plan( + first: int, + second: int, + *, + third: int, + ) -> MsgGenerator: + yield from () + + context.register_plan(mixed_plan) + task = Task( + name="mixed_plan", + args=[1, 2], + params={"third": 3}, + ) + args, kwargs = task.prepare_params(context) + assert args == [1, 2] + assert kwargs == {"third": 3} + + +def test_task_worker_resolves_positional_device( + context: BlueskyContext, + fake_device: FakeDevice, +) -> None: + def positional_device_plan(device: FakeDevice) -> MsgGenerator: + yield from () + + context.register_plan(positional_device_plan) + + task = Task(name="positional_device_plan", args=[fake_device.name]) + args, kwargs = task.prepare_params(context) + assert args == [fake_device] + assert kwargs == {} + + +def test_positional_only_plan(fake_device: FakeDevice) -> None: + def positional_only_plan(val: int, /, device: FakeDevice) -> MsgGenerator: + yield from () + + context = BlueskyContext() + context.register_device(fake_device) + context.register_plan(positional_only_plan) + + task = Task( + name="positional_only_plan", + args=[1], + params={"device": fake_device.name}, + ) + + args, kwargs = task.prepare_params(context) + assert args == [1] + assert kwargs == {"device": fake_device} + + +def test_var_positional_plan(fake_device: FakeDevice) -> None: + def positional_device_plan(val: int, *device: FakeDevice) -> MsgGenerator: + yield from () + + context = BlueskyContext() + context.register_device(fake_device) + context.register_plan(positional_device_plan) + + task = Task( + name="positional_device_plan", + args=[1, fake_device.name, fake_device.name], + ) + args, kwargs = task.prepare_params(context) + assert args == [1, fake_device, fake_device] + assert kwargs == {} + + +def test_positional_only_and_var_positional_plan( + fake_device: FakeDevice, +) -> None: + def positional_device_plan(val: int, /, *device: FakeDevice) -> MsgGenerator: + yield from () + + context = BlueskyContext() + context.register_device(fake_device) + context.register_plan(positional_device_plan) + + task = Task( + name="positional_device_plan", + args=[1, fake_device.name, fake_device.name], + ) + + args, kwargs = task.prepare_params(context) + assert args == [1, fake_device, fake_device] + assert kwargs == {} + + +def test_var_keyword_plan(fake_device: FakeDevice) -> None: + def keyword_device_plan(val: int, **devices: FakeDevice) -> MsgGenerator: + yield from () + + context = BlueskyContext() + context.register_device(fake_device) + context.register_plan(keyword_device_plan) + + task = Task( + name="keyword_device_plan", + args=[1], + params={"first": fake_device.name, "second": fake_device.name}, + ) + args, kwargs = task.prepare_params(context) + assert args == [1] + assert kwargs == {"first": fake_device, "second": fake_device} + + +def test_positional_only_and_var_keyword_plan( + fake_device: FakeDevice, +) -> None: + def positional_only_plan(val: int, /, **devices: FakeDevice) -> MsgGenerator: + yield from () + + context = BlueskyContext() + context.register_device(fake_device) + context.register_plan(positional_only_plan) + + task = Task( + name="positional_only_plan", + args=[1], + params={"first": fake_device.name, "second": fake_device.name}, + ) + args, kwargs = task.prepare_params(context) + assert args == [1] + assert kwargs == {"first": fake_device, "second": fake_device} + + +def test_arg_in_args_and_kwargs(): + + def positionals(one: int, two: int) -> MsgGenerator: + yield from [] + + context = BlueskyContext() + context.register_plan(positionals) + + task = Task(name="positionals", args=[1], params={"one": 2}) + with pytest.raises(TaskParameterError, match="multiple values for argument 'one'"): + task.prepare_params(context) From d5d5d6e014ca9f80e971c11c88081d719ab698d1 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 20 Oct 2026 10:55:05 +0100 Subject: [PATCH 02/17] Add CLI support for varargs --- src/blueapi/cli/cli.py | 23 ++++++++++++++++++++++- src/blueapi/client/rest.py | 4 +++- src/blueapi/core/context.py | 5 +++-- src/blueapi/service/main.py | 15 ++++++++++----- 4 files changed, 38 insertions(+), 9 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 81dc836371..4e65bb2a3b 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -73,6 +73,23 @@ def convert( return super().convert(value, param, ctx) +class PositionalArgType(ParamType): + """CLI input parameter to accept a JSON value as an argument""" + + name = "PositionalArg" + + def convert( + self, value: Any | None, param: Parameter | None, ctx: Context | None + ) -> Any: + if isinstance(value, str): + try: + return json.loads(value) + except json.JSONDecodeError as jde: + self.fail(f"Argument is not valid JSON: {jde}") + else: + return super().convert(value, param, ctx) + + def is_str_dict(val: Any) -> TypeGuard[TaskParameters]: return isinstance(val, dict) and all(isinstance(k, str) for k in val) @@ -321,6 +338,9 @@ def on_event( @controller.command(name="run") @click.argument("name", type=str) @click.argument("parameters", type=ParametersType(), default={}, required=False) +@click.option( + "-a", "--arg", type=PositionalArgType(), multiple=True, default=[], required=False +) @click.option( "--ws/--stomp", type=bool, @@ -366,6 +386,7 @@ def run_plan( foreground: bool, ws: bool | None, instrument_session: str, + arg: tuple[Any, ...], parameters: TaskParameters, ) -> None: """Run a plan with parameters @@ -377,7 +398,7 @@ def run_plan( client = cast(BlueapiClient, obj["client"]) task = TaskRequest( - name=name, params=parameters, instrument_session=instrument_session + name=name, args=arg, params=parameters, instrument_session=instrument_session ) try: diff --git a/src/blueapi/client/rest.py b/src/blueapi/client/rest.py index 166620ac2a..f07f6b659c 100644 --- a/src/blueapi/client/rest.py +++ b/src/blueapi/client/rest.py @@ -114,10 +114,12 @@ def __str__(self) -> str: return f"Missing value for {self.field()!r}" case "extra_forbidden": return f"Unexpected field {self.field()!r}" - case _: + case _ if self.loc[2:]: return ( f"Invalid value {self.input!r} for field {self.field()}: {self.msg}" ) + case _: + return self.msg or "Unknown error" class InvalidParametersError(Exception): diff --git a/src/blueapi/core/context.py b/src/blueapi/core/context.py index f8a96b0d6a..6cf4c1d6de 100644 --- a/src/blueapi/core/context.py +++ b/src/blueapi/core/context.py @@ -386,7 +386,8 @@ class Reference(target): def __get_pydantic_core_schema__( cls, source_type: Any, handler: GetCoreSchemaHandler ) -> CoreSchema: - def valid(value): + def device_lookup(value): + print(f"Looking up {value}") val = self.find_device(value) if not val: raise ValueError(f"Device {value} cannot be found") @@ -399,7 +400,7 @@ def valid(value): return val return core_schema.no_info_after_validator_function( - valid, handler(str) + device_lookup, handler(str) ) @classmethod diff --git a/src/blueapi/service/main.py b/src/blueapi/service/main.py index 225413304b..2ba3107879 100644 --- a/src/blueapi/service/main.py +++ b/src/blueapi/service/main.py @@ -361,12 +361,17 @@ def submit_task( } for err in e.errors ] - LOGGER.info("Error submitting task: %s - %s", task_request, e) - raise HTTPException( - status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, - detail=errors, - ) from e + except Exception as e: + # Catch all for anything else that can go wrong + errors = [ + {"loc": ["body", "params"], "msg": str(e), "type": "invalid", "input": None} + ] + LOGGER.info("Error submitting task: %s - %s", task_request, e) + raise HTTPException( + status_code=status.HTTP_422_UNPROCESSABLE_CONTENT, + detail=errors, + ) @secure_router_v1.delete( From da1c9c3a686c8f25bd0dabd34cf3ed1bbb878cc8 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Tue, 20 Oct 2026 11:29:38 +0100 Subject: [PATCH 03/17] Undo unrelated reformatting --- src/blueapi/service/model.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/blueapi/service/model.py b/src/blueapi/service/model.py index 13a98b188d..1b690d5c04 100644 --- a/src/blueapi/service/model.py +++ b/src/blueapi/service/model.py @@ -70,8 +70,7 @@ class TaskRequest(BlueapiBaseModel): exclude_if=lambda args: not args, ) params: Mapping[str, Any] = Field( - description="Values for parameters to plan, if any", - default_factory=dict, + description="Values for parameters to plan, if any", default_factory=dict ) instrument_session: str = Field( description="Instrument session associated with this task", From 0029787c278277ea3add17ff23df9ad3e10c12e4 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 21 Oct 2026 20:29:14 +0100 Subject: [PATCH 04/17] Include TypeError message as inner InvalidTaskParameter error --- src/blueapi/worker/task.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/blueapi/worker/task.py b/src/blueapi/worker/task.py index a0f2f09ce5..329f21cf07 100644 --- a/src/blueapi/worker/task.py +++ b/src/blueapi/worker/task.py @@ -1,9 +1,9 @@ import logging from collections.abc import Mapping -from typing import Any, Self, TypedDict from inspect import Parameter, signature +from typing import Any, Self, TypedDict -from pydantic import BaseModel, Field, TypeAdapter, ValidationError +from pydantic import Field, TypeAdapter, ValidationError from blueapi.core import BlueskyContext from blueapi.utils import BlueapiBaseModel @@ -49,7 +49,14 @@ def prepare_params( except ValidationError as ve: raise TaskParameterError.from_validation_error(ve) from None except TypeError as te: - raise TaskParameterError(str(te), []) from None + raise TaskParameterError( + "Invalid arguments to plan", + [ + InvalidTaskParameter( + loc=(), msg=str(te), type="TypeError", input=None + ) + ], + ) from None def _inner_prepare_params( self, ctx: BlueskyContext @@ -127,7 +134,7 @@ def __init__(self, message: str, errors: list[InvalidTaskParameter]): def from_validation_error(cls, error: ValidationError) -> Self: errors = [ InvalidTaskParameter( - loc=e.get("loc", []), + loc=e.get("loc", ()), msg=e.get("msg", None), type=e.get("type", None), input=e.get("input", None), From 27655a062c4156b752984ef4800d214fe8f411be Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 21 Oct 2026 20:54:40 +0100 Subject: [PATCH 05/17] Accept all posisional args as single CLI option --- src/blueapi/cli/cli.py | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 4e65bb2a3b..7bc08dde7e 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -74,7 +74,7 @@ def convert( class PositionalArgType(ParamType): - """CLI input parameter to accept a JSON value as an argument""" + """CLI input parameter to accept a JSON value as a list of positional args""" name = "PositionalArg" @@ -83,7 +83,10 @@ def convert( ) -> Any: if isinstance(value, str): try: - return json.loads(value) + args = json.loads(value) + if isinstance(args, list): + return args + self.fail("Positional arguments should be a list of arguments") except json.JSONDecodeError as jde: self.fail(f"Argument is not valid JSON: {jde}") else: @@ -338,9 +341,7 @@ def on_event( @controller.command(name="run") @click.argument("name", type=str) @click.argument("parameters", type=ParametersType(), default={}, required=False) -@click.option( - "-a", "--arg", type=PositionalArgType(), multiple=True, default=[], required=False -) +@click.option("-a", "--args", type=PositionalArgType(), default=[], required=False) @click.option( "--ws/--stomp", type=bool, @@ -386,7 +387,7 @@ def run_plan( foreground: bool, ws: bool | None, instrument_session: str, - arg: tuple[Any, ...], + args: tuple[Any, ...], parameters: TaskParameters, ) -> None: """Run a plan with parameters @@ -398,7 +399,7 @@ def run_plan( client = cast(BlueapiClient, obj["client"]) task = TaskRequest( - name=name, args=arg, params=parameters, instrument_session=instrument_session + name=name, args=args, params=parameters, instrument_session=instrument_session ) try: From 05470a8abda13083c4938beba1c372ff4ac2b6dc Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 21 Oct 2026 21:11:28 +0100 Subject: [PATCH 06/17] Rely on server handling of args and kwards for repl --- src/blueapi/client/plan_cache.py | 44 ++------------------------------ 1 file changed, 2 insertions(+), 42 deletions(-) diff --git a/src/blueapi/client/plan_cache.py b/src/blueapi/client/plan_cache.py index 69e64a1aba..814c120bf2 100644 --- a/src/blueapi/client/plan_cache.py +++ b/src/blueapi/client/plan_cache.py @@ -76,7 +76,8 @@ def __call__(self, *args, **kwargs) -> Any: """ req = TaskRequest( name=self.name, - params=self._build_args(*args, **kwargs), + args=args, + params=kwargs, instrument_session=self._client.instrument_session, ) match self._client.run_task(req): @@ -97,47 +98,6 @@ def properties(self) -> dict[str, Any]: def required(self) -> list[str]: return self.model.parameter_schema.get("required", []) - def _build_args(self, *args, **kwargs) -> dict[str, Any]: - """Build a parameter mapping from positional and keyword arguments. - - Positional arguments are assigned to parameters according to their - order in the plan's parameter schema. Keyword arguments are then - added by name. - - Raises: - TypeError: If too many positional arguments, unexpected keyword - arguments, duplicate arguments, or required arguments are - supplied incorrectly. - """ - log.info( - "Building args for %s, using %s and %s", - "[" + ",".join(self.properties) + "]", - args, - kwargs, - ) - - if len(args) > len(self.properties): - raise TypeError(f"{self.name} got too many arguments") - if extra := {k for k in kwargs if k not in self.properties}: - raise TypeError(f"{self.name} got unexpected arguments: {extra}") - - params = {} - # Initially fill parameters using positional args assuming the order - # from the parameter_schema - for req, arg in zip(self.properties, args, strict=False): - params[req] = arg - - # Then append any values given via kwargs - for key, value in kwargs.items(): - # If we've already assumed a positional arg was this value, bail out - if key in params: - raise TypeError(f"{self.name} got multiple values for {key}") - params[key] = value - - if missing := {k for k in self.required if k not in params}: - raise TypeError(f"Missing argument(s) for {missing}") - return params - def __repr__(self) -> str: """Return a signature-like representation of the plan. From 6dccdfea82fcdb3ff3d877ae35f495a289dbc2fd Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 21 Oct 2026 21:24:33 +0100 Subject: [PATCH 07/17] Raise TypeError if plan args are not valid --- src/blueapi/client/plan_cache.py | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/blueapi/client/plan_cache.py b/src/blueapi/client/plan_cache.py index 814c120bf2..0ad2bcdfe7 100644 --- a/src/blueapi/client/plan_cache.py +++ b/src/blueapi/client/plan_cache.py @@ -1,6 +1,7 @@ import logging from typing import Any +from blueapi.client.rest import InvalidParametersError from blueapi.service.model import PlanModel, TaskRequest from blueapi.worker.event import TaskError, TaskResult, TaskStatus @@ -80,11 +81,14 @@ def __call__(self, *args, **kwargs) -> Any: params=kwargs, instrument_session=self._client.instrument_session, ) - match self._client.run_task(req): - case TaskStatus(result=TaskResult(result=res)): - return res - case TaskStatus(result=TaskError(type=typ, message=msg)): - raise PlanFailedError(typ, msg) + try: + match self._client.run_task(req): + case TaskStatus(result=TaskResult(result=res)): + return res + case TaskStatus(result=TaskError(type=typ, message=msg)): + raise PlanFailedError(typ, msg) + except InvalidParametersError as ipe: + raise TypeError(ipe.message()) from None @property def help_text(self) -> str: From 9b9848f2ac4d8a2126a52e7ad813d7b23de3706a Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Wed, 21 Oct 2026 21:41:10 +0100 Subject: [PATCH 08/17] Update tests to replace plan cache changes --- tests/unit_tests/client/test_plan_cache.py | 52 ++++++++++----------- tests/unit_tests/worker/test_task_worker.py | 4 +- 2 files changed, 28 insertions(+), 28 deletions(-) diff --git a/tests/unit_tests/client/test_plan_cache.py b/tests/unit_tests/client/test_plan_cache.py index 3da4d1c9b8..6b36246932 100644 --- a/tests/unit_tests/client/test_plan_cache.py +++ b/tests/unit_tests/client/test_plan_cache.py @@ -5,6 +5,7 @@ from blueapi.client.client import PlanCache from blueapi.client.plan_cache import Plan +from blueapi.client.rest import InvalidParametersError, ParameterError from blueapi.service.model import PlanModel, TaskRequest FULL_PLAN = PlanModel( @@ -132,46 +133,43 @@ def test_plan_empty_fallback_help_text(client): @pytest.mark.parametrize( - "args,kwargs,params", + "args,kwargs", [ - p((1,), {}, {"one": 1}, id="required_as_positional"), - p((), {"one": 7}, {"one": 7}, id="required_as_keyword"), - p((1,), {"two": 23}, {"one": 1, "two": 23}, id="all_as_mixed_args_kwargs"), - p((1, 2), {}, {"one": 1, "two": 2}, id="all_as_positional"), - p((), {"one": 21, "two": 42}, {"one": 21, "two": 42}, id="all_as_keyword"), + p((1,), {}, id="required_as_positional"), + p((), {"one": 7}, id="required_as_keyword"), + p((1,), {"two": 23}, id="all_as_mixed_args_kwargs"), + p((1, 2), {}, id="all_as_positional"), + p((), {"one": 21, "two": 42}, id="all_as_keyword"), ], ) -def test_plan_param_mapping(args, kwargs, params): +def test_plan_param_forwarding(args, kwargs): client = Mock() client.instrument_session = "cm12345-1" plan = Plan("foobar", FULL_PLAN, client) plan(*args, **kwargs) client.run_task.assert_called_once_with( - TaskRequest(name="foobar", instrument_session="cm12345-1", params=params) + TaskRequest( + name="foobar", instrument_session="cm12345-1", args=args, params=kwargs + ) ) -@pytest.mark.parametrize( - "args,kwargs,msg", - [ - p((), {}, r"Missing argument\(s\) for \{'one'\}", id="missing_required"), - p((1,), {"one": 7}, "multiple values for one", id="duplicate_required"), - p((1, 2), {"two": 23}, "multiple values for two", id="duplicate_optional"), - p((1, 2, 3), {}, "too many arguments", id="too_many_args"), - p( - (), - {"unknown_key": 42}, - r"got unexpected arguments: \{'unknown_key'\}", - id="unknown_arg", - ), - ], -) -def test_plan_invalid_param_mapping(args, kwargs, msg): +def test_plan_invalid_param_mapping(): client = Mock() client.instrument_session = "cm12345-1" + client.run_task.side_effect = InvalidParametersError( + [ + ParameterError( + msg="foo is not valid", + loc=["body", "params", "foo"], + input="bar", + type="invalid", + ) + ] + ) plan = Plan("foobar", FULL_PLAN, client) - with pytest.raises(TypeError, match=msg): - plan(*args, **kwargs) - client.run_task.assert_not_called() + with pytest.raises(TypeError, match="foo is not valid"): + plan(1, 2, foo="bar") + client.run_task.assert_called_once() diff --git a/tests/unit_tests/worker/test_task_worker.py b/tests/unit_tests/worker/test_task_worker.py index 796f633b17..8f0a1269c5 100644 --- a/tests/unit_tests/worker/test_task_worker.py +++ b/tests/unit_tests/worker/test_task_worker.py @@ -1123,5 +1123,7 @@ def positionals(one: int, two: int) -> MsgGenerator: context.register_plan(positionals) task = Task(name="positionals", args=[1], params={"one": 2}) - with pytest.raises(TaskParameterError, match="multiple values for argument 'one'"): + with pytest.raises(TaskParameterError, match="Invalid arguments to plan") as tpe: task.prepare_params(context) + + assert tpe.value.errors[0]["msg"] == "multiple values for argument 'one'" From b910617df18866d49e59720687d4854e49d729c9 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 14:28:52 +0100 Subject: [PATCH 09/17] Make task args a tuple --- src/blueapi/service/model.py | 4 +-- src/blueapi/worker/task.py | 6 ++-- tests/system_tests/test_blueapi_system.py | 4 +-- tests/unit_tests/worker/test_task_worker.py | 38 ++++++++++----------- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/blueapi/service/model.py b/src/blueapi/service/model.py index 1b690d5c04..4ca22f5e98 100644 --- a/src/blueapi/service/model.py +++ b/src/blueapi/service/model.py @@ -64,9 +64,9 @@ class TaskRequest(BlueapiBaseModel): """ name: str = Field(description="Name of plan to run") - args: list[Any] = Field( + args: tuple[Any, ...] = Field( description="Positional arguments to plan", - default_factory=list, + default_factory=tuple, exclude_if=lambda args: not args, ) params: Mapping[str, Any] = Field( diff --git a/src/blueapi/worker/task.py b/src/blueapi/worker/task.py index 329f21cf07..c32127193a 100644 --- a/src/blueapi/worker/task.py +++ b/src/blueapi/worker/task.py @@ -17,9 +17,9 @@ class Task(BlueapiBaseModel): """ name: str = Field(description="Name of plan to run") - args: list[Any] = Field( + args: tuple[Any, ...] = Field( description="Positional arguments to plan", - default_factory=list, + default_factory=tuple, exclude_if=lambda args: not args, ) params: Mapping[str, Any] = Field( @@ -134,7 +134,7 @@ def __init__(self, message: str, errors: list[InvalidTaskParameter]): def from_validation_error(cls, error: ValidationError) -> Self: errors = [ InvalidTaskParameter( - loc=e.get("loc", ()), + loc=e.get("loc"), msg=e.get("msg", None), type=e.get("type", None), input=e.get("input", None), diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 9feda470c6..6c155fe6bb 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -545,7 +545,7 @@ def test_delete_current_environment(client: BlueapiClient): pytest.param( TaskRequest( name="count", - args=[["det"], 3], + args=(["det"], 3), instrument_session=VALID_INSTRUMENT_SESSION[User.alice], ), User.alice, @@ -554,7 +554,7 @@ def test_delete_current_environment(client: BlueapiClient): pytest.param( TaskRequest( name="count", - args=[["det"]], + args=(["det"],), params={"num": 3}, instrument_session=VALID_INSTRUMENT_SESSION[User.alice], ), diff --git a/tests/unit_tests/worker/test_task_worker.py b/tests/unit_tests/worker/test_task_worker.py index 8f0a1269c5..6c1c21ddf8 100644 --- a/tests/unit_tests/worker/test_task_worker.py +++ b/tests/unit_tests/worker/test_task_worker.py @@ -962,10 +962,10 @@ def positional_plan(value: int) -> MsgGenerator: context.register_plan(positional_plan) - task = Task(name="positional_plan", args=[42]) + task = Task(name="positional_plan", args=(1,)) args, kwargs = task.prepare_params(context) - assert args == [42] + assert args == (1,) assert kwargs == {} @@ -976,10 +976,10 @@ def positional_plan(first: int, second: int) -> MsgGenerator: yield from () context.register_plan(positional_plan) - task = Task(name="positional_plan", args=[1, 2]) + task = Task(name="positional_plan", args=(1, 2)) args, kwargs = task.prepare_params(context) - assert args == [1, 2] + assert args == (1, 2) assert kwargs == {} @@ -997,11 +997,11 @@ def mixed_plan( context.register_plan(mixed_plan) task = Task( name="mixed_plan", - args=[1, 2], + args=(1, 2), params={"third": 3}, ) args, kwargs = task.prepare_params(context) - assert args == [1, 2] + assert args == (1, 2) assert kwargs == {"third": 3} @@ -1014,9 +1014,9 @@ def positional_device_plan(device: FakeDevice) -> MsgGenerator: context.register_plan(positional_device_plan) - task = Task(name="positional_device_plan", args=[fake_device.name]) + task = Task(name="positional_device_plan", args=(fake_device.name,)) args, kwargs = task.prepare_params(context) - assert args == [fake_device] + assert args == (fake_device,) assert kwargs == {} @@ -1030,12 +1030,12 @@ def positional_only_plan(val: int, /, device: FakeDevice) -> MsgGenerator: task = Task( name="positional_only_plan", - args=[1], + args=(1,), params={"device": fake_device.name}, ) args, kwargs = task.prepare_params(context) - assert args == [1] + assert args == (1,) assert kwargs == {"device": fake_device} @@ -1049,10 +1049,10 @@ def positional_device_plan(val: int, *device: FakeDevice) -> MsgGenerator: task = Task( name="positional_device_plan", - args=[1, fake_device.name, fake_device.name], + args=(1, fake_device.name, fake_device.name), ) args, kwargs = task.prepare_params(context) - assert args == [1, fake_device, fake_device] + assert args == (1, fake_device, fake_device) assert kwargs == {} @@ -1068,11 +1068,11 @@ def positional_device_plan(val: int, /, *device: FakeDevice) -> MsgGenerator: task = Task( name="positional_device_plan", - args=[1, fake_device.name, fake_device.name], + args=(1, fake_device.name, fake_device.name), ) args, kwargs = task.prepare_params(context) - assert args == [1, fake_device, fake_device] + assert args == (1, fake_device, fake_device) assert kwargs == {} @@ -1086,11 +1086,11 @@ def keyword_device_plan(val: int, **devices: FakeDevice) -> MsgGenerator: task = Task( name="keyword_device_plan", - args=[1], + args=(1,), params={"first": fake_device.name, "second": fake_device.name}, ) args, kwargs = task.prepare_params(context) - assert args == [1] + assert args == (1,) assert kwargs == {"first": fake_device, "second": fake_device} @@ -1106,11 +1106,11 @@ def positional_only_plan(val: int, /, **devices: FakeDevice) -> MsgGenerator: task = Task( name="positional_only_plan", - args=[1], + args=(1,), params={"first": fake_device.name, "second": fake_device.name}, ) args, kwargs = task.prepare_params(context) - assert args == [1] + assert args == (1,) assert kwargs == {"first": fake_device, "second": fake_device} @@ -1122,7 +1122,7 @@ def positionals(one: int, two: int) -> MsgGenerator: context = BlueskyContext() context.register_plan(positionals) - task = Task(name="positionals", args=[1], params={"one": 2}) + task = Task(name="positionals", args=(1,), params={"one": 2}) with pytest.raises(TaskParameterError, match="Invalid arguments to plan") as tpe: task.prepare_params(context) From 3877d12414dbb75da632d9a4e5089e508a6f72ac Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 14:31:42 +0100 Subject: [PATCH 10/17] Remove debug print --- src/blueapi/core/context.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/blueapi/core/context.py b/src/blueapi/core/context.py index 6cf4c1d6de..f8a96b0d6a 100644 --- a/src/blueapi/core/context.py +++ b/src/blueapi/core/context.py @@ -386,8 +386,7 @@ class Reference(target): def __get_pydantic_core_schema__( cls, source_type: Any, handler: GetCoreSchemaHandler ) -> CoreSchema: - def device_lookup(value): - print(f"Looking up {value}") + def valid(value): val = self.find_device(value) if not val: raise ValueError(f"Device {value} cannot be found") @@ -400,7 +399,7 @@ def device_lookup(value): return val return core_schema.no_info_after_validator_function( - device_lookup, handler(str) + valid, handler(str) ) @classmethod From 3062e44c9271dcc8b370f2475a250b8bb66a47b7 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 14:44:17 +0100 Subject: [PATCH 11/17] Add help text to CLI args --- src/blueapi/cli/cli.py | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index 7bc08dde7e..b64e173779 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -76,7 +76,7 @@ def convert( class PositionalArgType(ParamType): """CLI input parameter to accept a JSON value as a list of positional args""" - name = "PositionalArg" + name = "PositionalArgs" def convert( self, value: Any | None, param: Parameter | None, ctx: Context | None @@ -341,7 +341,21 @@ def on_event( @controller.command(name="run") @click.argument("name", type=str) @click.argument("parameters", type=ParametersType(), default={}, required=False) -@click.option("-a", "--args", type=PositionalArgType(), default=[], required=False) +@click.option( + "-a", + "--args", + type=PositionalArgType(), + default=[], + required=False, + help=textwrap.dedent("""\ + Arguments to be passed to the plan as positional arguments. + + This should be passed as a single string representing a JSON array and + may need quoting in some shells, eg + + --args '[1, 2, "foo"]' + """), +) @click.option( "--ws/--stomp", type=bool, From 63e911b465563f00388d843afa198bcb29547968 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 14:58:23 +0100 Subject: [PATCH 12/17] Revert "Make task args a tuple" This reverts commit b910617df18866d49e59720687d4854e49d729c9. --- src/blueapi/service/model.py | 4 +-- src/blueapi/worker/task.py | 6 ++-- tests/system_tests/test_blueapi_system.py | 4 +-- tests/unit_tests/worker/test_task_worker.py | 38 ++++++++++----------- 4 files changed, 26 insertions(+), 26 deletions(-) diff --git a/src/blueapi/service/model.py b/src/blueapi/service/model.py index 4ca22f5e98..1b690d5c04 100644 --- a/src/blueapi/service/model.py +++ b/src/blueapi/service/model.py @@ -64,9 +64,9 @@ class TaskRequest(BlueapiBaseModel): """ name: str = Field(description="Name of plan to run") - args: tuple[Any, ...] = Field( + args: list[Any] = Field( description="Positional arguments to plan", - default_factory=tuple, + default_factory=list, exclude_if=lambda args: not args, ) params: Mapping[str, Any] = Field( diff --git a/src/blueapi/worker/task.py b/src/blueapi/worker/task.py index c32127193a..329f21cf07 100644 --- a/src/blueapi/worker/task.py +++ b/src/blueapi/worker/task.py @@ -17,9 +17,9 @@ class Task(BlueapiBaseModel): """ name: str = Field(description="Name of plan to run") - args: tuple[Any, ...] = Field( + args: list[Any] = Field( description="Positional arguments to plan", - default_factory=tuple, + default_factory=list, exclude_if=lambda args: not args, ) params: Mapping[str, Any] = Field( @@ -134,7 +134,7 @@ def __init__(self, message: str, errors: list[InvalidTaskParameter]): def from_validation_error(cls, error: ValidationError) -> Self: errors = [ InvalidTaskParameter( - loc=e.get("loc"), + loc=e.get("loc", ()), msg=e.get("msg", None), type=e.get("type", None), input=e.get("input", None), diff --git a/tests/system_tests/test_blueapi_system.py b/tests/system_tests/test_blueapi_system.py index 6c155fe6bb..9feda470c6 100644 --- a/tests/system_tests/test_blueapi_system.py +++ b/tests/system_tests/test_blueapi_system.py @@ -545,7 +545,7 @@ def test_delete_current_environment(client: BlueapiClient): pytest.param( TaskRequest( name="count", - args=(["det"], 3), + args=[["det"], 3], instrument_session=VALID_INSTRUMENT_SESSION[User.alice], ), User.alice, @@ -554,7 +554,7 @@ def test_delete_current_environment(client: BlueapiClient): pytest.param( TaskRequest( name="count", - args=(["det"],), + args=[["det"]], params={"num": 3}, instrument_session=VALID_INSTRUMENT_SESSION[User.alice], ), diff --git a/tests/unit_tests/worker/test_task_worker.py b/tests/unit_tests/worker/test_task_worker.py index 6c1c21ddf8..8f0a1269c5 100644 --- a/tests/unit_tests/worker/test_task_worker.py +++ b/tests/unit_tests/worker/test_task_worker.py @@ -962,10 +962,10 @@ def positional_plan(value: int) -> MsgGenerator: context.register_plan(positional_plan) - task = Task(name="positional_plan", args=(1,)) + task = Task(name="positional_plan", args=[42]) args, kwargs = task.prepare_params(context) - assert args == (1,) + assert args == [42] assert kwargs == {} @@ -976,10 +976,10 @@ def positional_plan(first: int, second: int) -> MsgGenerator: yield from () context.register_plan(positional_plan) - task = Task(name="positional_plan", args=(1, 2)) + task = Task(name="positional_plan", args=[1, 2]) args, kwargs = task.prepare_params(context) - assert args == (1, 2) + assert args == [1, 2] assert kwargs == {} @@ -997,11 +997,11 @@ def mixed_plan( context.register_plan(mixed_plan) task = Task( name="mixed_plan", - args=(1, 2), + args=[1, 2], params={"third": 3}, ) args, kwargs = task.prepare_params(context) - assert args == (1, 2) + assert args == [1, 2] assert kwargs == {"third": 3} @@ -1014,9 +1014,9 @@ def positional_device_plan(device: FakeDevice) -> MsgGenerator: context.register_plan(positional_device_plan) - task = Task(name="positional_device_plan", args=(fake_device.name,)) + task = Task(name="positional_device_plan", args=[fake_device.name]) args, kwargs = task.prepare_params(context) - assert args == (fake_device,) + assert args == [fake_device] assert kwargs == {} @@ -1030,12 +1030,12 @@ def positional_only_plan(val: int, /, device: FakeDevice) -> MsgGenerator: task = Task( name="positional_only_plan", - args=(1,), + args=[1], params={"device": fake_device.name}, ) args, kwargs = task.prepare_params(context) - assert args == (1,) + assert args == [1] assert kwargs == {"device": fake_device} @@ -1049,10 +1049,10 @@ def positional_device_plan(val: int, *device: FakeDevice) -> MsgGenerator: task = Task( name="positional_device_plan", - args=(1, fake_device.name, fake_device.name), + args=[1, fake_device.name, fake_device.name], ) args, kwargs = task.prepare_params(context) - assert args == (1, fake_device, fake_device) + assert args == [1, fake_device, fake_device] assert kwargs == {} @@ -1068,11 +1068,11 @@ def positional_device_plan(val: int, /, *device: FakeDevice) -> MsgGenerator: task = Task( name="positional_device_plan", - args=(1, fake_device.name, fake_device.name), + args=[1, fake_device.name, fake_device.name], ) args, kwargs = task.prepare_params(context) - assert args == (1, fake_device, fake_device) + assert args == [1, fake_device, fake_device] assert kwargs == {} @@ -1086,11 +1086,11 @@ def keyword_device_plan(val: int, **devices: FakeDevice) -> MsgGenerator: task = Task( name="keyword_device_plan", - args=(1,), + args=[1], params={"first": fake_device.name, "second": fake_device.name}, ) args, kwargs = task.prepare_params(context) - assert args == (1,) + assert args == [1] assert kwargs == {"first": fake_device, "second": fake_device} @@ -1106,11 +1106,11 @@ def positional_only_plan(val: int, /, **devices: FakeDevice) -> MsgGenerator: task = Task( name="positional_only_plan", - args=(1,), + args=[1], params={"first": fake_device.name, "second": fake_device.name}, ) args, kwargs = task.prepare_params(context) - assert args == (1,) + assert args == [1] assert kwargs == {"first": fake_device, "second": fake_device} @@ -1122,7 +1122,7 @@ def positionals(one: int, two: int) -> MsgGenerator: context = BlueskyContext() context.register_plan(positionals) - task = Task(name="positionals", args=(1,), params={"one": 2}) + task = Task(name="positionals", args=[1], params={"one": 2}) with pytest.raises(TaskParameterError, match="Invalid arguments to plan") as tpe: task.prepare_params(context) From e4851ac729e0a6000541f7db08d229976a0c6ac3 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 15:03:48 +0100 Subject: [PATCH 13/17] Remove unrelated change --- src/blueapi/worker/task.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/blueapi/worker/task.py b/src/blueapi/worker/task.py index 329f21cf07..d4cf0e809a 100644 --- a/src/blueapi/worker/task.py +++ b/src/blueapi/worker/task.py @@ -134,7 +134,7 @@ def __init__(self, message: str, errors: list[InvalidTaskParameter]): def from_validation_error(cls, error: ValidationError) -> Self: errors = [ InvalidTaskParameter( - loc=e.get("loc", ()), + loc=e.get("loc", []), msg=e.get("msg", None), type=e.get("type", None), input=e.get("input", None), From e3096850a46374eb3502ea2d34f0b51d248d124c Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 15:04:55 +0100 Subject: [PATCH 14/17] Update error message in test --- tests/system_tests/test_blueapi_client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/system_tests/test_blueapi_client.py b/tests/system_tests/test_blueapi_client.py index 540efeff44..d2147deb50 100644 --- a/tests/system_tests/test_blueapi_client.py +++ b/tests/system_tests/test_blueapi_client.py @@ -190,7 +190,7 @@ def test_plans_and_devices_are_cached(bc: BlueapiClient): def test_plan_argument_errors(bc: BlueapiClient): det, stage = bc.devices.det, bc.devices.stage - with pytest.raises(TypeError, match="multiple values for detectors"): + with pytest.raises(TypeError, match="multiple values for 'detectors'"): bc.plans.count([det], detectors=[stage.x]) with pytest.raises(TypeError, match="unexpected arguments"): bc.plans.count([det], foo=1) From 2a0d2cba236811726508a0f573ff6c4b5e3d110d Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 15:11:22 +0100 Subject: [PATCH 15/17] Fix tuple/list mismatch --- src/blueapi/cli/cli.py | 2 +- src/blueapi/client/plan_cache.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/blueapi/cli/cli.py b/src/blueapi/cli/cli.py index b64e173779..8424c3c4ec 100644 --- a/src/blueapi/cli/cli.py +++ b/src/blueapi/cli/cli.py @@ -401,7 +401,7 @@ def run_plan( foreground: bool, ws: bool | None, instrument_session: str, - args: tuple[Any, ...], + args: list[Any], parameters: TaskParameters, ) -> None: """Run a plan with parameters diff --git a/src/blueapi/client/plan_cache.py b/src/blueapi/client/plan_cache.py index 0ad2bcdfe7..acf5b27ee8 100644 --- a/src/blueapi/client/plan_cache.py +++ b/src/blueapi/client/plan_cache.py @@ -77,7 +77,7 @@ def __call__(self, *args, **kwargs) -> Any: """ req = TaskRequest( name=self.name, - args=args, + args=list(args), params=kwargs, instrument_session=self._client.instrument_session, ) From fdb2c2cf269b05fe695611297927d3b1d181afe2 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 15:21:52 +0100 Subject: [PATCH 16/17] Up the coverage --- tests/unit_tests/cli/test_cli.py | 26 ++++++++++++++++++++++- tests/unit_tests/client/test_rest.py | 10 +++++++++ tests/unit_tests/service/test_rest_api.py | 25 ++++++++++++++++++++++ 3 files changed, 60 insertions(+), 1 deletion(-) diff --git a/tests/unit_tests/cli/test_cli.py b/tests/unit_tests/cli/test_cli.py index 1840216ff8..e8458f0ac1 100644 --- a/tests/unit_tests/cli/test_cli.py +++ b/tests/unit_tests/cli/test_cli.py @@ -15,6 +15,7 @@ from bluesky.protocols import Movable from bluesky_stomp.messaging import StompClient from bluesky_stomp.models import MessageTopic +from click import BadParameter from click.testing import CliRunner from ophyd_async.core import AsyncStatus from pydantic import BaseModel @@ -23,7 +24,7 @@ from stomp.connect import StompConnection11 as Connection from blueapi import __version__ -from blueapi.cli.cli import ParametersType, main +from blueapi.cli.cli import ParametersType, PositionalArgType, main from blueapi.cli.format import OutputFormat, fmt_dict from blueapi.client.event_bus import BlueskyStreamingError from blueapi.client.rest import ( @@ -1428,6 +1429,29 @@ def test_task_parameter_type(value, result): assert t.convert(value, None, None) == result +@pytest.mark.parametrize( + "value,result", [([], []), ("[1,2,3]", [1, 2, 3]), (None, None)] +) +def test_positional_arg_type(value, result): + t = PositionalArgType() + assert t.convert(value, None, None) == result + + +@pytest.mark.parametrize( + "value,error", + [ + ("1,2,3", "not valid JSON"), + ("[foo, bar]", "not valid JSON"), + ("23", "should be a list of arguments"), + ('"string"', "should be a list of arguments"), + ], +) +def test_invalid_positional_arg_type(value, error): + t = PositionalArgType() + with pytest.raises(BadParameter, match=error): + assert t.convert(value, None, None) + + @pytest.mark.parametrize( "flag,level", [("--verbose", "DEBUG"), ("--quiet", "ERROR"), ("-v", "DEBUG"), ("-q", "ERROR")], diff --git a/tests/unit_tests/client/test_rest.py b/tests/unit_tests/client/test_rest.py index 8c8c004cd8..b756b0b75f 100644 --- a/tests/unit_tests/client/test_rest.py +++ b/tests/unit_tests/client/test_rest.py @@ -281,6 +281,16 @@ def test_parameter_error_extra_string(): assert str(p1) == "Unexpected field 'foo'" +def test_parameter_error_unknown_general_error(): + p1 = ParameterError( + loc=["body", "params"], + msg="Something else is wrong", + type="TypeError", + input=None, + ) + assert str(p1) == "Something else is wrong" + + def test_parameter_error_other_string(): p1 = ParameterError( loc=["body", "parameters", "field_one", 0], diff --git a/tests/unit_tests/service/test_rest_api.py b/tests/unit_tests/service/test_rest_api.py index a02b1e3016..115ae5aae4 100644 --- a/tests/unit_tests/service/test_rest_api.py +++ b/tests/unit_tests/service/test_rest_api.py @@ -371,6 +371,31 @@ def test_create_task_parameter_error(mock_runner: Mock, client: TestClient) -> N } +def test_create_task_parameter_type_error( + mock_runner: Mock, client: TestClient +) -> None: + mock_runner.run.side_effect = [TypeError("Some other error")] + + response = client.post( + "/tasks", + json={ + "name": "my-plan", + "instrument_session": FAKE_INSTRUMENT_SESSION, + }, + ) + assert response.status_code == 422 + assert response.json() == { + "detail": [ + { + "input": None, + "loc": ["body", "params"], + "msg": "Some other error", + "type": "invalid", + } + ] + } + + def test_put_plan_begins_task(client: TestClient) -> None: task_id = "04cd9aa6-b902-414b-ae4b-49ea4200e957" From bb4b7a09d08930291fa955a60421b75bf31d8405 Mon Sep 17 00:00:00 2001 From: Peter Holloway Date: Thu, 22 Oct 2026 17:29:25 +0100 Subject: [PATCH 17/17] Fix error message mismatch in client system test --- tests/system_tests/test_blueapi_client.py | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/tests/system_tests/test_blueapi_client.py b/tests/system_tests/test_blueapi_client.py index d2147deb50..f16e267cf4 100644 --- a/tests/system_tests/test_blueapi_client.py +++ b/tests/system_tests/test_blueapi_client.py @@ -188,16 +188,18 @@ def test_plans_and_devices_are_cached(bc: BlueapiClient): assert bc.devices is bc.devices -def test_plan_argument_errors(bc: BlueapiClient): - det, stage = bc.devices.det, bc.devices.stage - with pytest.raises(TypeError, match="multiple values for 'detectors'"): - bc.plans.count([det], detectors=[stage.x]) - with pytest.raises(TypeError, match="unexpected arguments"): - bc.plans.count([det], foo=1) - with pytest.raises(TypeError, match="too many arguments"): - bc.plans.sleep(1, 2) - with pytest.raises(TypeError, match="Missing argument"): - bc.plans.count() +@pytest.mark.parametrize( + "a,kw,err", + [ + (("det",), {"detectors": ["det"]}, "multiple values for argument 'detectors'"), + (("det",), {"foo": "bar"}, "got an unexpected keyword argument 'foo'"), + ((("det",), 2, 3, 4, 5), {}, "too many positional arguments"), + ((), {}, "Missing value for 'detectors'"), + ], +) +def test_plan_argument_errors(bc: BlueapiClient, a, kw, err: str): + with pytest.raises(TypeError, match=err): + bc.plans.count(*a, **kw) def _record_events(bc: BlueapiClient) -> tuple[list[AnyEvent], int]: