From 26ab1ef6bf2610a218d1c508adf2d81dd610d7be Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 09:57:01 +0100 Subject: [PATCH 01/40] Add anon_queue to pixl_core with different message and no priority --- pixl_core/src/core/anon_queue/__init__.py | 20 +++++ pixl_core/src/core/anon_queue/_base.py | 91 +++++++++++++++++++++++ pixl_core/src/core/anon_queue/message.py | 65 ++++++++++++++++ pixl_core/src/core/anon_queue/producer.py | 86 +++++++++++++++++++++ 4 files changed, 262 insertions(+) create mode 100644 pixl_core/src/core/anon_queue/__init__.py create mode 100644 pixl_core/src/core/anon_queue/_base.py create mode 100644 pixl_core/src/core/anon_queue/message.py create mode 100644 pixl_core/src/core/anon_queue/producer.py diff --git a/pixl_core/src/core/anon_queue/__init__.py b/pixl_core/src/core/anon_queue/__init__.py new file mode 100644 index 000000000..e477b448e --- /dev/null +++ b/pixl_core/src/core/anon_queue/__init__.py @@ -0,0 +1,20 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""RabbitMQ consumer for Pixl""" + +from __future__ import annotations + +from .subscriber import PixlConsumer + +__all__ = ["PixlConsumer"] diff --git a/pixl_core/src/core/anon_queue/_base.py b/pixl_core/src/core/anon_queue/_base.py new file mode 100644 index 000000000..b5bcdf2ef --- /dev/null +++ b/pixl_core/src/core/anon_queue/_base.py @@ -0,0 +1,91 @@ +# Copyright (c) University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from __future__ import annotations + +import os +from typing import Any + +import pika +from loguru import logger + + +class PixlQueueInterface: + def __init__( + self, + queue_name: str, + host: str = "localhost", + port: int = 5672, + username: str = "guest", + password: str = "guest", # noqa: S107 + ) -> None: + """ + Generic RabbitMQ interface. Environment variables RABBITMQ_ take precedence + over arguments + + :param queue_name: Name of the queue this interfaces to. + :param host: Hostname of the RabbitMQ service. + :param port: Port on which RabbitMQ service is running. + :param username: RabbitMQ username as configured for queue. + :param password: RabbitMQ user password as configured for queue. + """ + self.queue_name = queue_name + + self._host = os.environ.get("RABBITMQ_HOST", default=host) + self._port = int(os.environ.get("RABBITMQ_PORT", default=port)) + self._username = os.environ.get("RABBITMQ_USERNAME", default=username) + self._password = os.environ.get("RABBITMQ_PASSWORD", default=password) + + self._connection: Any = None + self._channel: Any = None + self._queue: Any = None + + +class PixlBlockingInterface(PixlQueueInterface): + def __enter__(self) -> Any: + """Establishes connection to RabbitMQ service.""" + params = pika.ConnectionParameters( + host=self._host, + port=self._port, + credentials=pika.PlainCredentials(self._username, self._password), + ) + + if self._connection is None or self._connection.is_closed: + self._connection = pika.BlockingConnection(params) + + if self._channel is None or self._channel.is_closed: + self._channel = self._connection.channel() + self._queue = self._channel.queue_declare( + queue=self.queue_name, + durable=True, + ) + + logger.debug("Connected to {}", self.queue_name) + return self + + def __exit__(self, *args: object, **kwargs: Any) -> None: + """Shutdown the connection to RabbitMQ service.""" + self._channel.close() + self._connection.close() + + @property + def connection_open(self) -> bool: + return bool(self._connection.is_open) + + @property + def message_count(self) -> int: + try: + return int(self._queue.method.message_count) + except (ValueError, TypeError): + logger.exception("Failed to determine the number of messages. Returning 0") + return 0 diff --git a/pixl_core/src/core/anon_queue/message.py b/pixl_core/src/core/anon_queue/message.py new file mode 100644 index 000000000..07138393e --- /dev/null +++ b/pixl_core/src/core/anon_queue/message.py @@ -0,0 +1,65 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Classes to represent messages in the patient queue.""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import Any + +from jsonpickle import decode, encode +from loguru import logger + + +@dataclass +class Message: + """ + Representation of a RabbitMQ message containing the information + to identify an anonymisation request. + """ + + resource_id: str + study_uid: str + series_uid: str + project_name: str + + @property + def identifier(self) -> str: + """Identifier for message""" + return (f"Message({self.resource_id=} {self.study_uid=} {self.series_uid=}").replace( + "self.", "" + ) + + def serialise(self, *, deserialisable: bool = True) -> bytes: + """ + Serialise the message into a JSON string and convert to bytes. + + :param deserialisable: If True, the serialised message will be deserialisable, by setting + the unpicklable flag to False in jsonpickle.encode(), meaning that the original Message + object can be recovered by `deserialise()`. If False, calling `deserialise()` on the + serialised message will return a dictionary. + """ + logger.trace("Serialising {}", self) + return str.encode(encode(self, unpicklable=deserialisable)) + + +def deserialise(serialised_msg: bytes) -> Any: + """ + Deserialise a message from a bytes-encoded JSON string. + If the message was serialised with `deserialisable=True`, the original Message object will be + returned. Otherwise, a dictionary will be returned. + + :param serialised_msg: The serialised message. + """ + return decode(serialised_msg) # noqa: S301, since we control the input, so no security risks diff --git a/pixl_core/src/core/anon_queue/producer.py b/pixl_core/src/core/anon_queue/producer.py new file mode 100644 index 000000000..fdbd9b345 --- /dev/null +++ b/pixl_core/src/core/anon_queue/producer.py @@ -0,0 +1,86 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Producer for RabbitMQ""" + +from __future__ import annotations + +from typing import TYPE_CHECKING + +from loguru import logger +from opentelemetry import trace +from pika import BasicProperties, DeliveryMode + +from ._base import PixlBlockingInterface + +if TYPE_CHECKING: + from core.anon_queue.message import Message + +tracer = trace.get_tracer("pixl_core.anon_queue.producer") + + +class PixlProducer(PixlBlockingInterface): + """Generic publisher for RabbitMQ""" + + def publish(self, messages: list[Message]) -> None: + """ + Sends a list of serialised messages to a queue. + :param messages: list of messages to be sent to queue + """ + if len(messages) == 0: + logger.warning("List of messages is empty so nothing will be published to queue.") + return + + logger.info("Publishing {} messages to queue: {}", len(messages), self.queue_name) + for msg in messages: + attributes = { + "project_name": msg.project_name, + "resource_id": msg.resource_id, + "series_uid": msg.series_uid, + "study_uid": msg.study_uid, + } + with tracer.start_as_current_span("publish_message", attributes=attributes): + self._publish_message(msg) + + def _publish_message(self, message: Message) -> None: + """ + Publish a single serialised message to a queue. + :param message: message to be sent to queue + """ + serialised_msg = message.serialise() + self._channel.basic_publish( + exchange="", + routing_key=self.queue_name, + body=serialised_msg, + properties=BasicProperties( + delivery_mode=DeliveryMode.Persistent, + ), + ) + + logger.bind( + project_name=message.project_name, + resource_id=message.resource_id, + series_uid=message.series_uid, + study_uid=message.study_uid, + ).debug( + "Message {} published to queue {}", + message, + self.queue_name, + ) + + def clear_queue(self) -> None: + """ + Triggering a purge of all the messages currently in the queue. Mainly used to + clean after tests. + """ + self._channel.queue_purge(queue=self.queue_name) From 735d67b78eac732434a496e2fa97cd631f4cf518 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 10:24:09 +0100 Subject: [PATCH 02/40] Remove async functionality from anon_queue subscriber --- pixl_core/src/core/anon_queue/subscriber.py | 128 ++++++++++++++++++++ 1 file changed, 128 insertions(+) create mode 100644 pixl_core/src/core/anon_queue/subscriber.py diff --git a/pixl_core/src/core/anon_queue/subscriber.py b/pixl_core/src/core/anon_queue/subscriber.py new file mode 100644 index 000000000..5c987dc5b --- /dev/null +++ b/pixl_core/src/core/anon_queue/subscriber.py @@ -0,0 +1,128 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Subscriber for RabbitMQ""" + +from __future__ import annotations + +from typing import TYPE_CHECKING, Any + +import pika +from decouple import config + +from core.anon_queue._base import PixlQueueInterface +from core.anon_queue.message import deserialise +from core.anon_queue.producer import PixlProducer +from core.exceptions import ( + PixlDiscardError, + PixlOutOfHoursError, + PixlRequeueMessageError, + PixlStudyNotInPrimaryArchiveError, +) + +if TYPE_CHECKING: + from collections.abc import Awaitable, Callable + from typing import Self + + from aio_pika.abc import AbstractIncomingMessage + + from core.anon_queue.message import Message + +import time + +from loguru import logger + + +class PixlConsumer(PixlQueueInterface): + """Connector to RabbitMQ. Consumes messages from a queue""" + + def __init__( + self, + queue_name: str, + callback: Callable[[Message], Awaitable[None]], + ) -> None: + """Creating connection to RabbitMQ queue""" + super().__init__(queue_name=queue_name) + self._callback = callback + + @property + def _url(self) -> str: + return f"amqp://{self._username}:{self._password}@{self._host}:{self._port}/" + + def __enter__(self) -> Self: + """Establishes connection to queue.""" + self._connection = pika.BlockingConnection(pika.URLParameters(self._url)) + self._channel = self._connection.channel() + # Set number of messages in flight + max_in_flight = config("PIXL_MAX_MESSAGES_IN_FLIGHT", cast=int) + logger.info("Pika will consume up to {} messages concurrently", max_in_flight) + self._channel.set_qos(prefetch_count=max_in_flight) + self._queue = self._channel.declare_queue( + self.queue_name, + durable=True, + ) + return self + + def _process_message(self, message: AbstractIncomingMessage) -> None: + + pixl_message: Message = deserialise(message.body) + logger.debug("Picked up from queue: {}", pixl_message.identifier) + try: + self._callback(pixl_message) + except PixlRequeueMessageError as requeue: + logger.trace("Requeue message: {} from {}", pixl_message.identifier, requeue) + time.sleep(1) + message.reject(requeue=True) + except PixlStudyNotInPrimaryArchiveError as discard: + logger.info( + "Discard message: {} from {}. Sending to secondary imaging queue with priority {}.", + pixl_message.identifier, + discard, + message.priority, + ) + time.sleep(1) + message.reject(requeue=False) + with PixlProducer( + queue_name="imaging-secondary", + host=config("RABBITMQ_HOST"), + port=config("RABBITMQ_PORT", cast=int), + username=config("RABBITMQ_USERNAME"), + password=config("RABBITMQ_PASSWORD"), + ) as producer: + producer.publish([pixl_message], priority=message.priority) + except PixlOutOfHoursError as nack_requeue: + logger.trace( + "Nack and requeue message: {} from {}", pixl_message.identifier, nack_requeue + ) + time.sleep(10) + message.nack(requeue=True) + except PixlDiscardError as exception: + logger.warning("Failed message {}: {}", pixl_message.identifier, exception) + (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin + except Exception: # noqa: BLE001 + logger.exception( + "Failed to process {}. Not re-queuing message", + pixl_message.identifier, + ) + (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin + else: + logger.success("Finished message {}", pixl_message.identifier) + message.ack() + + def run(self) -> None: + """Processes messages from queue.""" + self._queue.consume(self._process_message) + + def __exit__(self, *args: object, **kwargs: Any) -> None: + """Requirement for the context manager""" From 91b891bed605e0c213e7a3c497ed4013a0fa8b1e Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 10:54:10 +0100 Subject: [PATCH 03/40] Replace POST with anon queue in pixl_imaging/_orthanc.py and update naming of queue objects --- pixl_core/src/core/anon_queue/__init__.py | 4 ++-- pixl_core/src/core/anon_queue/message.py | 10 ++++----- pixl_core/src/core/anon_queue/producer.py | 24 ++++++++++----------- pixl_core/src/core/anon_queue/subscriber.py | 14 ++++++------ pixl_imaging/src/pixl_imaging/_orthanc.py | 21 +++++++++++------- 5 files changed, 39 insertions(+), 34 deletions(-) diff --git a/pixl_core/src/core/anon_queue/__init__.py b/pixl_core/src/core/anon_queue/__init__.py index e477b448e..7b6fd6a6d 100644 --- a/pixl_core/src/core/anon_queue/__init__.py +++ b/pixl_core/src/core/anon_queue/__init__.py @@ -15,6 +15,6 @@ from __future__ import annotations -from .subscriber import PixlConsumer +from .subscriber import AnonymisationPixlConsumer -__all__ = ["PixlConsumer"] +__all__ = ["AnonymisationPixlConsumer"] diff --git a/pixl_core/src/core/anon_queue/message.py b/pixl_core/src/core/anon_queue/message.py index 07138393e..1c8d0ce14 100644 --- a/pixl_core/src/core/anon_queue/message.py +++ b/pixl_core/src/core/anon_queue/message.py @@ -23,21 +23,21 @@ @dataclass -class Message: +class AnonymisationMessage: """ Representation of a RabbitMQ message containing the information to identify an anonymisation request. """ - resource_id: str - study_uid: str - series_uid: str + resource_ids: list[str] + study_uids: list[str] + series_uids: list[str] project_name: str @property def identifier(self) -> str: """Identifier for message""" - return (f"Message({self.resource_id=} {self.study_uid=} {self.series_uid=}").replace( + return (f"Message({self.resource_ids=} {self.study_uids=} {self.series_uids=}").replace( "self.", "" ) diff --git a/pixl_core/src/core/anon_queue/producer.py b/pixl_core/src/core/anon_queue/producer.py index fdbd9b345..5bcc979b4 100644 --- a/pixl_core/src/core/anon_queue/producer.py +++ b/pixl_core/src/core/anon_queue/producer.py @@ -24,15 +24,15 @@ from ._base import PixlBlockingInterface if TYPE_CHECKING: - from core.anon_queue.message import Message + from core.anon_queue.message import AnonymisationMessage tracer = trace.get_tracer("pixl_core.anon_queue.producer") -class PixlProducer(PixlBlockingInterface): - """Generic publisher for RabbitMQ""" +class AnonymisationProducer(PixlBlockingInterface): + """Anonymisation publisher for RabbitMQ""" - def publish(self, messages: list[Message]) -> None: + def publish(self, messages: list[AnonymisationMessage]) -> None: """ Sends a list of serialised messages to a queue. :param messages: list of messages to be sent to queue @@ -45,14 +45,14 @@ def publish(self, messages: list[Message]) -> None: for msg in messages: attributes = { "project_name": msg.project_name, - "resource_id": msg.resource_id, - "series_uid": msg.series_uid, - "study_uid": msg.study_uid, + "resource_id": msg.resource_ids, + "series_uid": msg.series_uids, + "study_uid": msg.study_uids, } with tracer.start_as_current_span("publish_message", attributes=attributes): self._publish_message(msg) - def _publish_message(self, message: Message) -> None: + def _publish_message(self, message: AnonymisationMessage) -> None: """ Publish a single serialised message to a queue. :param message: message to be sent to queue @@ -69,11 +69,11 @@ def _publish_message(self, message: Message) -> None: logger.bind( project_name=message.project_name, - resource_id=message.resource_id, - series_uid=message.series_uid, - study_uid=message.study_uid, + resource_id=message.resource_ids, + series_uid=message.series_uids, + study_uid=message.study_uids, ).debug( - "Message {} published to queue {}", + "AnonymisationMessage {} published to queue {}", message, self.queue_name, ) diff --git a/pixl_core/src/core/anon_queue/subscriber.py b/pixl_core/src/core/anon_queue/subscriber.py index 5c987dc5b..8372cb9d6 100644 --- a/pixl_core/src/core/anon_queue/subscriber.py +++ b/pixl_core/src/core/anon_queue/subscriber.py @@ -23,7 +23,7 @@ from core.anon_queue._base import PixlQueueInterface from core.anon_queue.message import deserialise -from core.anon_queue.producer import PixlProducer +from core.anon_queue.producer import AnonymisationProducer from core.exceptions import ( PixlDiscardError, PixlOutOfHoursError, @@ -37,20 +37,20 @@ from aio_pika.abc import AbstractIncomingMessage - from core.anon_queue.message import Message + from core.anon_queue.message import AnonymisationMessage import time from loguru import logger -class PixlConsumer(PixlQueueInterface): +class AnonymisationPixlConsumer(PixlQueueInterface): """Connector to RabbitMQ. Consumes messages from a queue""" def __init__( self, queue_name: str, - callback: Callable[[Message], Awaitable[None]], + callback: Callable[[AnonymisationMessage], Awaitable[None]], ) -> None: """Creating connection to RabbitMQ queue""" super().__init__(queue_name=queue_name) @@ -76,7 +76,7 @@ def __enter__(self) -> Self: def _process_message(self, message: AbstractIncomingMessage) -> None: - pixl_message: Message = deserialise(message.body) + pixl_message: AnonymisationMessage = deserialise(message.body) logger.debug("Picked up from queue: {}", pixl_message.identifier) try: self._callback(pixl_message) @@ -93,8 +93,8 @@ def _process_message(self, message: AbstractIncomingMessage) -> None: ) time.sleep(1) message.reject(requeue=False) - with PixlProducer( - queue_name="imaging-secondary", + with AnonymisationProducer( + queue_name="anonymisation", host=config("RABBITMQ_HOST"), port=config("RABBITMQ_PORT", cast=int), username=config("RABBITMQ_USERNAME"), diff --git a/pixl_imaging/src/pixl_imaging/_orthanc.py b/pixl_imaging/src/pixl_imaging/_orthanc.py index b87305364..b43945f2d 100644 --- a/pixl_imaging/src/pixl_imaging/_orthanc.py +++ b/pixl_imaging/src/pixl_imaging/_orthanc.py @@ -18,9 +18,12 @@ from typing import Any import aiohttp +from core.anon_queue.message import AnonymisationMessage +from core.anon_queue.producer import AnonymisationProducer from core.exceptions import PixlDiscardError, PixlRequeueMessageError from decouple import config from loguru import logger +from pixl_cli._config import SERVICE_SETTINGS class Orthanc: @@ -300,12 +303,14 @@ async def notify_anon_to_retrieve_study_resources( series_uids = series_uid.split("\\") if series_uid else [] - await self._post( - path="/import-from-raw", - data={ - "ResourceIDs": resource_ids, - "StudyInstanceUIDs": study_uids, - "SeriesInstanceUIDs": series_uids, - "ProjectName": project_name, - }, + message = AnonymisationMessage( + resource_ids=resource_ids, + series_uids=series_uids, + study_uids=study_uids, + project_name=project_name, ) + + with AnonymisationProducer( + queue_name="anonymisation", **SERVICE_SETTINGS["rabbitmq"] + ) as producer: + producer.publish(message) From 6c5b362cfd0233aa625e0ed202e0dc97bb08bfb0 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 10:57:10 +0100 Subject: [PATCH 04/40] AnonymisationProducer publishes [message] instead of message --- pixl_core/src/core/anon_queue/producer.py | 6 +++--- pixl_imaging/src/pixl_imaging/_orthanc.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/pixl_core/src/core/anon_queue/producer.py b/pixl_core/src/core/anon_queue/producer.py index 5bcc979b4..31d0ec0b6 100644 --- a/pixl_core/src/core/anon_queue/producer.py +++ b/pixl_core/src/core/anon_queue/producer.py @@ -45,9 +45,9 @@ def publish(self, messages: list[AnonymisationMessage]) -> None: for msg in messages: attributes = { "project_name": msg.project_name, - "resource_id": msg.resource_ids, - "series_uid": msg.series_uids, - "study_uid": msg.study_uids, + "resource_ids": msg.resource_ids, + "series_uids": msg.series_uids, + "study_uids": msg.study_uids, } with tracer.start_as_current_span("publish_message", attributes=attributes): self._publish_message(msg) diff --git a/pixl_imaging/src/pixl_imaging/_orthanc.py b/pixl_imaging/src/pixl_imaging/_orthanc.py index b43945f2d..275bc4248 100644 --- a/pixl_imaging/src/pixl_imaging/_orthanc.py +++ b/pixl_imaging/src/pixl_imaging/_orthanc.py @@ -313,4 +313,4 @@ async def notify_anon_to_retrieve_study_resources( with AnonymisationProducer( queue_name="anonymisation", **SERVICE_SETTINGS["rabbitmq"] ) as producer: - producer.publish(message) + producer.publish([message]) From 0e0ee07eccece5b28c51adc0fdfde4aed03488e4 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 11:10:49 +0100 Subject: [PATCH 05/40] Clean up anon_queue subscriber --- pixl_core/src/core/anon_queue/subscriber.py | 37 ++++++++------------- 1 file changed, 13 insertions(+), 24 deletions(-) diff --git a/pixl_core/src/core/anon_queue/subscriber.py b/pixl_core/src/core/anon_queue/subscriber.py index 8372cb9d6..56186342e 100644 --- a/pixl_core/src/core/anon_queue/subscriber.py +++ b/pixl_core/src/core/anon_queue/subscriber.py @@ -23,20 +23,16 @@ from core.anon_queue._base import PixlQueueInterface from core.anon_queue.message import deserialise -from core.anon_queue.producer import AnonymisationProducer from core.exceptions import ( PixlDiscardError, PixlOutOfHoursError, PixlRequeueMessageError, - PixlStudyNotInPrimaryArchiveError, ) if TYPE_CHECKING: from collections.abc import Awaitable, Callable from typing import Self - from aio_pika.abc import AbstractIncomingMessage - from core.anon_queue.message import AnonymisationMessage import time @@ -45,7 +41,7 @@ class AnonymisationPixlConsumer(PixlQueueInterface): - """Connector to RabbitMQ. Consumes messages from a queue""" + """Connector to RabbitMQ. Consumes messages from anonymisation queue""" def __init__( self, @@ -74,7 +70,7 @@ def __enter__(self) -> Self: ) return self - def _process_message(self, message: AbstractIncomingMessage) -> None: + def _process_message(self, message: Any) -> None: pixl_message: AnonymisationMessage = deserialise(message.body) logger.debug("Picked up from queue: {}", pixl_message.identifier) @@ -84,23 +80,6 @@ def _process_message(self, message: AbstractIncomingMessage) -> None: logger.trace("Requeue message: {} from {}", pixl_message.identifier, requeue) time.sleep(1) message.reject(requeue=True) - except PixlStudyNotInPrimaryArchiveError as discard: - logger.info( - "Discard message: {} from {}. Sending to secondary imaging queue with priority {}.", - pixl_message.identifier, - discard, - message.priority, - ) - time.sleep(1) - message.reject(requeue=False) - with AnonymisationProducer( - queue_name="anonymisation", - host=config("RABBITMQ_HOST"), - port=config("RABBITMQ_PORT", cast=int), - username=config("RABBITMQ_USERNAME"), - password=config("RABBITMQ_PASSWORD"), - ) as producer: - producer.publish([pixl_message], priority=message.priority) except PixlOutOfHoursError as nack_requeue: logger.trace( "Nack and requeue message: {} from {}", pixl_message.identifier, nack_requeue @@ -122,7 +101,17 @@ def _process_message(self, message: AbstractIncomingMessage) -> None: def run(self) -> None: """Processes messages from queue.""" - self._queue.consume(self._process_message) + self._channel.basic_consume( + queue=self.queue_name, + on_message_callback=self._process_message, + auto_ack=False, + ) + self._channel.start_consuming() def __exit__(self, *args: object, **kwargs: Any) -> None: """Requirement for the context manager""" + if self._channel is not None and self._channel.is_open: + self._channel.close() + + if self._connection is not None and self._connection.is_open: + self._connection.close() From 48cee589d82d36c68e8f146c372514e110b38bd2 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 11:39:44 +0100 Subject: [PATCH 06/40] Add RabbitMQ anon_queue in orthanc-anon plugin --- orthanc/orthanc-anon/plugin/pixl.py | 34 +++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 5cb0504ce..ce8ec635e 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -34,6 +34,7 @@ import pydicom import requests +from core.anon_queue.subscriber import AnonymisationPixlConsumer from core.exceptions import PixlDiscardError, PixlSkipInstanceError from core.metrics import ( record_instance_deidentification_failure, @@ -64,6 +65,7 @@ if TYPE_CHECKING: from typing import Any + from core.anon_queue.message import AnonymisationMessage from core.project_config.pixl_config_model import PixlConfig from opentelemetry.context import Context from pixl_dcmd.dicom_helpers import StudyInfo @@ -261,6 +263,38 @@ def ImportStudiesFromRaw(output, uri, **request): # noqa: ARG001 output.AnswerBuffer(response, "application/json") +def process_anonymisation_message(message: AnonymisationMessage) -> None: + """ + Import studies from Orthanc Raw. + + Offload to a thread pool executor to avoid blocking the Orthanc main thread. + """ + data = { + "resource_ids": message.resource_ids, + "series_uids": message.series_uids, + "study_uids": message.study_uids, + "project_name": message.project_name, + } + + executor.submit(_import_studies_from_raw, data) + + +def consume_anonymisation_queue() -> None: + """Consume anonymisation requests from RabbitMQ and submit them for processing.""" + with AnonymisationPixlConsumer( + queue_name="anonymisation", + callback=process_anonymisation_message, + ) as consumer: + consumer.run() + + +consumer_thread = threading.Thread( + target=consume_anonymisation_queue, + daemon=True, +) +consumer_thread.start() + + def _import_studies_from_raw( study_resource_ids: list[str], study_uids: list[str], From 250f7753f5b4232931bb0e3d3d8a8884d14494ba Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 13:48:20 +0100 Subject: [PATCH 07/40] Add anonymisation queue to _message_count() --- cli/src/pixl_cli/_message_processing.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cli/src/pixl_cli/_message_processing.py b/cli/src/pixl_cli/_message_processing.py index f7bb93b65..6377e783f 100644 --- a/cli/src/pixl_cli/_message_processing.py +++ b/cli/src/pixl_cli/_message_processing.py @@ -129,6 +129,8 @@ def _message_count(queues_to_populate: list[str]) -> int: if "imaging-primary" in queues_to_populate: queues_to_count.add("imaging-secondary") + queues_to_count.add("anonymisation") + messages_in_queues = 0 for queue in queues_to_count: with PixlBlockingInterface(queue_name=queue, **SERVICE_SETTINGS["rabbitmq"]) as rabbitmq: From 0e3c3d1561ba01abb3ecc8d3448d4a68e041f536 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 14:41:21 +0100 Subject: [PATCH 08/40] Added tests for anon_queue and modified subscriber to move away from async --- pixl_core/src/core/anon_queue/subscriber.py | 6 +- pixl_core/tests/anon_queue/test_message.py | 33 ++++++++++ pixl_core/tests/anon_queue/test_producer.py | 41 ++++++++++++ pixl_core/tests/anon_queue/test_subscriber.py | 65 +++++++++++++++++++ pixl_core/tests/conftest.py | 12 ++++ 5 files changed, 154 insertions(+), 3 deletions(-) create mode 100644 pixl_core/tests/anon_queue/test_message.py create mode 100644 pixl_core/tests/anon_queue/test_producer.py create mode 100644 pixl_core/tests/anon_queue/test_subscriber.py diff --git a/pixl_core/src/core/anon_queue/subscriber.py b/pixl_core/src/core/anon_queue/subscriber.py index 56186342e..16a4e2fdc 100644 --- a/pixl_core/src/core/anon_queue/subscriber.py +++ b/pixl_core/src/core/anon_queue/subscriber.py @@ -63,9 +63,9 @@ def __enter__(self) -> Self: # Set number of messages in flight max_in_flight = config("PIXL_MAX_MESSAGES_IN_FLIGHT", cast=int) logger.info("Pika will consume up to {} messages concurrently", max_in_flight) - self._channel.set_qos(prefetch_count=max_in_flight) - self._queue = self._channel.declare_queue( - self.queue_name, + self._channel.basic_qos(prefetch_count=max_in_flight) + self._queue = self._channel.queue_declare( + queue=self.queue_name, durable=True, ) return self diff --git a/pixl_core/tests/anon_queue/test_message.py b/pixl_core/tests/anon_queue/test_message.py new file mode 100644 index 000000000..6db99e6fe --- /dev/null +++ b/pixl_core/tests/anon_queue/test_message.py @@ -0,0 +1,33 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from __future__ import annotations + +from core.anon_queue.message import deserialise + + +def test_serialise(mock_anon_message) -> None: + """Checks that messages can be correctly serialised""" + msg_body = mock_anon_message.serialise(deserialisable=False) + assert ( + msg_body == b'{"resource_ids": ["resource-1", "resource-2"], ' + b'"study_uids": ["1.2.3", "4.5.6"], ' + b'"series_uids": ["1.2.3.1", "1.2.3.2"], ' + b'"project_name": "test project"}' + ) + + +def test_deserialise(mock_message) -> None: + """Checks if deserialised messages are the same as the original""" + serialised_msg = mock_message.serialise() + assert deserialise(serialised_msg) == mock_message diff --git a/pixl_core/tests/anon_queue/test_producer.py b/pixl_core/tests/anon_queue/test_producer.py new file mode 100644 index 000000000..5070e1868 --- /dev/null +++ b/pixl_core/tests/anon_queue/test_producer.py @@ -0,0 +1,41 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from __future__ import annotations + +import pytest + +from core.anon_queue.producer import AnonymisationProducer + +TEST_QUEUE = "test_publish" + + +@pytest.mark.usefixtures("run_containers") +def test_create_pixl_producer() -> None: + """Checks that AnonymisationProducer can be instantiated.""" + with AnonymisationProducer(queue_name=TEST_QUEUE) as pp: + assert pp.connection_open + + +@pytest.mark.usefixtures("run_containers") +def test_publish(mock_anon_message) -> None: + """ + Checks that after publishing, there is one message in the queue. + Will only work if nothing has been added to queue before. + """ + with AnonymisationProducer(queue_name=TEST_QUEUE) as pp: + pp.clear_queue() + pp.publish(messages=[mock_anon_message]) + + with AnonymisationProducer(queue_name=TEST_QUEUE) as pp: + assert pp.message_count == 1 diff --git a/pixl_core/tests/anon_queue/test_subscriber.py b/pixl_core/tests/anon_queue/test_subscriber.py new file mode 100644 index 000000000..fcc65c2b9 --- /dev/null +++ b/pixl_core/tests/anon_queue/test_subscriber.py @@ -0,0 +1,65 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from __future__ import annotations + +from unittest.mock import Mock + +import pytest + +from core.anon_queue.subscriber import AnonymisationPixlConsumer + +TEST_QUEUE = "test_consume" + + +class ExpectedTestError(Exception): + """Expected error for testing.""" + + +@pytest.mark.usefixtures("run_containers") +def test_run() -> None: + """Checks that the consumer starts consuming messages.""" + callback = Mock() + + with AnonymisationPixlConsumer( + queue_name=TEST_QUEUE, + callback=callback, + ) as consumer: + consumer._channel.basic_consume = Mock() + consumer._channel.start_consuming = Mock() + + consumer.run() + + consumer._channel.basic_consume.assert_called_once_with( + queue=TEST_QUEUE, + on_message_callback=consumer._process_message, + auto_ack=False, + ) + consumer._channel.start_consuming.assert_called_once() + + +@pytest.mark.usefixtures("run_containers") +def test_process_message(mock_anon_message) -> None: + """Checks that a received message is passed to the callback.""" + callback = Mock() + + with AnonymisationPixlConsumer( + queue_name=TEST_QUEUE, + callback=callback, + ) as consumer: + message = Mock() + message.body = mock_anon_message.serialise() + + consumer._process_message(message) + + callback.assert_called_once_with(mock_anon_message) diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index 324ce88e5..903ad4972 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -34,6 +34,7 @@ from sqlalchemy import Engine, create_engine from sqlalchemy.orm import Session, sessionmaker +from core.anon_queue.message import AnonymisationMessage from core.db.models import Base, Extract, Image from core.logging import OTelSink from core.patient_queue.message import Message @@ -236,6 +237,17 @@ def mock_message() -> Message: ) +@pytest.fixture +def mock_anon_message() -> AnonymisationMessage: + """An example AnonymisationMessage used for testing""" + return AnonymisationMessage( + resource_ids=["resource-1", "resource-2"], + study_uids=["1.2.3", "4.5.6"], + series_uids=["1.2.3.1", "1.2.3.2"], + project_name="test project", + ) + + @pytest.fixture def log_exporter() -> InMemoryLogRecordExporter: """In-memory exporter capturing the OTel log records the sink emits.""" From 050ddfbbef16cabf3eebf6ee2145bf2b66f26401 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 14:49:53 +0100 Subject: [PATCH 09/40] Remove SERVICE_SETTINGS[rabbitmq] from AnonymisationProducer --- pixl_imaging/src/pixl_imaging/_orthanc.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pixl_imaging/src/pixl_imaging/_orthanc.py b/pixl_imaging/src/pixl_imaging/_orthanc.py index 275bc4248..0f9229a35 100644 --- a/pixl_imaging/src/pixl_imaging/_orthanc.py +++ b/pixl_imaging/src/pixl_imaging/_orthanc.py @@ -23,7 +23,6 @@ from core.exceptions import PixlDiscardError, PixlRequeueMessageError from decouple import config from loguru import logger -from pixl_cli._config import SERVICE_SETTINGS class Orthanc: @@ -310,7 +309,5 @@ async def notify_anon_to_retrieve_study_resources( project_name=project_name, ) - with AnonymisationProducer( - queue_name="anonymisation", **SERVICE_SETTINGS["rabbitmq"] - ) as producer: + with AnonymisationProducer(queue_name="anonymisation") as producer: producer.publish([message]) From e093a011e2b045154d46636a7070ad72ef482e43 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 14:52:20 +0100 Subject: [PATCH 10/40] Add anonymisation queue to cli tests --- cli/tests/test_message_processing.py | 6 +++--- cli/tests/test_messages_from_files.py | 20 +++++++++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/cli/tests/test_message_processing.py b/cli/tests/test_message_processing.py index a0391cc60..7bb627cd2 100644 --- a/cli/tests/test_message_processing.py +++ b/cli/tests/test_message_processing.py @@ -51,7 +51,7 @@ def test_no_retry_if_none_exported(example_messages_df, db_session, mock_publish retry_until_export_count_is_unchanged( example_messages_df, num_retries=5, - queues_to_populate=["imaging-primary"], + queues_to_populate=["imaging-primary", "anonymisation"], messages_priority=1, ) @@ -72,7 +72,7 @@ def test_retry_with_image_exported_and_no_change( retry_until_export_count_is_unchanged( example_messages_df, num_retries=5, - queues_to_populate=["imaging-primary"], + queues_to_populate=["imaging-primary", "anonymisation"], messages_priority=1, ) @@ -93,7 +93,7 @@ def test_retry_with_image_exported_and_no_change_multiple_projects( retry_until_export_count_is_unchanged( example_messages_multiple_projects_df, num_retries=5, - queues_to_populate=["imaging-primary"], + queues_to_populate=["imaging-primary", "anonymisation"], messages_priority=1, ) diff --git a/cli/tests/test_messages_from_files.py b/cli/tests/test_messages_from_files.py index 03ea8138b..9e17b306e 100644 --- a/cli/tests/test_messages_from_files.py +++ b/cli/tests/test_messages_from_files.py @@ -96,7 +96,9 @@ def test_messages_from_csv_multiple_projects( """ input_file = omop_resources / "multiple_projects.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) + messages = populate_queue_and_db( + ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 + ) # Database has 6 rows now images_in_db = rows_in_session.query(Image).all() @@ -264,7 +266,9 @@ def test_batch_upload(omop_resources: Path, rows_in_session, mock_publisher) -> """ input_file = omop_resources / "batch_input.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) + messages = populate_queue_and_db( + ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 + ) # Database has 3 rows now images_in_db: list[Image] = rows_in_session.query(Image).all() @@ -281,7 +285,9 @@ def test_duplicate_upload(omop_resources: Path, rows_in_session, mock_publisher) """ input_file = omop_resources / "duplicate_input.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) + messages = populate_queue_and_db( + ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 + ) # Database has 3 rows now images_in_db = rows_in_session.query(Image).all() @@ -299,7 +305,9 @@ def test_upload_with_participant_id(omop_resources: Path, db_session, mock_publi """ input_file = omop_resources / "participant_id.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) + messages = populate_queue_and_db( + ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 + ) # Database has 3 rows now images_in_db: list[Image] = db_session.query(Image).all() @@ -321,7 +329,9 @@ def test_upload_with_no_participant_id(omop_resources: Path, db_session, mock_pu """ input_file = omop_resources / "batch_input.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) + messages = populate_queue_and_db( + ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 + ) # Database has 3 rows now images_in_db: list[Image] = db_session.query(Image).all() From 47830a770e760c88cd83519cd16ccbab70a0ad04 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 15:14:55 +0100 Subject: [PATCH 11/40] Remove anonymisation queue being populated from cli --- cli/tests/test_message_processing.py | 6 +++--- cli/tests/test_messages_from_files.py | 20 +++++--------------- 2 files changed, 8 insertions(+), 18 deletions(-) diff --git a/cli/tests/test_message_processing.py b/cli/tests/test_message_processing.py index 7bb627cd2..a0391cc60 100644 --- a/cli/tests/test_message_processing.py +++ b/cli/tests/test_message_processing.py @@ -51,7 +51,7 @@ def test_no_retry_if_none_exported(example_messages_df, db_session, mock_publish retry_until_export_count_is_unchanged( example_messages_df, num_retries=5, - queues_to_populate=["imaging-primary", "anonymisation"], + queues_to_populate=["imaging-primary"], messages_priority=1, ) @@ -72,7 +72,7 @@ def test_retry_with_image_exported_and_no_change( retry_until_export_count_is_unchanged( example_messages_df, num_retries=5, - queues_to_populate=["imaging-primary", "anonymisation"], + queues_to_populate=["imaging-primary"], messages_priority=1, ) @@ -93,7 +93,7 @@ def test_retry_with_image_exported_and_no_change_multiple_projects( retry_until_export_count_is_unchanged( example_messages_multiple_projects_df, num_retries=5, - queues_to_populate=["imaging-primary", "anonymisation"], + queues_to_populate=["imaging-primary"], messages_priority=1, ) diff --git a/cli/tests/test_messages_from_files.py b/cli/tests/test_messages_from_files.py index 9e17b306e..03ea8138b 100644 --- a/cli/tests/test_messages_from_files.py +++ b/cli/tests/test_messages_from_files.py @@ -96,9 +96,7 @@ def test_messages_from_csv_multiple_projects( """ input_file = omop_resources / "multiple_projects.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db( - ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 - ) + messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) # Database has 6 rows now images_in_db = rows_in_session.query(Image).all() @@ -266,9 +264,7 @@ def test_batch_upload(omop_resources: Path, rows_in_session, mock_publisher) -> """ input_file = omop_resources / "batch_input.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db( - ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 - ) + messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) # Database has 3 rows now images_in_db: list[Image] = rows_in_session.query(Image).all() @@ -285,9 +281,7 @@ def test_duplicate_upload(omop_resources: Path, rows_in_session, mock_publisher) """ input_file = omop_resources / "duplicate_input.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db( - ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 - ) + messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) # Database has 3 rows now images_in_db = rows_in_session.query(Image).all() @@ -305,9 +299,7 @@ def test_upload_with_participant_id(omop_resources: Path, db_session, mock_publi """ input_file = omop_resources / "participant_id.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db( - ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 - ) + messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) # Database has 3 rows now images_in_db: list[Image] = db_session.query(Image).all() @@ -329,9 +321,7 @@ def test_upload_with_no_participant_id(omop_resources: Path, db_session, mock_pu """ input_file = omop_resources / "batch_input.csv" messages_df = read_patient_info(input_file) - messages = populate_queue_and_db( - ["imaging-primary", "anonymisation"], messages_df, messages_priority=1 - ) + messages = populate_queue_and_db(["imaging-primary"], messages_df, messages_priority=1) # Database has 3 rows now images_in_db: list[Image] = db_session.query(Image).all() From 124246ea6a86fc71478a5238856f6f34dba20b84 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 15:15:58 +0100 Subject: [PATCH 12/40] Added __init__.py to both queues tests so differentiate same names --- pixl_core/tests/anon_queue/__init__.py | 15 +++++++++++++++ pixl_core/tests/patient_queue/__init__.py | 15 +++++++++++++++ 2 files changed, 30 insertions(+) create mode 100644 pixl_core/tests/anon_queue/__init__.py create mode 100644 pixl_core/tests/patient_queue/__init__.py diff --git a/pixl_core/tests/anon_queue/__init__.py b/pixl_core/tests/anon_queue/__init__.py new file mode 100644 index 000000000..417e1ce01 --- /dev/null +++ b/pixl_core/tests/anon_queue/__init__.py @@ -0,0 +1,15 @@ +""" +# Copyright (c) University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" diff --git a/pixl_core/tests/patient_queue/__init__.py b/pixl_core/tests/patient_queue/__init__.py new file mode 100644 index 000000000..417e1ce01 --- /dev/null +++ b/pixl_core/tests/patient_queue/__init__.py @@ -0,0 +1,15 @@ +""" +# Copyright (c) University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +""" From 2dd0829c166aa6c29a7e9f3a168c149b59bb87e2 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 15:27:24 +0100 Subject: [PATCH 13/40] Renamed anon_queue test queues to fix name conflict --- pixl_core/tests/anon_queue/test_producer.py | 2 +- pixl_core/tests/anon_queue/test_subscriber.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/pixl_core/tests/anon_queue/test_producer.py b/pixl_core/tests/anon_queue/test_producer.py index 5070e1868..9c1d5a9f7 100644 --- a/pixl_core/tests/anon_queue/test_producer.py +++ b/pixl_core/tests/anon_queue/test_producer.py @@ -17,7 +17,7 @@ from core.anon_queue.producer import AnonymisationProducer -TEST_QUEUE = "test_publish" +TEST_QUEUE = "test_anon_publish" @pytest.mark.usefixtures("run_containers") diff --git a/pixl_core/tests/anon_queue/test_subscriber.py b/pixl_core/tests/anon_queue/test_subscriber.py index fcc65c2b9..f7ea69ae2 100644 --- a/pixl_core/tests/anon_queue/test_subscriber.py +++ b/pixl_core/tests/anon_queue/test_subscriber.py @@ -19,7 +19,7 @@ from core.anon_queue.subscriber import AnonymisationPixlConsumer -TEST_QUEUE = "test_consume" +TEST_QUEUE = "test_anon_consume" class ExpectedTestError(Exception): From 2f22ef9096fec55f90a8203395ab5b40dd5fc3a5 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 15:36:59 +0100 Subject: [PATCH 14/40] Add test to increase coverage including anonymisation in queue_names --- cli/tests/test_message_processing.py | 24 +++++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/cli/tests/test_message_processing.py b/cli/tests/test_message_processing.py index a0391cc60..352044166 100644 --- a/cli/tests/test_message_processing.py +++ b/cli/tests/test_message_processing.py @@ -21,7 +21,10 @@ import pytest from _pytest.monkeypatch import MonkeyPatch from core.patient_queue.producer import PixlProducer -from pixl_cli._message_processing import retry_until_export_count_is_unchanged +from pixl_cli._message_processing import ( + _message_count, + retry_until_export_count_is_unchanged, +) @pytest.fixture @@ -98,3 +101,22 @@ def test_retry_with_image_exported_and_no_change_multiple_projects( ) mock_publisher.assert_called_once() + + +def test_message_count_includes_anonymisation(mocker) -> None: + """Checks that the anonymisation queue is included when counting messages.""" + mock_rabbitmq = Mock() + mock_rabbitmq.message_count = 0 + + mock_interface = mocker.patch("pixl_cli._message_processing.PixlBlockingInterface") + mock_interface.return_value.__enter__.return_value = mock_rabbitmq + + _message_count(["imaging-primary"]) + + queue_names = {call.kwargs["queue_name"] for call in mock_interface.call_args_list} + + assert queue_names == { + "imaging-primary", + "imaging-secondary", + "anonymisation", + } From 1eb4a9dd05e8c8fe5035fb9887ebbaf7e5f24208 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 21 Aug 2026 15:53:23 +0100 Subject: [PATCH 15/40] Add test to cover new orthanc queue connection --- cli/tests/test_message_processing.py | 44 +++++++++++++++++++++++++++- 1 file changed, 43 insertions(+), 1 deletion(-) diff --git a/cli/tests/test_message_processing.py b/cli/tests/test_message_processing.py index 352044166..6478a8bb4 100644 --- a/cli/tests/test_message_processing.py +++ b/cli/tests/test_message_processing.py @@ -16,15 +16,17 @@ import os from collections.abc import Generator -from unittest.mock import Mock +from unittest.mock import AsyncMock, Mock import pytest from _pytest.monkeypatch import MonkeyPatch +from core.anon_queue.message import AnonymisationMessage from core.patient_queue.producer import PixlProducer from pixl_cli._message_processing import ( _message_count, retry_until_export_count_is_unchanged, ) +from pixl_imaging._orthanc import PIXLAnonOrthanc @pytest.fixture @@ -120,3 +122,43 @@ def test_message_count_includes_anonymisation(mocker) -> None: "imaging-secondary", "anonymisation", } + + +@pytest.mark.asyncio +async def test_notify_anon_publishes_anonymisation_message(monkeypatch) -> None: + """Checks that anonymisation requests are published to RabbitMQ.""" + orthanc_raw = AsyncMock() + orthanc_raw.get_local_study.side_effect = [ + {"MainDicomTags": {"StudyInstanceUID": "1.2.3"}}, + {"MainDicomTags": {"StudyInstanceUID": "4.5.6"}}, + ] + + producer = Mock() + producer_context = Mock() + producer_context.__enter__ = Mock(return_value=producer) + producer_context.__exit__ = Mock(return_value=None) + + monkeypatch.setattr( + "pixl_imaging._orthanc.AnonymisationProducer", + Mock(return_value=producer_context), + ) + + orthanc_anon = PIXLAnonOrthanc() + + await orthanc_anon.notify_anon_to_retrieve_study_resources( + orthanc_raw=orthanc_raw, + resource_ids=["resource-1", "resource-2"], + series_uid="1.2.3.1\\1.2.3.2", + project_name="test project", + ) + + producer.publish.assert_called_once_with( + [ + AnonymisationMessage( + resource_ids=["resource-1", "resource-2"], + series_uids=["1.2.3.1", "1.2.3.2"], + study_uids=["1.2.3", "4.5.6"], + project_name="test project", + ) + ] + ) From 6586344d824db2d628fec31006f0b8b697a1b108 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 9 Sep 2026 12:15:30 +0100 Subject: [PATCH 16/40] Combine 2 queues into core.queue --- pixl_core/src/core/anon_queue/__init__.py | 20 --- pixl_core/src/core/anon_queue/_base.py | 91 -------------- pixl_core/src/core/anon_queue/message.py | 65 ---------- pixl_core/src/core/anon_queue/producer.py | 86 ------------- pixl_core/src/core/anon_queue/subscriber.py | 117 ------------------ .../core/{patient_queue => queue}/__init__.py | 0 .../core/{patient_queue => queue}/_base.py | 40 ++++++ .../core/{patient_queue => queue}/message.py | 32 +++++ .../core/{patient_queue => queue}/producer.py | 63 +++++++++- .../{patient_queue => queue}/subscriber.py | 87 ++++++++++++- pixl_core/tests/conftest.py | 4 +- pixl_core/tests/patient_queue/test_message.py | 2 +- .../tests/patient_queue/test_producer.py | 2 +- .../tests/patient_queue/test_subscriber.py | 4 +- 14 files changed, 221 insertions(+), 392 deletions(-) delete mode 100644 pixl_core/src/core/anon_queue/__init__.py delete mode 100644 pixl_core/src/core/anon_queue/_base.py delete mode 100644 pixl_core/src/core/anon_queue/message.py delete mode 100644 pixl_core/src/core/anon_queue/producer.py delete mode 100644 pixl_core/src/core/anon_queue/subscriber.py rename pixl_core/src/core/{patient_queue => queue}/__init__.py (100%) rename pixl_core/src/core/{patient_queue => queue}/_base.py (70%) rename pixl_core/src/core/{patient_queue => queue}/message.py (69%) rename pixl_core/src/core/{patient_queue => queue}/producer.py (58%) rename pixl_core/src/core/{patient_queue => queue}/subscriber.py (61%) diff --git a/pixl_core/src/core/anon_queue/__init__.py b/pixl_core/src/core/anon_queue/__init__.py deleted file mode 100644 index 7b6fd6a6d..000000000 --- a/pixl_core/src/core/anon_queue/__init__.py +++ /dev/null @@ -1,20 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -"""RabbitMQ consumer for Pixl""" - -from __future__ import annotations - -from .subscriber import AnonymisationPixlConsumer - -__all__ = ["AnonymisationPixlConsumer"] diff --git a/pixl_core/src/core/anon_queue/_base.py b/pixl_core/src/core/anon_queue/_base.py deleted file mode 100644 index b5bcdf2ef..000000000 --- a/pixl_core/src/core/anon_queue/_base.py +++ /dev/null @@ -1,91 +0,0 @@ -# Copyright (c) University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -from __future__ import annotations - -import os -from typing import Any - -import pika -from loguru import logger - - -class PixlQueueInterface: - def __init__( - self, - queue_name: str, - host: str = "localhost", - port: int = 5672, - username: str = "guest", - password: str = "guest", # noqa: S107 - ) -> None: - """ - Generic RabbitMQ interface. Environment variables RABBITMQ_ take precedence - over arguments - - :param queue_name: Name of the queue this interfaces to. - :param host: Hostname of the RabbitMQ service. - :param port: Port on which RabbitMQ service is running. - :param username: RabbitMQ username as configured for queue. - :param password: RabbitMQ user password as configured for queue. - """ - self.queue_name = queue_name - - self._host = os.environ.get("RABBITMQ_HOST", default=host) - self._port = int(os.environ.get("RABBITMQ_PORT", default=port)) - self._username = os.environ.get("RABBITMQ_USERNAME", default=username) - self._password = os.environ.get("RABBITMQ_PASSWORD", default=password) - - self._connection: Any = None - self._channel: Any = None - self._queue: Any = None - - -class PixlBlockingInterface(PixlQueueInterface): - def __enter__(self) -> Any: - """Establishes connection to RabbitMQ service.""" - params = pika.ConnectionParameters( - host=self._host, - port=self._port, - credentials=pika.PlainCredentials(self._username, self._password), - ) - - if self._connection is None or self._connection.is_closed: - self._connection = pika.BlockingConnection(params) - - if self._channel is None or self._channel.is_closed: - self._channel = self._connection.channel() - self._queue = self._channel.queue_declare( - queue=self.queue_name, - durable=True, - ) - - logger.debug("Connected to {}", self.queue_name) - return self - - def __exit__(self, *args: object, **kwargs: Any) -> None: - """Shutdown the connection to RabbitMQ service.""" - self._channel.close() - self._connection.close() - - @property - def connection_open(self) -> bool: - return bool(self._connection.is_open) - - @property - def message_count(self) -> int: - try: - return int(self._queue.method.message_count) - except (ValueError, TypeError): - logger.exception("Failed to determine the number of messages. Returning 0") - return 0 diff --git a/pixl_core/src/core/anon_queue/message.py b/pixl_core/src/core/anon_queue/message.py deleted file mode 100644 index 1c8d0ce14..000000000 --- a/pixl_core/src/core/anon_queue/message.py +++ /dev/null @@ -1,65 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -"""Classes to represent messages in the patient queue.""" - -from __future__ import annotations - -from dataclasses import dataclass -from typing import Any - -from jsonpickle import decode, encode -from loguru import logger - - -@dataclass -class AnonymisationMessage: - """ - Representation of a RabbitMQ message containing the information - to identify an anonymisation request. - """ - - resource_ids: list[str] - study_uids: list[str] - series_uids: list[str] - project_name: str - - @property - def identifier(self) -> str: - """Identifier for message""" - return (f"Message({self.resource_ids=} {self.study_uids=} {self.series_uids=}").replace( - "self.", "" - ) - - def serialise(self, *, deserialisable: bool = True) -> bytes: - """ - Serialise the message into a JSON string and convert to bytes. - - :param deserialisable: If True, the serialised message will be deserialisable, by setting - the unpicklable flag to False in jsonpickle.encode(), meaning that the original Message - object can be recovered by `deserialise()`. If False, calling `deserialise()` on the - serialised message will return a dictionary. - """ - logger.trace("Serialising {}", self) - return str.encode(encode(self, unpicklable=deserialisable)) - - -def deserialise(serialised_msg: bytes) -> Any: - """ - Deserialise a message from a bytes-encoded JSON string. - If the message was serialised with `deserialisable=True`, the original Message object will be - returned. Otherwise, a dictionary will be returned. - - :param serialised_msg: The serialised message. - """ - return decode(serialised_msg) # noqa: S301, since we control the input, so no security risks diff --git a/pixl_core/src/core/anon_queue/producer.py b/pixl_core/src/core/anon_queue/producer.py deleted file mode 100644 index 31d0ec0b6..000000000 --- a/pixl_core/src/core/anon_queue/producer.py +++ /dev/null @@ -1,86 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -"""Producer for RabbitMQ""" - -from __future__ import annotations - -from typing import TYPE_CHECKING - -from loguru import logger -from opentelemetry import trace -from pika import BasicProperties, DeliveryMode - -from ._base import PixlBlockingInterface - -if TYPE_CHECKING: - from core.anon_queue.message import AnonymisationMessage - -tracer = trace.get_tracer("pixl_core.anon_queue.producer") - - -class AnonymisationProducer(PixlBlockingInterface): - """Anonymisation publisher for RabbitMQ""" - - def publish(self, messages: list[AnonymisationMessage]) -> None: - """ - Sends a list of serialised messages to a queue. - :param messages: list of messages to be sent to queue - """ - if len(messages) == 0: - logger.warning("List of messages is empty so nothing will be published to queue.") - return - - logger.info("Publishing {} messages to queue: {}", len(messages), self.queue_name) - for msg in messages: - attributes = { - "project_name": msg.project_name, - "resource_ids": msg.resource_ids, - "series_uids": msg.series_uids, - "study_uids": msg.study_uids, - } - with tracer.start_as_current_span("publish_message", attributes=attributes): - self._publish_message(msg) - - def _publish_message(self, message: AnonymisationMessage) -> None: - """ - Publish a single serialised message to a queue. - :param message: message to be sent to queue - """ - serialised_msg = message.serialise() - self._channel.basic_publish( - exchange="", - routing_key=self.queue_name, - body=serialised_msg, - properties=BasicProperties( - delivery_mode=DeliveryMode.Persistent, - ), - ) - - logger.bind( - project_name=message.project_name, - resource_id=message.resource_ids, - series_uid=message.series_uids, - study_uid=message.study_uids, - ).debug( - "AnonymisationMessage {} published to queue {}", - message, - self.queue_name, - ) - - def clear_queue(self) -> None: - """ - Triggering a purge of all the messages currently in the queue. Mainly used to - clean after tests. - """ - self._channel.queue_purge(queue=self.queue_name) diff --git a/pixl_core/src/core/anon_queue/subscriber.py b/pixl_core/src/core/anon_queue/subscriber.py deleted file mode 100644 index 16a4e2fdc..000000000 --- a/pixl_core/src/core/anon_queue/subscriber.py +++ /dev/null @@ -1,117 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -"""Subscriber for RabbitMQ""" - -from __future__ import annotations - -from typing import TYPE_CHECKING, Any - -import pika -from decouple import config - -from core.anon_queue._base import PixlQueueInterface -from core.anon_queue.message import deserialise -from core.exceptions import ( - PixlDiscardError, - PixlOutOfHoursError, - PixlRequeueMessageError, -) - -if TYPE_CHECKING: - from collections.abc import Awaitable, Callable - from typing import Self - - from core.anon_queue.message import AnonymisationMessage - -import time - -from loguru import logger - - -class AnonymisationPixlConsumer(PixlQueueInterface): - """Connector to RabbitMQ. Consumes messages from anonymisation queue""" - - def __init__( - self, - queue_name: str, - callback: Callable[[AnonymisationMessage], Awaitable[None]], - ) -> None: - """Creating connection to RabbitMQ queue""" - super().__init__(queue_name=queue_name) - self._callback = callback - - @property - def _url(self) -> str: - return f"amqp://{self._username}:{self._password}@{self._host}:{self._port}/" - - def __enter__(self) -> Self: - """Establishes connection to queue.""" - self._connection = pika.BlockingConnection(pika.URLParameters(self._url)) - self._channel = self._connection.channel() - # Set number of messages in flight - max_in_flight = config("PIXL_MAX_MESSAGES_IN_FLIGHT", cast=int) - logger.info("Pika will consume up to {} messages concurrently", max_in_flight) - self._channel.basic_qos(prefetch_count=max_in_flight) - self._queue = self._channel.queue_declare( - queue=self.queue_name, - durable=True, - ) - return self - - def _process_message(self, message: Any) -> None: - - pixl_message: AnonymisationMessage = deserialise(message.body) - logger.debug("Picked up from queue: {}", pixl_message.identifier) - try: - self._callback(pixl_message) - except PixlRequeueMessageError as requeue: - logger.trace("Requeue message: {} from {}", pixl_message.identifier, requeue) - time.sleep(1) - message.reject(requeue=True) - except PixlOutOfHoursError as nack_requeue: - logger.trace( - "Nack and requeue message: {} from {}", pixl_message.identifier, nack_requeue - ) - time.sleep(10) - message.nack(requeue=True) - except PixlDiscardError as exception: - logger.warning("Failed message {}: {}", pixl_message.identifier, exception) - (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin - except Exception: # noqa: BLE001 - logger.exception( - "Failed to process {}. Not re-queuing message", - pixl_message.identifier, - ) - (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin - else: - logger.success("Finished message {}", pixl_message.identifier) - message.ack() - - def run(self) -> None: - """Processes messages from queue.""" - self._channel.basic_consume( - queue=self.queue_name, - on_message_callback=self._process_message, - auto_ack=False, - ) - self._channel.start_consuming() - - def __exit__(self, *args: object, **kwargs: Any) -> None: - """Requirement for the context manager""" - if self._channel is not None and self._channel.is_open: - self._channel.close() - - if self._connection is not None and self._connection.is_open: - self._connection.close() diff --git a/pixl_core/src/core/patient_queue/__init__.py b/pixl_core/src/core/queue/__init__.py similarity index 100% rename from pixl_core/src/core/patient_queue/__init__.py rename to pixl_core/src/core/queue/__init__.py diff --git a/pixl_core/src/core/patient_queue/_base.py b/pixl_core/src/core/queue/_base.py similarity index 70% rename from pixl_core/src/core/patient_queue/_base.py rename to pixl_core/src/core/queue/_base.py index aa9c19922..9ca94de6b 100644 --- a/pixl_core/src/core/patient_queue/_base.py +++ b/pixl_core/src/core/queue/_base.py @@ -90,3 +90,43 @@ def message_count(self) -> int: except (ValueError, TypeError): logger.exception("Failed to determine the number of messages. Returning 0") return 0 + + +class PixlBlockingInterfaceAnon(PixlQueueInterface): + def __enter__(self) -> Any: + """Establishes connection to RabbitMQ service.""" + params = pika.ConnectionParameters( + host=self._host, + port=self._port, + credentials=pika.PlainCredentials(self._username, self._password), + ) + + if self._connection is None or self._connection.is_closed: + self._connection = pika.BlockingConnection(params) + + if self._channel is None or self._channel.is_closed: + self._channel = self._connection.channel() + self._queue = self._channel.queue_declare( + queue=self.queue_name, + durable=True, + ) + + logger.debug("Connected to {}", self.queue_name) + return self + + def __exit__(self, *args: object, **kwargs: Any) -> None: + """Shutdown the connection to RabbitMQ service.""" + self._channel.close() + self._connection.close() + + @property + def connection_open(self) -> bool: + return bool(self._connection.is_open) + + @property + def message_count(self) -> int: + try: + return int(self._queue.method.message_count) + except (ValueError, TypeError): + logger.exception("Failed to determine the number of messages. Returning 0") + return 0 diff --git a/pixl_core/src/core/patient_queue/message.py b/pixl_core/src/core/queue/message.py similarity index 69% rename from pixl_core/src/core/patient_queue/message.py rename to pixl_core/src/core/queue/message.py index d7ed2d8f6..b27f0c4cb 100644 --- a/pixl_core/src/core/patient_queue/message.py +++ b/pixl_core/src/core/queue/message.py @@ -59,6 +59,38 @@ def serialise(self, *, deserialisable: bool = True) -> bytes: return str.encode(encode(self, unpicklable=deserialisable)) +@dataclass +class AnonymisationMessage: + """ + Representation of a RabbitMQ message containing the information + to identify an anonymisation request. + """ + + resource_ids: list[str] + study_uids: list[str] + series_uids: list[str] + project_name: str + + @property + def identifier(self) -> str: + """Identifier for message""" + return (f"Message({self.resource_ids=} {self.study_uids=} {self.series_uids=}").replace( + "self.", "" + ) + + def serialise(self, *, deserialisable: bool = True) -> bytes: + """ + Serialise the message into a JSON string and convert to bytes. + + :param deserialisable: If True, the serialised message will be deserialisable, by setting + the unpicklable flag to False in jsonpickle.encode(), meaning that the original Message + object can be recovered by `deserialise()`. If False, calling `deserialise()` on the + serialised message will return a dictionary. + """ + logger.trace("Serialising {}", self) + return str.encode(encode(self, unpicklable=deserialisable)) + + def deserialise(serialised_msg: bytes) -> Any: """ Deserialise a message from a bytes-encoded JSON string. diff --git a/pixl_core/src/core/patient_queue/producer.py b/pixl_core/src/core/queue/producer.py similarity index 58% rename from pixl_core/src/core/patient_queue/producer.py rename to pixl_core/src/core/queue/producer.py index 78bf21c41..bb7013ae1 100644 --- a/pixl_core/src/core/patient_queue/producer.py +++ b/pixl_core/src/core/queue/producer.py @@ -21,12 +21,12 @@ from opentelemetry import trace from pika import BasicProperties, DeliveryMode -from ._base import PixlBlockingInterface +from ._base import PixlBlockingInterface, PixlBlockingInterfaceAnon if TYPE_CHECKING: - from core.patient_queue.message import Message + from core.queue.message import AnonymisationMessage, Message -tracer = trace.get_tracer("pixl_core.patient_queue.producer") +tracer = trace.get_tracer("pixl_core.queue.producer") class PixlProducer(PixlBlockingInterface): @@ -88,3 +88,60 @@ def clear_queue(self) -> None: clean after tests. """ self._channel.queue_purge(queue=self.queue_name) + + +class AnonymisationProducer(PixlBlockingInterfaceAnon): + """Anonymisation publisher for RabbitMQ""" + + def publish(self, messages: list[AnonymisationMessage]) -> None: + """ + Sends a list of serialised messages to a queue. + :param messages: list of messages to be sent to queue + """ + if len(messages) == 0: + logger.warning("List of messages is empty so nothing will be published to queue.") + return + + logger.info("Publishing {} messages to queue: {}", len(messages), self.queue_name) + for msg in messages: + attributes = { + "project_name": msg.project_name, + "resource_ids": msg.resource_ids, + "series_uids": msg.series_uids, + "study_uids": msg.study_uids, + } + with tracer.start_as_current_span("publish_message", attributes=attributes): + self._publish_message(msg) + + def _publish_message(self, message: AnonymisationMessage) -> None: + """ + Publish a single serialised message to a queue. + :param message: message to be sent to queue + """ + serialised_msg = message.serialise() + self._channel.basic_publish( + exchange="", + routing_key=self.queue_name, + body=serialised_msg, + properties=BasicProperties( + delivery_mode=DeliveryMode.Persistent, + ), + ) + + logger.bind( + project_name=message.project_name, + resource_id=message.resource_ids, + series_uid=message.series_uids, + study_uid=message.study_uids, + ).debug( + "AnonymisationMessage {} published to queue {}", + message, + self.queue_name, + ) + + def clear_queue(self) -> None: + """ + Triggering a purge of all the messages currently in the queue. Mainly used to + clean after tests. + """ + self._channel.queue_purge(queue=self.queue_name) diff --git a/pixl_core/src/core/patient_queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py similarity index 61% rename from pixl_core/src/core/patient_queue/subscriber.py rename to pixl_core/src/core/queue/subscriber.py index 62708efb9..310b82657 100644 --- a/pixl_core/src/core/patient_queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -17,9 +17,11 @@ from __future__ import annotations import asyncio +import time from typing import TYPE_CHECKING, Any import aio_pika +import pika from decouple import config from core.exceptions import ( @@ -28,9 +30,9 @@ PixlRequeueMessageError, PixlStudyNotInPrimaryArchiveError, ) -from core.patient_queue._base import PixlQueueInterface -from core.patient_queue.message import deserialise -from core.patient_queue.producer import PixlProducer +from core.queue._base import PixlQueueInterface +from core.queue.message import deserialise +from core.queue.producer import PixlProducer if TYPE_CHECKING: from collections.abc import Awaitable, Callable @@ -38,7 +40,7 @@ from aio_pika.abc import AbstractIncomingMessage - from core.patient_queue.message import Message + from core.queue.message import AnonymisationMessage, Message from core.token_buffer.tokens import TokenBucket from loguru import logger @@ -142,3 +144,80 @@ async def run(self) -> None: async def __aexit__(self, *args: object, **kwargs: Any) -> None: """Requirement for the asynchronous context manager""" + + +class AnonymisationPixlConsumer(PixlQueueInterface): + """Connector to RabbitMQ. Consumes messages from anonymisation queue""" + + def __init__( + self, + queue_name: str, + callback: Callable[[AnonymisationMessage], Awaitable[None]], + ) -> None: + """Creating connection to RabbitMQ queue""" + super().__init__(queue_name=queue_name) + self._callback = callback + + @property + def _url(self) -> str: + return f"amqp://{self._username}:{self._password}@{self._host}:{self._port}/" + + def __enter__(self) -> Self: + """Establishes connection to queue.""" + self._connection = pika.BlockingConnection(pika.URLParameters(self._url)) + self._channel = self._connection.channel() + # Set number of messages in flight + max_in_flight = config("PIXL_MAX_MESSAGES_IN_FLIGHT", cast=int) + logger.info("Pika will consume up to {} messages concurrently", max_in_flight) + self._channel.basic_qos(prefetch_count=max_in_flight) + self._queue = self._channel.queue_declare( + queue=self.queue_name, + durable=True, + ) + return self + + def _process_message(self, message: Any) -> None: + + pixl_message: AnonymisationMessage = deserialise(message.body) + logger.debug("Picked up from queue: {}", pixl_message.identifier) + try: + self._callback(pixl_message) + except PixlRequeueMessageError as requeue: + logger.trace("Requeue message: {} from {}", pixl_message.identifier, requeue) + time.sleep(1) + message.reject(requeue=True) + except PixlOutOfHoursError as nack_requeue: + logger.trace( + "Nack and requeue message: {} from {}", pixl_message.identifier, nack_requeue + ) + time.sleep(10) + message.nack(requeue=True) + except PixlDiscardError as exception: + logger.warning("Failed message {}: {}", pixl_message.identifier, exception) + (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin + except Exception: # noqa: BLE001 + logger.exception( + "Failed to process {}. Not re-queuing message", + pixl_message.identifier, + ) + (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin + else: + logger.success("Finished message {}", pixl_message.identifier) + message.ack() + + def run(self) -> None: + """Processes messages from queue.""" + self._channel.basic_consume( + queue=self.queue_name, + on_message_callback=self._process_message, + auto_ack=False, + ) + self._channel.start_consuming() + + def __exit__(self, *args: object, **kwargs: Any) -> None: + """Requirement for the context manager""" + if self._channel is not None and self._channel.is_open: + self._channel.close() + + if self._connection is not None and self._connection.is_open: + self._connection.close() diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index 903ad4972..0adb8b01d 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -22,6 +22,7 @@ import pytest import requests +from core.anon_queue.message import AnonymisationMessage from loguru import logger from opentelemetry.sdk._logs import LoggerProvider from opentelemetry.sdk._logs.export import ( @@ -34,10 +35,9 @@ from sqlalchemy import Engine, create_engine from sqlalchemy.orm import Session, sessionmaker -from core.anon_queue.message import AnonymisationMessage from core.db.models import Base, Extract, Image from core.logging import OTelSink -from core.patient_queue.message import Message +from core.queue.message import Message if TYPE_CHECKING: import subprocess diff --git a/pixl_core/tests/patient_queue/test_message.py b/pixl_core/tests/patient_queue/test_message.py index cb5b22c70..ea1141c70 100644 --- a/pixl_core/tests/patient_queue/test_message.py +++ b/pixl_core/tests/patient_queue/test_message.py @@ -13,7 +13,7 @@ # limitations under the License. from __future__ import annotations -from core.patient_queue.message import deserialise +from core.queue.message import deserialise def test_serialise(mock_message) -> None: diff --git a/pixl_core/tests/patient_queue/test_producer.py b/pixl_core/tests/patient_queue/test_producer.py index 619e951a2..7c91d4aef 100644 --- a/pixl_core/tests/patient_queue/test_producer.py +++ b/pixl_core/tests/patient_queue/test_producer.py @@ -15,7 +15,7 @@ import pytest -from core.patient_queue.producer import PixlProducer +from core.queue.producer import PixlProducer TEST_QUEUE = "test_publish" diff --git a/pixl_core/tests/patient_queue/test_subscriber.py b/pixl_core/tests/patient_queue/test_subscriber.py index f0b8f0e59..a23df3f5b 100644 --- a/pixl_core/tests/patient_queue/test_subscriber.py +++ b/pixl_core/tests/patient_queue/test_subscriber.py @@ -18,8 +18,8 @@ import pytest -from core.patient_queue.producer import PixlProducer -from core.patient_queue.subscriber import PixlConsumer +from core.queue.producer import PixlProducer +from core.queue.subscriber import PixlConsumer from core.token_buffer.tokens import TokenBucket TEST_QUEUE = "test_consume" From d5bbd004f1197e44c39f56b2d4f7595ee731a7e5 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 9 Sep 2026 12:16:17 +0100 Subject: [PATCH 17/40] Update imports to match new core.queue --- cli/src/pixl_cli/_message_processing.py | 6 +++--- cli/src/pixl_cli/main.py | 2 +- cli/tests/conftest.py | 4 ++-- cli/tests/test_message_processing.py | 2 +- cli/tests/test_messages_from_files.py | 2 +- cli/tests/test_populate.py | 4 ++-- pixl_imaging/README.md | 2 +- pixl_imaging/src/pixl_imaging/_processing.py | 2 +- pixl_imaging/src/pixl_imaging/main.py | 2 +- pixl_imaging/tests/test_imaging_processing.py | 2 +- 10 files changed, 14 insertions(+), 14 deletions(-) diff --git a/cli/src/pixl_cli/_message_processing.py b/cli/src/pixl_cli/_message_processing.py index 6377e783f..b57f22ef4 100644 --- a/cli/src/pixl_cli/_message_processing.py +++ b/cli/src/pixl_cli/_message_processing.py @@ -20,9 +20,9 @@ import pandas as pd import tqdm -from core.patient_queue._base import PixlBlockingInterface -from core.patient_queue.message import Message -from core.patient_queue.producer import PixlProducer +from core.queue._base import PixlBlockingInterface +from core.queue.message import Message +from core.queue.producer import PixlProducer from decouple import config from loguru import logger diff --git a/cli/src/pixl_cli/main.py b/cli/src/pixl_cli/main.py index 492cfe1b7..d0efb4bdd 100644 --- a/cli/src/pixl_cli/main.py +++ b/cli/src/pixl_cli/main.py @@ -23,7 +23,7 @@ import click import requests from core.exports import ParquetExport -from core.patient_queue.producer import PixlProducer +from core.queue.producer import PixlProducer from core.telemetry import configure_logging, configure_tracing, telemetry_is_enabled from decouple import RepositoryEnv, UndefinedValueError from loguru import logger diff --git a/cli/tests/conftest.py b/cli/tests/conftest.py index 582583c16..29e32a74b 100644 --- a/cli/tests/conftest.py +++ b/cli/tests/conftest.py @@ -24,8 +24,8 @@ import pandas as pd import pytest from core.db.models import Base, Extract, Image -from core.patient_queue.message import Message -from core.patient_queue.producer import PixlProducer +from core.queue.message import Message +from core.queue.producer import PixlProducer from sqlalchemy import Engine, create_engine from sqlalchemy.orm import Session, sessionmaker diff --git a/cli/tests/test_message_processing.py b/cli/tests/test_message_processing.py index 6478a8bb4..280c83b88 100644 --- a/cli/tests/test_message_processing.py +++ b/cli/tests/test_message_processing.py @@ -21,7 +21,7 @@ import pytest from _pytest.monkeypatch import MonkeyPatch from core.anon_queue.message import AnonymisationMessage -from core.patient_queue.producer import PixlProducer +from core.queue.producer import PixlProducer from pixl_cli._message_processing import ( _message_count, retry_until_export_count_is_unchanged, diff --git a/cli/tests/test_messages_from_files.py b/cli/tests/test_messages_from_files.py index 03ea8138b..0c0e45fce 100644 --- a/cli/tests/test_messages_from_files.py +++ b/cli/tests/test_messages_from_files.py @@ -20,7 +20,7 @@ import pytest from core.db.models import Image -from core.patient_queue.message import Message +from core.queue.message import Message from pixl_cli._io import read_patient_info from pixl_cli._message_processing import messages_from_df, populate_queue_and_db diff --git a/cli/tests/test_populate.py b/cli/tests/test_populate.py index 20a5e4d98..59576e4da 100644 --- a/cli/tests/test_populate.py +++ b/cli/tests/test_populate.py @@ -19,13 +19,13 @@ import pixl_cli._message_processing from click.testing import CliRunner -from core.patient_queue.producer import PixlProducer +from core.queue.producer import PixlProducer from pixl_cli.main import populate if TYPE_CHECKING: from pathlib import Path - from core.patient_queue.message import Message + from core.queue.message import Message class MockProducer(PixlProducer): diff --git a/pixl_imaging/README.md b/pixl_imaging/README.md index 9e8a09172..24371d0c4 100644 --- a/pixl_imaging/README.md +++ b/pixl_imaging/README.md @@ -9,7 +9,7 @@ The imaging API has two queues: - `imaging-secondary`, for querying PACS The imaging API uses RabbitMQ to expose a single HTTP endpoint that expects a JSON-formatted message structured as -defined by the [`Message`](../pixl_core/src/core/patient_queue/message.py) class in `pixl_core/patient_queue`. +defined by the [`Message`](../pixl_core/src/core/queue/message.py) class in `pixl_core/queue`. Users should send messages to the `imaging-primary` queue only. On arrival of the input message, the imaging API will query the VNA for the requested study. If the study does not exist in the VNA, the input message will be sent diff --git a/pixl_imaging/src/pixl_imaging/_processing.py b/pixl_imaging/src/pixl_imaging/_processing.py index 9ed26d022..2dd75e3ae 100644 --- a/pixl_imaging/src/pixl_imaging/_processing.py +++ b/pixl_imaging/src/pixl_imaging/_processing.py @@ -25,7 +25,7 @@ from pixl_imaging._orthanc import Orthanc, PIXLAnonOrthanc, PIXLRawOrthanc if TYPE_CHECKING: - from core.patient_queue.message import Message + from core.queue.message import Message from loguru import logger diff --git a/pixl_imaging/src/pixl_imaging/main.py b/pixl_imaging/src/pixl_imaging/main.py index f664a03eb..3c2fc1396 100644 --- a/pixl_imaging/src/pixl_imaging/main.py +++ b/pixl_imaging/src/pixl_imaging/main.py @@ -18,7 +18,7 @@ import asyncio import importlib.metadata -from core.patient_queue.subscriber import PixlConsumer +from core.queue.subscriber import PixlConsumer from core.rest_api.router import router, state from core.telemetry import configure_logging from decouple import config diff --git a/pixl_imaging/tests/test_imaging_processing.py b/pixl_imaging/tests/test_imaging_processing.py index 962fcd8dc..10dea4c7b 100644 --- a/pixl_imaging/tests/test_imaging_processing.py +++ b/pixl_imaging/tests/test_imaging_processing.py @@ -23,7 +23,7 @@ import pytest from core.exceptions import PixlDiscardError, PixlOutOfHoursError, PixlStudyNotInPrimaryArchiveError -from core.patient_queue.message import Message +from core.queue.message import Message from decouple import config from pydicom import dcmread from pydicom.data import get_testdata_file From d3e4b3f6ca5efbb1996caffaf9951eb74b150b0b Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 9 Sep 2026 12:21:11 +0100 Subject: [PATCH 18/40] Remove ImportStudiesFromRaw and API call --- orthanc/orthanc-anon/plugin/pixl.py | 32 ---------------------------- pixl_core/src/core/queue/__init__.py | 4 ++-- 2 files changed, 2 insertions(+), 34 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 0f28fb8d7..94260dda9 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -47,7 +47,6 @@ from opentelemetry import trace from opentelemetry.instrumentation.requests import RequestsInstrumentor from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor -from opentelemetry.propagate import extract from pixl_dcmd._database import engine as pixl_db_engine from pixl_dcmd._database import record_skip_reasons_for_study from pixl_dcmd.dicom_helpers import get_study_info @@ -233,36 +232,6 @@ def OnHeartBeat(output, uri, **request) -> Any: # noqa: ARG001 output.AnswerBuffer("OK\n", "text/plain") -def ImportStudiesFromRaw(output, uri, **request): # noqa: ARG001 - """ - Import studies from Orthanc Raw. - - Offload to a thread pool executor to avoid blocking the Orthanc main thread. - """ - payload = json.loads(request["body"]) - study_resource_ids = payload["ResourceIDs"] - study_uids = payload["StudyInstanceUIDs"] - series_to_keep = payload["SeriesInstanceUIDs"] - project_name = payload["ProjectName"] - - # Extract the trace context injected into the request headers by the caller, and pass it to - # the thread pool job so the import continues the same trace - headers = {key.lower(): value for key, value in request.get("headers", {}).items()} - parent_context = extract(headers) - - executor.submit( - _import_studies_from_raw, - study_resource_ids, - study_uids, - project_name, - series_to_keep, - parent_context, - ) - - response = json.dumps({"Message": "Ok"}) - output.AnswerBuffer(response, "application/json") - - def process_anonymisation_message(message: AnonymisationMessage) -> None: """ Import studies from Orthanc Raw. @@ -630,4 +599,3 @@ def notify_export_api_of_readiness(study_id: str, project_name: str) -> None: orthanc.RegisterOnChangeCallback(OnChange) orthanc.RegisterRestCallback("/heart-beat", OnHeartBeat) -orthanc.RegisterRestCallback("/import-from-raw", ImportStudiesFromRaw) diff --git a/pixl_core/src/core/queue/__init__.py b/pixl_core/src/core/queue/__init__.py index e477b448e..b4c2aa3a2 100644 --- a/pixl_core/src/core/queue/__init__.py +++ b/pixl_core/src/core/queue/__init__.py @@ -15,6 +15,6 @@ from __future__ import annotations -from .subscriber import PixlConsumer +from .subscriber import AnonymisationPixlConsumer, PixlConsumer -__all__ = ["PixlConsumer"] +__all__ = ["AnonymisationPixlConsumer", "PixlConsumer"] From 2e61304d25018da022a15a4e2990574f92d9a4c9 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 9 Sep 2026 15:45:52 +0100 Subject: [PATCH 19/40] Create models.py for ImagingRequest message and AnonymisationMessage --- pixl_core/src/core/queue/message.py | 34 +---------- pixl_core/src/core/queue/models.py | 60 ++++++++++++++++++++ pixl_core/src/core/queue/producer.py | 6 +- pixl_core/src/core/queue/subscriber.py | 6 +- pixl_imaging/src/pixl_imaging/_processing.py | 8 +-- 5 files changed, 71 insertions(+), 43 deletions(-) create mode 100644 pixl_core/src/core/queue/models.py diff --git a/pixl_core/src/core/queue/message.py b/pixl_core/src/core/queue/message.py index b27f0c4cb..916824901 100644 --- a/pixl_core/src/core/queue/message.py +++ b/pixl_core/src/core/queue/message.py @@ -28,7 +28,7 @@ @dataclass class Message: - """Representation of a RabbitMQ message containing the information to identify a DICOM study.""" + """Base class for a RabbitMQ message.""" mrn: str accession_number: str @@ -59,38 +59,6 @@ def serialise(self, *, deserialisable: bool = True) -> bytes: return str.encode(encode(self, unpicklable=deserialisable)) -@dataclass -class AnonymisationMessage: - """ - Representation of a RabbitMQ message containing the information - to identify an anonymisation request. - """ - - resource_ids: list[str] - study_uids: list[str] - series_uids: list[str] - project_name: str - - @property - def identifier(self) -> str: - """Identifier for message""" - return (f"Message({self.resource_ids=} {self.study_uids=} {self.series_uids=}").replace( - "self.", "" - ) - - def serialise(self, *, deserialisable: bool = True) -> bytes: - """ - Serialise the message into a JSON string and convert to bytes. - - :param deserialisable: If True, the serialised message will be deserialisable, by setting - the unpicklable flag to False in jsonpickle.encode(), meaning that the original Message - object can be recovered by `deserialise()`. If False, calling `deserialise()` on the - serialised message will return a dictionary. - """ - logger.trace("Serialising {}", self) - return str.encode(encode(self, unpicklable=deserialisable)) - - def deserialise(serialised_msg: bytes) -> Any: """ Deserialise a message from a bytes-encoded JSON string. diff --git a/pixl_core/src/core/queue/models.py b/pixl_core/src/core/queue/models.py new file mode 100644 index 000000000..0606b49c0 --- /dev/null +++ b/pixl_core/src/core/queue/models.py @@ -0,0 +1,60 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +"""Data classes to represent imaging and anonymisation messages in their respective queues.""" + +from __future__ import annotations + +from dataclasses import dataclass +from typing import TYPE_CHECKING + +if TYPE_CHECKING: + from datetime import date, datetime + + +@dataclass +class ImagingRequestMessage: + """Data containing the information to identify a DICOM study for an imaging request.""" + + mrn: str + accession_number: str + study_uid: str + series_uid: str + study_date: date + procedure_occurrence_id: int + project_name: str + extract_generated_timestamp: datetime + + @property + def identifier(self) -> str: + """Identifier for message""" + return ( + f"Message({self.mrn=} {self.accession_number=} {self.study_uid=} {self.series_uid=}" + ).replace("self.", "") + + +@dataclass +class AnonymisationMessage: + """Data containing the information to identify an anonymisation request.""" + + resource_ids: list[str] + study_uids: list[str] + series_uids: list[str] + project_name: str + + @property + def identifier(self) -> str: + """Identifier for message""" + return (f"Message({self.resource_ids=} {self.study_uids=} {self.series_uids=}").replace( + "self.", "" + ) diff --git a/pixl_core/src/core/queue/producer.py b/pixl_core/src/core/queue/producer.py index bb7013ae1..d36090d13 100644 --- a/pixl_core/src/core/queue/producer.py +++ b/pixl_core/src/core/queue/producer.py @@ -24,7 +24,7 @@ from ._base import PixlBlockingInterface, PixlBlockingInterfaceAnon if TYPE_CHECKING: - from core.queue.message import AnonymisationMessage, Message + from core.queue.modles import AnonymisationMessage, ImagingRequestMessage tracer = trace.get_tracer("pixl_core.queue.producer") @@ -32,7 +32,7 @@ class PixlProducer(PixlBlockingInterface): """Generic publisher for RabbitMQ""" - def publish(self, messages: list[Message], priority: int) -> None: + def publish(self, messages: list[ImagingRequestMessage], priority: int) -> None: """ Sends a list of serialised messages to a queue. :param messages: list of messages to be sent to queue @@ -53,7 +53,7 @@ def publish(self, messages: list[Message], priority: int) -> None: with tracer.start_as_current_span("publish_message", attributes=attributes): self._publish_message(msg, priority) - def _publish_message(self, message: Message, priority: int) -> None: + def _publish_message(self, message: ImagingRequestMessage, priority: int) -> None: """ Publish a single serialised message to a queue. :param message: message to be sent to queue diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index 310b82657..81b538c56 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -40,7 +40,7 @@ from aio_pika.abc import AbstractIncomingMessage - from core.queue.message import AnonymisationMessage, Message + from core.queue.models import AnonymisationMessage, ImagingRequestMessage from core.token_buffer.tokens import TokenBucket from loguru import logger @@ -54,7 +54,7 @@ def __init__( queue_name: str, token_bucket: TokenBucket, token_bucket_key: str, - callback: Callable[[Message], Awaitable[None]], + callback: Callable[[ImagingRequestMessage], Awaitable[None]], ) -> None: """ Creating connection to RabbitMQ queue @@ -90,7 +90,7 @@ async def _process_message(self, message: AbstractIncomingMessage) -> None: await message.reject(requeue=True) return - pixl_message: Message = deserialise(message.body) + pixl_message: ImagingRequestMessage = deserialise(message.body) logger.debug("Picked up from queue: {}", pixl_message.identifier) try: await self._callback(pixl_message) diff --git a/pixl_imaging/src/pixl_imaging/_processing.py b/pixl_imaging/src/pixl_imaging/_processing.py index 2dd75e3ae..0d768e665 100644 --- a/pixl_imaging/src/pixl_imaging/_processing.py +++ b/pixl_imaging/src/pixl_imaging/_processing.py @@ -25,7 +25,7 @@ from pixl_imaging._orthanc import Orthanc, PIXLAnonOrthanc, PIXLRawOrthanc if TYPE_CHECKING: - from core.queue.message import Message + from core.queue.models import ImagingRequestMessage from loguru import logger @@ -35,7 +35,7 @@ class DicomModality(StrEnum): secondary = config("SECONDARY_DICOM_SOURCE_MODALITY") -async def process_message(message: Message, archive: DicomModality) -> None: +async def process_message(message: ImagingRequestMessage, archive: DicomModality) -> None: """ Process message from queue by retrieving a study with the given Patient and Accession Number. We may receive multiple messages with same Patient + Acc Num, either as retries or because @@ -391,10 +391,10 @@ async def _get_missing_instances( class ImagingStudy: """Dataclass for DICOM study unique to a patient and imaging study""" - message: Message + message: ImagingRequestMessage @classmethod - def from_message(cls, message: Message) -> ImagingStudy: + def from_message(cls, message: ImagingRequestMessage) -> ImagingStudy: """Build an imaging study from a queue message.""" return ImagingStudy(message=message) From 93c608f9ad4317be42fc2c546869f44c5d61f1f3 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 9 Sep 2026 16:39:09 +0100 Subject: [PATCH 20/40] Add parent_context back to process_anon_message --- orthanc/orthanc-anon/plugin/pixl.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 94260dda9..838c91127 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -47,6 +47,7 @@ from opentelemetry import trace from opentelemetry.instrumentation.requests import RequestsInstrumentor from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor +from opentelemetry.propagate import extract from pixl_dcmd._database import engine as pixl_db_engine from pixl_dcmd._database import record_skip_reasons_for_study from pixl_dcmd.dicom_helpers import get_study_info @@ -238,11 +239,16 @@ def process_anonymisation_message(message: AnonymisationMessage) -> None: Offload to a thread pool executor to avoid blocking the Orthanc main thread. """ + # Extract the trace context injected into the request headers by the caller, and pass it to + # the thread pool job so the import continues the same trace + headers = {key.lower(): value for key, value in requests.request.get("headers", {}).items()} + parent_context = extract(headers) data = { "resource_ids": message.resource_ids, "series_uids": message.series_uids, "study_uids": message.study_uids, "project_name": message.project_name, + "parent_context": parent_context, } executor.submit(_import_studies_from_raw, data) From 5c0d0e1ef5465a87f273b81c2f989a8c54d3ce90 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 9 Sep 2026 16:52:40 +0100 Subject: [PATCH 21/40] Combine 2 queues testing into 1 test suite --- pixl_core/tests/anon_queue/test_message.py | 33 ---------- pixl_core/tests/anon_queue/test_producer.py | 41 ------------ pixl_core/tests/anon_queue/test_subscriber.py | 65 ------------------- pixl_core/tests/patient_queue/__init__.py | 15 ----- .../tests/{anon_queue => queue}/__init__.py | 0 .../{patient_queue => queue}/test_message.py | 15 ++++- .../{patient_queue => queue}/test_producer.py | 24 ++++++- .../test_subscriber.py | 44 ++++++++++++- 8 files changed, 78 insertions(+), 159 deletions(-) delete mode 100644 pixl_core/tests/anon_queue/test_message.py delete mode 100644 pixl_core/tests/anon_queue/test_producer.py delete mode 100644 pixl_core/tests/anon_queue/test_subscriber.py delete mode 100644 pixl_core/tests/patient_queue/__init__.py rename pixl_core/tests/{anon_queue => queue}/__init__.py (100%) rename pixl_core/tests/{patient_queue => queue}/test_message.py (70%) rename pixl_core/tests/{patient_queue => queue}/test_producer.py (62%) rename pixl_core/tests/{patient_queue => queue}/test_subscriber.py (61%) diff --git a/pixl_core/tests/anon_queue/test_message.py b/pixl_core/tests/anon_queue/test_message.py deleted file mode 100644 index 6db99e6fe..000000000 --- a/pixl_core/tests/anon_queue/test_message.py +++ /dev/null @@ -1,33 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -from __future__ import annotations - -from core.anon_queue.message import deserialise - - -def test_serialise(mock_anon_message) -> None: - """Checks that messages can be correctly serialised""" - msg_body = mock_anon_message.serialise(deserialisable=False) - assert ( - msg_body == b'{"resource_ids": ["resource-1", "resource-2"], ' - b'"study_uids": ["1.2.3", "4.5.6"], ' - b'"series_uids": ["1.2.3.1", "1.2.3.2"], ' - b'"project_name": "test project"}' - ) - - -def test_deserialise(mock_message) -> None: - """Checks if deserialised messages are the same as the original""" - serialised_msg = mock_message.serialise() - assert deserialise(serialised_msg) == mock_message diff --git a/pixl_core/tests/anon_queue/test_producer.py b/pixl_core/tests/anon_queue/test_producer.py deleted file mode 100644 index 9c1d5a9f7..000000000 --- a/pixl_core/tests/anon_queue/test_producer.py +++ /dev/null @@ -1,41 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -from __future__ import annotations - -import pytest - -from core.anon_queue.producer import AnonymisationProducer - -TEST_QUEUE = "test_anon_publish" - - -@pytest.mark.usefixtures("run_containers") -def test_create_pixl_producer() -> None: - """Checks that AnonymisationProducer can be instantiated.""" - with AnonymisationProducer(queue_name=TEST_QUEUE) as pp: - assert pp.connection_open - - -@pytest.mark.usefixtures("run_containers") -def test_publish(mock_anon_message) -> None: - """ - Checks that after publishing, there is one message in the queue. - Will only work if nothing has been added to queue before. - """ - with AnonymisationProducer(queue_name=TEST_QUEUE) as pp: - pp.clear_queue() - pp.publish(messages=[mock_anon_message]) - - with AnonymisationProducer(queue_name=TEST_QUEUE) as pp: - assert pp.message_count == 1 diff --git a/pixl_core/tests/anon_queue/test_subscriber.py b/pixl_core/tests/anon_queue/test_subscriber.py deleted file mode 100644 index f7ea69ae2..000000000 --- a/pixl_core/tests/anon_queue/test_subscriber.py +++ /dev/null @@ -1,65 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -from __future__ import annotations - -from unittest.mock import Mock - -import pytest - -from core.anon_queue.subscriber import AnonymisationPixlConsumer - -TEST_QUEUE = "test_anon_consume" - - -class ExpectedTestError(Exception): - """Expected error for testing.""" - - -@pytest.mark.usefixtures("run_containers") -def test_run() -> None: - """Checks that the consumer starts consuming messages.""" - callback = Mock() - - with AnonymisationPixlConsumer( - queue_name=TEST_QUEUE, - callback=callback, - ) as consumer: - consumer._channel.basic_consume = Mock() - consumer._channel.start_consuming = Mock() - - consumer.run() - - consumer._channel.basic_consume.assert_called_once_with( - queue=TEST_QUEUE, - on_message_callback=consumer._process_message, - auto_ack=False, - ) - consumer._channel.start_consuming.assert_called_once() - - -@pytest.mark.usefixtures("run_containers") -def test_process_message(mock_anon_message) -> None: - """Checks that a received message is passed to the callback.""" - callback = Mock() - - with AnonymisationPixlConsumer( - queue_name=TEST_QUEUE, - callback=callback, - ) as consumer: - message = Mock() - message.body = mock_anon_message.serialise() - - consumer._process_message(message) - - callback.assert_called_once_with(mock_anon_message) diff --git a/pixl_core/tests/patient_queue/__init__.py b/pixl_core/tests/patient_queue/__init__.py deleted file mode 100644 index 417e1ce01..000000000 --- a/pixl_core/tests/patient_queue/__init__.py +++ /dev/null @@ -1,15 +0,0 @@ -""" -# Copyright (c) University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -""" diff --git a/pixl_core/tests/anon_queue/__init__.py b/pixl_core/tests/queue/__init__.py similarity index 100% rename from pixl_core/tests/anon_queue/__init__.py rename to pixl_core/tests/queue/__init__.py diff --git a/pixl_core/tests/patient_queue/test_message.py b/pixl_core/tests/queue/test_message.py similarity index 70% rename from pixl_core/tests/patient_queue/test_message.py rename to pixl_core/tests/queue/test_message.py index ea1141c70..d0ac1868c 100644 --- a/pixl_core/tests/patient_queue/test_message.py +++ b/pixl_core/tests/queue/test_message.py @@ -16,8 +16,8 @@ from core.queue.message import deserialise -def test_serialise(mock_message) -> None: - """Checks that messages can be correctly serialised""" +def test_serialise_imagingrequests(mock_message) -> None: + """Checks that imaging request messages can be correctly serialised""" msg_body = mock_message.serialise(deserialisable=False) assert ( msg_body == b'{"mrn": "111", "accession_number": "123", "study_uid": "1.2.3", ' @@ -29,6 +29,17 @@ def test_serialise(mock_message) -> None: ) +def test_serialise_anon(mock_anon_message) -> None: + """Checks that anon messages can be correctly serialised""" + msg_body = mock_anon_message.serialise(deserialisable=False) + assert ( + msg_body == b'{"resource_ids": ["resource-1", "resource-2"], ' + b'"study_uids": ["1.2.3", "4.5.6"], ' + b'"series_uids": ["1.2.3.1", "1.2.3.2"], ' + b'"project_name": "test project"}' + ) + + def test_deserialise(mock_message) -> None: """Checks if deserialised messages are the same as the original""" serialised_msg = mock_message.serialise() diff --git a/pixl_core/tests/patient_queue/test_producer.py b/pixl_core/tests/queue/test_producer.py similarity index 62% rename from pixl_core/tests/patient_queue/test_producer.py rename to pixl_core/tests/queue/test_producer.py index 7c91d4aef..c80d170ff 100644 --- a/pixl_core/tests/patient_queue/test_producer.py +++ b/pixl_core/tests/queue/test_producer.py @@ -15,9 +15,10 @@ import pytest -from core.queue.producer import PixlProducer +from core.queue.producer import AnonymisationProducer, PixlProducer TEST_QUEUE = "test_publish" +TEST_QUEUE_ANON = "test_anon_publish" @pytest.mark.usefixtures("run_containers") @@ -39,3 +40,24 @@ def test_publish(mock_message) -> None: with PixlProducer(queue_name=TEST_QUEUE) as pp: assert pp.message_count == 1 + + +@pytest.mark.usefixtures("run_containers") +def test_create_pixl_producer_anon() -> None: + """Checks that AnonymisationProducer can be instantiated.""" + with AnonymisationProducer(queue_name=TEST_QUEUE_ANON) as pp: + assert pp.connection_open + + +@pytest.mark.usefixtures("run_containers") +def test_publish_anon(mock_anon_message) -> None: + """ + Checks that after publishing, there is one message in the queue. + Will only work if nothing has been added to queue before. + """ + with AnonymisationProducer(queue_name=TEST_QUEUE_ANON) as pp: + pp.clear_queue() + pp.publish(messages=[mock_anon_message]) + + with AnonymisationProducer(queue_name=TEST_QUEUE_ANON) as pp: + assert pp.message_count == 1 diff --git a/pixl_core/tests/patient_queue/test_subscriber.py b/pixl_core/tests/queue/test_subscriber.py similarity index 61% rename from pixl_core/tests/patient_queue/test_subscriber.py rename to pixl_core/tests/queue/test_subscriber.py index a23df3f5b..9d940dc2a 100644 --- a/pixl_core/tests/patient_queue/test_subscriber.py +++ b/pixl_core/tests/queue/test_subscriber.py @@ -14,15 +14,16 @@ from __future__ import annotations import asyncio -from unittest.mock import AsyncMock +from unittest.mock import AsyncMock, Mock import pytest from core.queue.producer import PixlProducer -from core.queue.subscriber import PixlConsumer +from core.queue.subscriber import AnonymisationPixlConsumer, PixlConsumer from core.token_buffer.tokens import TokenBucket TEST_QUEUE = "test_consume" +TEST_QUEUE_ANON = "test_anon_consume" class ExpectedTestError(Exception): @@ -58,3 +59,42 @@ async def test_create(mock_message) -> None: consume.assert_called_once() # Fail on purpose to check async test awaited raise ExpectedTestError + + +@pytest.mark.usefixtures("run_containers") +def test_run() -> None: + """Checks that the consumer starts consuming messages.""" + callback = Mock() + + with AnonymisationPixlConsumer( + queue_name=TEST_QUEUE, + callback=callback, + ) as consumer: + consumer._channel.basic_consume = Mock() + consumer._channel.start_consuming = Mock() + + consumer.run() + + consumer._channel.basic_consume.assert_called_once_with( + queue=TEST_QUEUE, + on_message_callback=consumer._process_message, + auto_ack=False, + ) + consumer._channel.start_consuming.assert_called_once() + + +@pytest.mark.usefixtures("run_containers") +def test_process_message(mock_anon_message) -> None: + """Checks that a received message is passed to the callback.""" + callback = Mock() + + with AnonymisationPixlConsumer( + queue_name=TEST_QUEUE, + callback=callback, + ) as consumer: + message = Mock() + message.body = mock_anon_message.serialise() + + consumer._process_message(message) + + callback.assert_called_once_with(mock_anon_message) From 82e161f42e5c03ef4fc5ea6fe50bbadff4145ebd Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Thu, 10 Sep 2026 14:28:46 +0100 Subject: [PATCH 22/40] Remove redundant anon_queue refs --- cli/tests/test_message_processing.py | 2 +- orthanc/orthanc-anon/plugin/pixl.py | 4 ++-- pixl_core/tests/conftest.py | 2 +- pixl_imaging/src/pixl_imaging/_orthanc.py | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/cli/tests/test_message_processing.py b/cli/tests/test_message_processing.py index 280c83b88..ef56b2479 100644 --- a/cli/tests/test_message_processing.py +++ b/cli/tests/test_message_processing.py @@ -20,7 +20,7 @@ import pytest from _pytest.monkeypatch import MonkeyPatch -from core.anon_queue.message import AnonymisationMessage +from core.queue.models import AnonymisationMessage from core.queue.producer import PixlProducer from pixl_cli._message_processing import ( _message_count, diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 838c91127..857b4a0da 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -34,13 +34,13 @@ import pydicom import requests -from core.anon_queue.subscriber import AnonymisationPixlConsumer from core.exceptions import PixlDiscardError, PixlSkipInstanceError from core.metrics import ( record_instance_deidentification_failure, record_study_deidentification_failure, ) from core.project_config.pixl_config_model import load_project_config +from core.queue.subscriber import AnonymisationPixlConsumer from core.telemetry import configure_logging, configure_metrics, configure_tracing from decouple import config from loguru import logger @@ -65,8 +65,8 @@ if TYPE_CHECKING: from typing import Any - from core.anon_queue.message import AnonymisationMessage from core.project_config.pixl_config_model import PixlConfig + from core.queue.models import AnonymisationMessage from opentelemetry.context import Context from pixl_dcmd.dicom_helpers import StudyInfo diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index 0adb8b01d..95e2d470e 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -22,7 +22,6 @@ import pytest import requests -from core.anon_queue.message import AnonymisationMessage from loguru import logger from opentelemetry.sdk._logs import LoggerProvider from opentelemetry.sdk._logs.export import ( @@ -38,6 +37,7 @@ from core.db.models import Base, Extract, Image from core.logging import OTelSink from core.queue.message import Message +from core.queue.models import AnonymisationMessage if TYPE_CHECKING: import subprocess diff --git a/pixl_imaging/src/pixl_imaging/_orthanc.py b/pixl_imaging/src/pixl_imaging/_orthanc.py index 0f9229a35..9f208fc87 100644 --- a/pixl_imaging/src/pixl_imaging/_orthanc.py +++ b/pixl_imaging/src/pixl_imaging/_orthanc.py @@ -18,9 +18,9 @@ from typing import Any import aiohttp -from core.anon_queue.message import AnonymisationMessage -from core.anon_queue.producer import AnonymisationProducer from core.exceptions import PixlDiscardError, PixlRequeueMessageError +from core.queue.models import AnonymisationMessage +from core.queue.producer import AnonymisationProducer from decouple import config from loguru import logger From 36c0e6c7c74051f267edbee2b6c608ca5a4ae129 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Thu, 10 Sep 2026 15:35:54 +0100 Subject: [PATCH 23/40] Ensure pixl_core tests pass locally --- cli/src/pixl_cli/_message_processing.py | 8 ++--- pixl_core/src/core/queue/message.py | 43 ++---------------------- pixl_core/src/core/queue/models.py | 28 +++++++++++++++ pixl_core/tests/conftest.py | 7 ++-- pixl_core/tests/queue/__init__.py | 15 --------- pixl_core/tests/queue/test_message.py | 30 ++++------------- pixl_core/tests/queue/test_models.py | 38 +++++++++++++++++++++ pixl_core/tests/queue/test_subscriber.py | 10 +++--- 8 files changed, 86 insertions(+), 93 deletions(-) delete mode 100644 pixl_core/tests/queue/__init__.py create mode 100644 pixl_core/tests/queue/test_models.py diff --git a/cli/src/pixl_cli/_message_processing.py b/cli/src/pixl_cli/_message_processing.py index b57f22ef4..c31418766 100644 --- a/cli/src/pixl_cli/_message_processing.py +++ b/cli/src/pixl_cli/_message_processing.py @@ -21,7 +21,7 @@ import pandas as pd import tqdm from core.queue._base import PixlBlockingInterface -from core.queue.message import Message +from core.queue.models import ImagingRequestMessage from core.queue.producer import PixlProducer from decouple import config from loguru import logger @@ -35,7 +35,7 @@ def messages_from_df( df: pd.DataFrame, -) -> list[Message]: +) -> list[ImagingRequestMessage]: """ Reads patient information from a DataFrame and transforms that into messages. @@ -43,7 +43,7 @@ def messages_from_df( """ messages = [] for _, row in df.iterrows(): - message = Message( + message = ImagingRequestMessage( mrn=row["mrn"], accession_number=row["accession_number"], study_uid=row["study_uid"], @@ -141,7 +141,7 @@ def _message_count(queues_to_populate: list[str]) -> int: def populate_queue_and_db( queues: list[str], messages_df: pd.DataFrame, messages_priority: int -) -> list[Message]: +) -> list[ImagingRequestMessage]: """ Populate queues with messages, for imaging queue update the database and filter out exported or skipped studies. diff --git a/pixl_core/src/core/queue/message.py b/pixl_core/src/core/queue/message.py index 916824901..51a8f5965 100644 --- a/pixl_core/src/core/queue/message.py +++ b/pixl_core/src/core/queue/message.py @@ -15,48 +15,9 @@ from __future__ import annotations -from dataclasses import dataclass -from typing import TYPE_CHECKING, Any +from typing import Any -from jsonpickle import decode, encode - -if TYPE_CHECKING: - from datetime import date, datetime - -from loguru import logger - - -@dataclass -class Message: - """Base class for a RabbitMQ message.""" - - mrn: str - accession_number: str - study_uid: str - series_uid: str - study_date: date - procedure_occurrence_id: int - project_name: str - extract_generated_timestamp: datetime - - @property - def identifier(self) -> str: - """Identifier for message""" - return ( - f"Message({self.mrn=} {self.accession_number=} {self.study_uid=} {self.series_uid=}" - ).replace("self.", "") - - def serialise(self, *, deserialisable: bool = True) -> bytes: - """ - Serialise the message into a JSON string and convert to bytes. - - :param deserialisable: If True, the serialised message will be deserialisable, by setting - the unpicklable flag to False in jsonpickle.encode(), meaning that the original Message - object can be recovered by `deserialise()`. If False, calling `deserialise()` on the - serialised message will return a dictionary. - """ - logger.trace("Serialising {}", self) - return str.encode(encode(self, unpicklable=deserialisable)) +from jsonpickle import decode def deserialise(serialised_msg: bytes) -> Any: diff --git a/pixl_core/src/core/queue/models.py b/pixl_core/src/core/queue/models.py index 0606b49c0..05dd1981c 100644 --- a/pixl_core/src/core/queue/models.py +++ b/pixl_core/src/core/queue/models.py @@ -18,9 +18,13 @@ from dataclasses import dataclass from typing import TYPE_CHECKING +from jsonpickle import encode + if TYPE_CHECKING: from datetime import date, datetime +from loguru import logger + @dataclass class ImagingRequestMessage: @@ -42,6 +46,18 @@ def identifier(self) -> str: f"Message({self.mrn=} {self.accession_number=} {self.study_uid=} {self.series_uid=}" ).replace("self.", "") + def serialise(self, *, deserialisable: bool = True) -> bytes: + """ + Serialise the message into a JSON string and convert to bytes. + + :param deserialisable: If True, the serialised message will be deserialisable, by setting + the unpicklable flag to False in jsonpickle.encode(), meaning that the original Message + object can be recovered by `deserialise()`. If False, calling `deserialise()` on the + serialised message will return a dictionary. + """ + logger.trace("Serialising {}", self) + return str.encode(encode(self, unpicklable=deserialisable)) + @dataclass class AnonymisationMessage: @@ -58,3 +74,15 @@ def identifier(self) -> str: return (f"Message({self.resource_ids=} {self.study_uids=} {self.series_uids=}").replace( "self.", "" ) + + def serialise(self, *, deserialisable: bool = True) -> bytes: + """ + Serialise the message into a JSON string and convert to bytes. + + :param deserialisable: If True, the serialised message will be deserialisable, by setting + the unpicklable flag to False in jsonpickle.encode(), meaning that the original Message + object can be recovered by `deserialise()`. If False, calling `deserialise()` on the + serialised message will return a dictionary. + """ + logger.trace("Serialising {}", self) + return str.encode(encode(self, unpicklable=deserialisable)) diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index 95e2d470e..d867c021f 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -36,8 +36,7 @@ from core.db.models import Base, Extract, Image from core.logging import OTelSink -from core.queue.message import Message -from core.queue.models import AnonymisationMessage +from core.queue.models import AnonymisationMessage, ImagingRequestMessage if TYPE_CHECKING: import subprocess @@ -221,9 +220,9 @@ def export_dir(tmp_path_factory: pytest.TempPathFactory) -> pathlib.Path: @pytest.fixture -def mock_message() -> Message: +def mock_message() -> ImagingRequestMessage: """An example Message used for testing""" - return Message( + return ImagingRequestMessage( mrn="111", accession_number="123", study_uid="1.2.3", diff --git a/pixl_core/tests/queue/__init__.py b/pixl_core/tests/queue/__init__.py deleted file mode 100644 index 417e1ce01..000000000 --- a/pixl_core/tests/queue/__init__.py +++ /dev/null @@ -1,15 +0,0 @@ -""" -# Copyright (c) University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -""" diff --git a/pixl_core/tests/queue/test_message.py b/pixl_core/tests/queue/test_message.py index d0ac1868c..1a5544829 100644 --- a/pixl_core/tests/queue/test_message.py +++ b/pixl_core/tests/queue/test_message.py @@ -16,31 +16,13 @@ from core.queue.message import deserialise -def test_serialise_imagingrequests(mock_message) -> None: - """Checks that imaging request messages can be correctly serialised""" - msg_body = mock_message.serialise(deserialisable=False) - assert ( - msg_body == b'{"mrn": "111", "accession_number": "123", "study_uid": "1.2.3", ' - b'"series_uid": "", ' - b'"study_date": "2022-11-22", ' - b'"procedure_occurrence_id": "234", ' - b'"project_name": "test project", ' - b'"extract_generated_timestamp": "2023-12-07T14:08:00+00:00"}' - ) - - -def test_serialise_anon(mock_anon_message) -> None: - """Checks that anon messages can be correctly serialised""" - msg_body = mock_anon_message.serialise(deserialisable=False) - assert ( - msg_body == b'{"resource_ids": ["resource-1", "resource-2"], ' - b'"study_uids": ["1.2.3", "4.5.6"], ' - b'"series_uids": ["1.2.3.1", "1.2.3.2"], ' - b'"project_name": "test project"}' - ) - - def test_deserialise(mock_message) -> None: """Checks if deserialised messages are the same as the original""" serialised_msg = mock_message.serialise() assert deserialise(serialised_msg) == mock_message + + +def test_deserialise_anon(mock_anon_message) -> None: + """Checks if deserialised anon messages are the same as the original""" + serialised_msg = mock_anon_message.serialise() + assert deserialise(serialised_msg) == mock_anon_message diff --git a/pixl_core/tests/queue/test_models.py b/pixl_core/tests/queue/test_models.py new file mode 100644 index 000000000..cc52bd9ec --- /dev/null +++ b/pixl_core/tests/queue/test_models.py @@ -0,0 +1,38 @@ +# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +from __future__ import annotations + + +def test_serialise_imagingrequests(mock_message) -> None: + """Checks that imaging request messages can be correctly serialised""" + msg_body = mock_message.serialise(deserialisable=False) + assert ( + msg_body == b'{"mrn": "111", "accession_number": "123", "study_uid": "1.2.3", ' + b'"series_uid": "", ' + b'"study_date": "2022-11-22", ' + b'"procedure_occurrence_id": "234", ' + b'"project_name": "test project", ' + b'"extract_generated_timestamp": "2023-12-07T14:08:00+00:00"}' + ) + + +def test_serialise_anon(mock_anon_message) -> None: + """Checks that anon messages can be correctly serialised""" + msg_body = mock_anon_message.serialise(deserialisable=False) + assert ( + msg_body == b'{"resource_ids": ["resource-1", "resource-2"], ' + b'"study_uids": ["1.2.3", "4.5.6"], ' + b'"series_uids": ["1.2.3.1", "1.2.3.2"], ' + b'"project_name": "test project"}' + ) diff --git a/pixl_core/tests/queue/test_subscriber.py b/pixl_core/tests/queue/test_subscriber.py index 9d940dc2a..3d6fd390a 100644 --- a/pixl_core/tests/queue/test_subscriber.py +++ b/pixl_core/tests/queue/test_subscriber.py @@ -62,12 +62,12 @@ async def test_create(mock_message) -> None: @pytest.mark.usefixtures("run_containers") -def test_run() -> None: +def test_run_anon() -> None: """Checks that the consumer starts consuming messages.""" callback = Mock() with AnonymisationPixlConsumer( - queue_name=TEST_QUEUE, + queue_name=TEST_QUEUE_ANON, callback=callback, ) as consumer: consumer._channel.basic_consume = Mock() @@ -76,7 +76,7 @@ def test_run() -> None: consumer.run() consumer._channel.basic_consume.assert_called_once_with( - queue=TEST_QUEUE, + queue=TEST_QUEUE_ANON, on_message_callback=consumer._process_message, auto_ack=False, ) @@ -84,12 +84,12 @@ def test_run() -> None: @pytest.mark.usefixtures("run_containers") -def test_process_message(mock_anon_message) -> None: +def test_process_message_anon(mock_anon_message) -> None: """Checks that a received message is passed to the callback.""" callback = Mock() with AnonymisationPixlConsumer( - queue_name=TEST_QUEUE, + queue_name=TEST_QUEUE_ANON, callback=callback, ) as consumer: message = Mock() From 8312af5d221b1e6f3f1d09fdc851f5df1fc8e456 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Thu, 10 Sep 2026 15:50:36 +0100 Subject: [PATCH 24/40] Update tests that need Message replace w/ ImagingRequestMessage --- cli/tests/conftest.py | 10 +-- cli/tests/test_messages_from_files.py | 28 +++---- cli/tests/test_populate.py | 4 +- pixl_imaging/tests/test_imaging_processing.py | 76 +++++++++++-------- 4 files changed, 64 insertions(+), 54 deletions(-) diff --git a/cli/tests/conftest.py b/cli/tests/conftest.py index 29e32a74b..9281d9c85 100644 --- a/cli/tests/conftest.py +++ b/cli/tests/conftest.py @@ -24,7 +24,7 @@ import pandas as pd import pytest from core.db.models import Base, Extract, Image -from core.queue.message import Message +from core.queue.models import ImagingRequestMessage from core.queue.producer import PixlProducer from sqlalchemy import Engine, create_engine from sqlalchemy.orm import Session, sessionmaker @@ -136,8 +136,8 @@ def _make_message( accession_number: str, mrn: str, study_uid: str, -) -> Message: - return Message( +) -> ImagingRequestMessage: + return ImagingRequestMessage( project_name=project_name, accession_number=accession_number, mrn=mrn, @@ -150,7 +150,7 @@ def _make_message( @pytest.fixture -def example_messages() -> list[Message]: +def example_messages() -> list[ImagingRequestMessage]: """Test input data.""" return [ _make_message( @@ -174,7 +174,7 @@ def example_messages_df(example_messages): @pytest.fixture -def example_messages_multiple_projects() -> list[Message]: +def example_messages_multiple_projects() -> list[ImagingRequestMessage]: """Test input data.""" return [ _make_message( diff --git a/cli/tests/test_messages_from_files.py b/cli/tests/test_messages_from_files.py index 0c0e45fce..18128ae47 100644 --- a/cli/tests/test_messages_from_files.py +++ b/cli/tests/test_messages_from_files.py @@ -20,7 +20,7 @@ import pytest from core.db.models import Image -from core.queue.message import Message +from core.queue.models import ImagingRequestMessage from pixl_cli._io import read_patient_info from pixl_cli._message_processing import messages_from_df, populate_queue_and_db @@ -40,10 +40,10 @@ def test_messages_from_csv(omop_resources: Path) -> None: # Act messages = messages_from_df(messages_df) # Assert - assert all(isinstance(msg, Message) for msg in messages) + assert all(isinstance(msg, ImagingRequestMessage) for msg in messages) expected_messages = [ - Message( + ImagingRequestMessage( procedure_occurrence_id=0, mrn="patient_identifier", accession_number="123456789", @@ -71,7 +71,7 @@ def test_whitespace_and_na_processing(omop_resources: Path) -> None: messages = messages_from_df(messages_df) # Assert assert messages == [ - Message( + ImagingRequestMessage( procedure_occurrence_id=0, mrn="patient_identifier", accession_number="123456789", @@ -117,10 +117,10 @@ def test_messages_from_parquet(omop_resources: Path) -> None: # Act messages = messages_from_df(messages_df) # Assert - assert all(isinstance(msg, Message) for msg in messages) + assert all(isinstance(msg, ImagingRequestMessage) for msg in messages) expected_messages = [ - Message( + ImagingRequestMessage( mrn="987654321", accession_number="AA12345601", study_uid="1.3.6.1.4.1.14519.5.2.1.99.1071.12985477682660597455732044031486", @@ -130,7 +130,7 @@ def test_messages_from_parquet(omop_resources: Path) -> None: project_name="test-extract-uclh-omop-cdm", extract_generated_timestamp=datetime.datetime.fromisoformat("2023-12-07T14:08:58"), ), - Message( + ImagingRequestMessage( mrn="987654321", accession_number="AA12345605", study_uid="1.2.276.0.7230010.3.1.2.929116473.1.1710754859.579485", @@ -157,10 +157,10 @@ def test_messages_from_batched_parquet(omop_resources: Path) -> None: # Act messages = messages_from_df(messages_df) # Assert - assert all(isinstance(msg, Message) for msg in messages) + assert all(isinstance(msg, ImagingRequestMessage) for msg in messages) expected_messages = [ - Message( + ImagingRequestMessage( mrn="5020765", accession_number="MIG0234560", study_uid="1.2.840.114350.2.525.2.798268.2.110000014.1", @@ -170,7 +170,7 @@ def test_messages_from_batched_parquet(omop_resources: Path) -> None: project_name="test-extract-uclh-omop-cdm", extract_generated_timestamp=datetime.datetime.fromisoformat("2023-12-07T14:08:58"), ), - Message( + ImagingRequestMessage( mrn="987654321", accession_number="ABC1234560", study_uid="1.2.840.114350.2.525.2.798268.2.190000013.1", @@ -180,7 +180,7 @@ def test_messages_from_batched_parquet(omop_resources: Path) -> None: project_name="test-extract-uclh-omop-cdm", extract_generated_timestamp=datetime.datetime.fromisoformat("2023-12-07T14:08:58"), ), - Message( + ImagingRequestMessage( mrn="987654321", accession_number="AA12345601", study_uid="1.2.840.114350.2.525.2.798268.2.190000015.1", @@ -190,7 +190,7 @@ def test_messages_from_batched_parquet(omop_resources: Path) -> None: project_name="test-extract-uclh-omop-cdm", extract_generated_timestamp=datetime.datetime.fromisoformat("2023-12-07T14:08:58"), ), - Message( + ImagingRequestMessage( mrn="987654321", accession_number="AA12345605", study_uid="1.2.840.114350.2.525.2.798268.2.190000016.1", @@ -200,7 +200,7 @@ def test_messages_from_batched_parquet(omop_resources: Path) -> None: project_name="test-extract-uclh-omop-cdm", extract_generated_timestamp=datetime.datetime.fromisoformat("2023-12-07T14:08:58"), ), - Message( + ImagingRequestMessage( mrn="12345678", accession_number="12345678", study_uid="1.2.840.114350.2.525.2.798268.2.190000011.1", @@ -210,7 +210,7 @@ def test_messages_from_batched_parquet(omop_resources: Path) -> None: project_name="test-extract-uclh-omop-cdm", extract_generated_timestamp=datetime.datetime.fromisoformat("2023-12-07T14:08:58"), ), - Message( + ImagingRequestMessage( mrn="12345678", accession_number="ABC1234567", study_uid="1.2.840.114350.2.525.2.798268.2.190000012.1", diff --git a/cli/tests/test_populate.py b/cli/tests/test_populate.py index 59576e4da..0d9c9bf29 100644 --- a/cli/tests/test_populate.py +++ b/cli/tests/test_populate.py @@ -25,7 +25,7 @@ if TYPE_CHECKING: from pathlib import Path - from core.queue.message import Message + from core.queue.models import ImagingRequestMessage class MockProducer(PixlProducer): @@ -39,7 +39,7 @@ def __exit__(self, *args: object, **kwargs) -> None: """Context exit point.""" return - def publish(self, messages: list[Message], priority: int) -> None: # noqa: ARG002 don't access messages or priority + def publish(self, messages: list[ImagingRequestMessage], priority: int) -> None: # noqa: ARG002 don't access messages or priority """Dummy method for publish.""" return diff --git a/pixl_imaging/tests/test_imaging_processing.py b/pixl_imaging/tests/test_imaging_processing.py index 10dea4c7b..c20e30ac9 100644 --- a/pixl_imaging/tests/test_imaging_processing.py +++ b/pixl_imaging/tests/test_imaging_processing.py @@ -23,7 +23,7 @@ import pytest from core.exceptions import PixlDiscardError, PixlOutOfHoursError, PixlStudyNotInPrimaryArchiveError -from core.queue.message import Message +from core.patient_queue.models import ImagingRequestMessage from decouple import config from pydicom import dcmread from pydicom.data import get_testdata_file @@ -58,9 +58,9 @@ @pytest.fixture(scope="module") -def message() -> Message: - """A Message with a valid study_uid.""" - return Message( +def message() -> ImagingRequestMessage: + """A ImagingRequestMessage with a valid study_uid.""" + return ImagingRequestMessage( mrn=PATIENT_ID, accession_number=ACCESSION_NUMBER, study_uid=STUDY_UID, @@ -75,9 +75,9 @@ def message() -> Message: @pytest.fixture(scope="module") -def message_with_series_uids() -> Message: - """A Message for querying a subset of series within a study.""" - return Message( +def message_with_series_uids() -> ImagingRequestMessage: + """A ImagingRequestMessage for querying a subset of series within a study.""" + return ImagingRequestMessage( mrn=PATIENT_ID, accession_number=ACCESSION_NUMBER, study_uid=STUDY_UID, @@ -92,9 +92,9 @@ def message_with_series_uids() -> Message: @pytest.fixture(scope="module") -def message_with_single_series_uid() -> Message: - """A Message for querying a subset of series within a study.""" - return Message( +def message_with_single_series_uid() -> ImagingRequestMessage: + """A ImagingRequestMessage for querying a subset of series within a study.""" + return ImagingRequestMessage( mrn=PATIENT_ID, accession_number=ACCESSION_NUMBER, study_uid=STUDY_UID, @@ -109,9 +109,9 @@ def message_with_single_series_uid() -> Message: @pytest.fixture(scope="module") -def no_uid_message() -> Message: - """A Message with a valid study_uid.""" - return Message( +def no_uid_message() -> ImagingRequestMessage: + """A ImagingRequestMessage with a valid study_uid.""" + return ImagingRequestMessage( mrn=PATIENT_ID, accession_number=ACCESSION_NUMBER, study_uid="", @@ -126,9 +126,12 @@ def no_uid_message() -> Message: @pytest.fixture(scope="module") -def pacs_message() -> Message: - """A Message with a valid study_uid for a study that exists in PACS but not VNA.""" - return Message( +def pacs_message() -> ImagingRequestMessage: + """ + A ImagingRequestMessage with a valid study_uid for a study + that exists in PACS but not VNA. + """ + return ImagingRequestMessage( mrn=PACS_PATIENT_ID, accession_number=PACS_ACCESSION_NUMBER, study_uid=PACS_STUDY_UID, @@ -143,9 +146,12 @@ def pacs_message() -> Message: @pytest.fixture(scope="module") -def pacs_no_uid_message() -> Message: - """A Message without a valid study_uid for a study that exists in PACS but not the VNA.""" - return Message( +def pacs_no_uid_message() -> ImagingRequestMessage: + """ + A ImagingRequestMessage without a valid study_uid for a study + that exists in PACS but not the VNA. + """ + return ImagingRequestMessage( mrn=PACS_PATIENT_ID, accession_number=PACS_ACCESSION_NUMBER, study_uid="ialsodontexist", @@ -160,9 +166,9 @@ def pacs_no_uid_message() -> Message: @pytest.fixture(scope="module") -def missing_message() -> Message: - """A Message for a study that does not exist in PACS nor the VNA.""" - return Message( +def missing_message() -> ImagingRequestMessage: + """A ImagingRequestMessage for a study that does not exist in PACS nor the VNA.""" + return ImagingRequestMessage( mrn=MISSING_PATIENT_ID, accession_number=MISSING_ACCESSION_NUMBER, study_uid=MISSING_STUDY_UID, @@ -272,7 +278,7 @@ async def orthanc_raw(run_containers) -> PIXLRawOrthanc: @pytest.mark.processing @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_vna") -async def test_image_saved(orthanc_raw, message: Message) -> None: +async def test_image_saved(orthanc_raw, message: ImagingRequestMessage) -> None: """ Given the VNA has images, and orthanc raw has no images When we run process_message @@ -313,7 +319,7 @@ async def test_image_saved(orthanc_raw, message: Message) -> None: @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_vna") async def test_message_with_series_uids( - orthanc_raw, message_with_series_uids: Message, caplog + orthanc_raw, message_with_series_uids: ImagingRequestMessage, caplog ) -> None: """ Given the VNA has a single study with 2 series, and Orthanc Raw has no images @@ -350,7 +356,7 @@ async def test_message_with_series_uids( @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_vna") async def test_message_with_one_series_uid( - orthanc_raw, message_with_single_series_uid: Message, caplog + orthanc_raw, message_with_single_series_uid: ImagingRequestMessage, caplog ) -> None: """ Given the VNA has a single study with 2 series, and Orthanc Raw has no images @@ -382,7 +388,7 @@ async def test_message_with_one_series_uid( @pytest.mark.processing @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_vna") -async def test_partial_retrieve(orthanc_raw, message: Message, caplog) -> None: +async def test_partial_retrieve(orthanc_raw, message: ImagingRequestMessage, caplog) -> None: """ Given the VNA has a single study with 2 instances, and orthanc raw has the same study with 1 instance @@ -424,7 +430,7 @@ async def test_partial_retrieve(orthanc_raw, message: Message, caplog) -> None: @pytest.mark.processing @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_vna") -async def test_existing_message_sent_twice(orthanc_raw, message: Message) -> None: +async def test_existing_message_sent_twice(orthanc_raw, message: ImagingRequestMessage) -> None: """ Given the VNA has images, and orthanc raw has no images When we run process_message on the same message twice @@ -474,7 +480,9 @@ async def test_existing_message_sent_twice(orthanc_raw, message: Message) -> Non @pytest.mark.processing @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_vna") -async def test_querying_without_uid(orthanc_raw, caplog, no_uid_message: Message) -> None: +async def test_querying_without_uid( + orthanc_raw, caplog, no_uid_message: ImagingRequestMessage +) -> None: """ Given a message with non-existent study_uid When we query the VNA @@ -516,7 +524,7 @@ def now(cls, tz=None) -> datetime.datetime: @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_pacs") async def test_querying_pacs_with_uid( - orthanc_raw, caplog, monkeypatch, pacs_message: Message + orthanc_raw, caplog, monkeypatch, pacs_message: ImagingRequestMessage ) -> None: """ Given a message with study_uid exists in PACS but not VNA, @@ -556,7 +564,7 @@ async def test_querying_pacs_with_uid( @pytest.mark.asyncio @pytest.mark.usefixtures("_add_image_to_fake_pacs") async def test_querying_pacs_without_uid( - orthanc_raw, caplog, monkeypatch, pacs_no_uid_message: Message + orthanc_raw, caplog, monkeypatch, pacs_no_uid_message: ImagingRequestMessage ) -> None: """ Given a message with non-existent study_uid exists in PACS but not VNA, @@ -591,7 +599,9 @@ async def test_querying_pacs_without_uid( @pytest.mark.processing @pytest.mark.asyncio -async def test_querying_missing_image(orthanc_raw, monkeypatch, missing_message: Message) -> None: +async def test_querying_missing_image( + orthanc_raw, monkeypatch, missing_message: ImagingRequestMessage +) -> None: """ Given a message for a study that is missing in both the VNA and PACS, When we query the archives within the window of Monday-Friday 8pm to 8am, @@ -626,7 +636,7 @@ async def test_querying_missing_image(orthanc_raw, monkeypatch, missing_message: ], ) async def test_querying_pacs_during_working_hours( - orthanc_raw, query_date, monkeypatch, missing_message: Message + orthanc_raw, query_date, monkeypatch, missing_message: ImagingRequestMessage ) -> None: """ Given a message for a study that is missing in both the VNA and PACS, @@ -647,7 +657,7 @@ async def test_querying_pacs_during_working_hours( @pytest.mark.processing @pytest.mark.asyncio async def test_querying_pacs_not_defined( - orthanc_raw, monkeypatch, missing_message: Message + orthanc_raw, monkeypatch, missing_message: ImagingRequestMessage ) -> None: """ Given a message for a study that is missing in the VNA and the SECONDARY_DICOM_SOURCE_AE_TITLE From 4b7ce8f347cb8b6d091b6d721904a54a73b40a05 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Thu, 10 Sep 2026 15:56:06 +0100 Subject: [PATCH 25/40] Update tests that need Message replace w/ ImagingRequestMessage --- pixl_imaging/tests/test_imaging_processing.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_imaging/tests/test_imaging_processing.py b/pixl_imaging/tests/test_imaging_processing.py index c20e30ac9..e77cb006c 100644 --- a/pixl_imaging/tests/test_imaging_processing.py +++ b/pixl_imaging/tests/test_imaging_processing.py @@ -23,7 +23,7 @@ import pytest from core.exceptions import PixlDiscardError, PixlOutOfHoursError, PixlStudyNotInPrimaryArchiveError -from core.patient_queue.models import ImagingRequestMessage +from core.queue.models import ImagingRequestMessage from decouple import config from pydicom import dcmread from pydicom.data import get_testdata_file From 942a15fbf097bd19e30153740bbf001ece2673e4 Mon Sep 17 00:00:00 2001 From: ruaridhg <32329546+ruaridhg@users.noreply.github.com> Date: Wed, 16 Sep 2026 12:20:27 +0100 Subject: [PATCH 26/40] Update pixl_core/src/core/queue/subscriber.py Co-authored-by: Stef Piatek --- pixl_core/src/core/queue/subscriber.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index 81b538c56..e56c19712 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -90,7 +90,7 @@ async def _process_message(self, message: AbstractIncomingMessage) -> None: await message.reject(requeue=True) return - pixl_message: ImagingRequestMessage = deserialise(message.body) + pixl_message: ImagingRequestMessage | AnonymisationMessage = deserialise(message.body) logger.debug("Picked up from queue: {}", pixl_message.identifier) try: await self._callback(pixl_message) From 24e118c9997762ae3f30c81ccc49512f45dfc8ee Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 16 Sep 2026 12:56:13 +0100 Subject: [PATCH 27/40] Address PR review comments - reduce redundancy --- pixl_core/src/core/queue/_base.py | 61 ++++++++++---------------- pixl_core/src/core/queue/message.py | 31 ------------- pixl_core/src/core/queue/models.py | 17 +++++-- pixl_core/src/core/queue/producer.py | 4 +- pixl_core/src/core/queue/subscriber.py | 11 +++-- pixl_imaging/src/pixl_imaging/main.py | 5 ++- 6 files changed, 46 insertions(+), 83 deletions(-) delete mode 100644 pixl_core/src/core/queue/message.py diff --git a/pixl_core/src/core/queue/_base.py b/pixl_core/src/core/queue/_base.py index 9ca94de6b..20a217f81 100644 --- a/pixl_core/src/core/queue/_base.py +++ b/pixl_core/src/core/queue/_base.py @@ -52,47 +52,26 @@ def __init__( class PixlBlockingInterface(PixlQueueInterface): - def __enter__(self) -> Any: - """Establishes connection to RabbitMQ service.""" - params = pika.ConnectionParameters( - host=self._host, - port=self._port, - credentials=pika.PlainCredentials(self._username, self._password), - ) - - if self._connection is None or self._connection.is_closed: - self._connection = pika.BlockingConnection(params) - - if self._channel is None or self._channel.is_closed: - self._channel = self._connection.channel() - self._queue = self._channel.queue_declare( - queue=self.queue_name, - durable=True, - arguments={"x-max-priority": 5}, - ) - - logger.debug("Connected to {}", self.queue_name) - return self - - def __exit__(self, *args: object, **kwargs: Any) -> None: - """Shutdown the connection to RabbitMQ service.""" - self._channel.close() - self._connection.close() - - @property - def connection_open(self) -> bool: - return bool(self._connection.is_open) - - @property - def message_count(self) -> int: - try: - return int(self._queue.method.message_count) - except (ValueError, TypeError): - logger.exception("Failed to determine the number of messages. Returning 0") - return 0 + def __init__( # noqa: PLR0913 + self, + queue_name: str, + host: str = "localhost", + port: int = 5672, + username: str = "guest", + password: str = "guest", # noqa: S107 + max_priority: int | None = None, + ) -> None: + """ + RabbitMQ interface using a blocking connection. + :param max_priority: If set, declares the queue as a priority queue with this + maximum priority. Must match the value used wherever else this queue is + declared, since RabbitMQ rejects redeclaring an existing queue with + different arguments. + """ + super().__init__(queue_name, host, port, username, password) + self._max_priority = max_priority -class PixlBlockingInterfaceAnon(PixlQueueInterface): def __enter__(self) -> Any: """Establishes connection to RabbitMQ service.""" params = pika.ConnectionParameters( @@ -106,9 +85,13 @@ def __enter__(self) -> Any: if self._channel is None or self._channel.is_closed: self._channel = self._connection.channel() + arguments = ( + {"x-max-priority": self._max_priority} if self._max_priority is not None else None + ) self._queue = self._channel.queue_declare( queue=self.queue_name, durable=True, + arguments=arguments, ) logger.debug("Connected to {}", self.queue_name) diff --git a/pixl_core/src/core/queue/message.py b/pixl_core/src/core/queue/message.py deleted file mode 100644 index 51a8f5965..000000000 --- a/pixl_core/src/core/queue/message.py +++ /dev/null @@ -1,31 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -"""Classes to represent messages in the patient queue.""" - -from __future__ import annotations - -from typing import Any - -from jsonpickle import decode - - -def deserialise(serialised_msg: bytes) -> Any: - """ - Deserialise a message from a bytes-encoded JSON string. - If the message was serialised with `deserialisable=True`, the original Message object will be - returned. Otherwise, a dictionary will be returned. - - :param serialised_msg: The serialised message. - """ - return decode(serialised_msg) # noqa: S301, since we control the input, so no security risks diff --git a/pixl_core/src/core/queue/models.py b/pixl_core/src/core/queue/models.py index 05dd1981c..cb81b36b7 100644 --- a/pixl_core/src/core/queue/models.py +++ b/pixl_core/src/core/queue/models.py @@ -11,14 +11,14 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. -"""Data classes to represent imaging and anonymisation messages in their respective queues.""" +"""Classes to represent imaging and anonymisation messages in their respective queues.""" from __future__ import annotations from dataclasses import dataclass -from typing import TYPE_CHECKING +from typing import TYPE_CHECKING, Any -from jsonpickle import encode +from jsonpickle import decode, encode if TYPE_CHECKING: from datetime import date, datetime @@ -86,3 +86,14 @@ def serialise(self, *, deserialisable: bool = True) -> bytes: """ logger.trace("Serialising {}", self) return str.encode(encode(self, unpicklable=deserialisable)) + + +def deserialise(serialised_msg: bytes) -> Any: + """ + Deserialise a message from a bytes-encoded JSON string. + If the message was serialised with `deserialisable=True`, the original Message object will be + returned. Otherwise, a dictionary will be returned. + + :param serialised_msg: The serialised message. + """ + return decode(serialised_msg) # noqa: S301, since we control the input, so no security risks diff --git a/pixl_core/src/core/queue/producer.py b/pixl_core/src/core/queue/producer.py index d36090d13..171e51e96 100644 --- a/pixl_core/src/core/queue/producer.py +++ b/pixl_core/src/core/queue/producer.py @@ -21,7 +21,7 @@ from opentelemetry import trace from pika import BasicProperties, DeliveryMode -from ._base import PixlBlockingInterface, PixlBlockingInterfaceAnon +from ._base import PixlBlockingInterface if TYPE_CHECKING: from core.queue.modles import AnonymisationMessage, ImagingRequestMessage @@ -90,7 +90,7 @@ def clear_queue(self) -> None: self._channel.queue_purge(queue=self.queue_name) -class AnonymisationProducer(PixlBlockingInterfaceAnon): +class AnonymisationProducer(PixlBlockingInterface): """Anonymisation publisher for RabbitMQ""" def publish(self, messages: list[AnonymisationMessage]) -> None: diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index e56c19712..82989cc63 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -31,7 +31,7 @@ PixlStudyNotInPrimaryArchiveError, ) from core.queue._base import PixlQueueInterface -from core.queue.message import deserialise +from core.queue.models import AnonymisationMessage, ImagingRequestMessage, deserialise from core.queue.producer import PixlProducer if TYPE_CHECKING: @@ -40,13 +40,12 @@ from aio_pika.abc import AbstractIncomingMessage - from core.queue.models import AnonymisationMessage, ImagingRequestMessage from core.token_buffer.tokens import TokenBucket from loguru import logger -class PixlConsumer(PixlQueueInterface): +class PixlConsumer[PixlMessage: (ImagingRequestMessage, AnonymisationMessage)](PixlQueueInterface): """Connector to RabbitMQ. Consumes messages from a queue""" def __init__( @@ -54,7 +53,7 @@ def __init__( queue_name: str, token_bucket: TokenBucket, token_bucket_key: str, - callback: Callable[[ImagingRequestMessage], Awaitable[None]], + callback: Callable[[PixlMessage], Awaitable[None]], ) -> None: """ Creating connection to RabbitMQ queue @@ -63,7 +62,7 @@ def __init__( super().__init__(queue_name=queue_name) self.token_bucket = token_bucket self.token_bucket_key = token_bucket_key - self._callback = callback + self._callback: Callable[[PixlMessage], Awaitable[None]] = callback @property def _url(self) -> str: @@ -90,7 +89,7 @@ async def _process_message(self, message: AbstractIncomingMessage) -> None: await message.reject(requeue=True) return - pixl_message: ImagingRequestMessage | AnonymisationMessage = deserialise(message.body) + pixl_message: PixlMessage = deserialise(message.body) logger.debug("Picked up from queue: {}", pixl_message.identifier) try: await self._callback(pixl_message) diff --git a/pixl_imaging/src/pixl_imaging/main.py b/pixl_imaging/src/pixl_imaging/main.py index 3c2fc1396..cee0448db 100644 --- a/pixl_imaging/src/pixl_imaging/main.py +++ b/pixl_imaging/src/pixl_imaging/main.py @@ -18,6 +18,7 @@ import asyncio import importlib.metadata +from core.queue.models import ImagingRequestMessage from core.queue.subscriber import PixlConsumer from core.rest_api.router import router, state from core.telemetry import configure_logging @@ -55,13 +56,13 @@ async def startup_event() -> None: """ background_tasks = set() async with ( - PixlConsumer( + PixlConsumer[ImagingRequestMessage]( QUEUE_NAME, token_bucket=state.token_bucket, token_bucket_key="primary", # noqa: S106 callback=lambda message: process_message(message, archive=DicomModality.primary), ) as primary_consumer, - PixlConsumer( + PixlConsumer[ImagingRequestMessage]( SECONDARY_QUEUE_NAME, token_bucket=state.token_bucket, token_bucket_key="secondary", # noqa: S106 From c145c6b08721b1542f47f02cca6462bfaba8478f Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 16 Sep 2026 13:15:20 +0100 Subject: [PATCH 28/40] Rename tests --- pixl_core/tests/queue/test_message.py | 28 --------------------------- pixl_core/tests/queue/test_models.py | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 28 deletions(-) delete mode 100644 pixl_core/tests/queue/test_message.py diff --git a/pixl_core/tests/queue/test_message.py b/pixl_core/tests/queue/test_message.py deleted file mode 100644 index 1a5544829..000000000 --- a/pixl_core/tests/queue/test_message.py +++ /dev/null @@ -1,28 +0,0 @@ -# Copyright (c) 2022 University College London Hospitals NHS Foundation Trust -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. -from __future__ import annotations - -from core.queue.message import deserialise - - -def test_deserialise(mock_message) -> None: - """Checks if deserialised messages are the same as the original""" - serialised_msg = mock_message.serialise() - assert deserialise(serialised_msg) == mock_message - - -def test_deserialise_anon(mock_anon_message) -> None: - """Checks if deserialised anon messages are the same as the original""" - serialised_msg = mock_anon_message.serialise() - assert deserialise(serialised_msg) == mock_anon_message diff --git a/pixl_core/tests/queue/test_models.py b/pixl_core/tests/queue/test_models.py index cc52bd9ec..32737fd2d 100644 --- a/pixl_core/tests/queue/test_models.py +++ b/pixl_core/tests/queue/test_models.py @@ -13,6 +13,8 @@ # limitations under the License. from __future__ import annotations +from core.queue.models import deserialise + def test_serialise_imagingrequests(mock_message) -> None: """Checks that imaging request messages can be correctly serialised""" @@ -36,3 +38,15 @@ def test_serialise_anon(mock_anon_message) -> None: b'"series_uids": ["1.2.3.1", "1.2.3.2"], ' b'"project_name": "test project"}' ) + + +def test_deserialise(mock_message) -> None: + """Checks if deserialised messages are the same as the original""" + serialised_msg = mock_message.serialise() + assert deserialise(serialised_msg) == mock_message + + +def test_deserialise_anon(mock_anon_message) -> None: + """Checks if deserialised anon messages are the same as the original""" + serialised_msg = mock_anon_message.serialise() + assert deserialise(serialised_msg) == mock_anon_message From fe71ad3641f41924f6e2e0e619d01a03d0dbd805 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 16 Sep 2026 13:25:53 +0100 Subject: [PATCH 29/40] Fix unit tests --- pixl_core/tests/queue/test_subscriber.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_core/tests/queue/test_subscriber.py b/pixl_core/tests/queue/test_subscriber.py index 3d6fd390a..5420d228a 100644 --- a/pixl_core/tests/queue/test_subscriber.py +++ b/pixl_core/tests/queue/test_subscriber.py @@ -37,7 +37,7 @@ class ExpectedTestError(Exception): ) async def test_create(mock_message) -> None: """Checks consume is working.""" - with PixlProducer(queue_name=TEST_QUEUE) as producer: + with PixlProducer(queue_name=TEST_QUEUE, max_priority=5) as producer: producer.publish(messages=[mock_message], priority=1) consume = AsyncMock() From 09a171d7bca05e91fc381ece49e5bf4e12df6fc9 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Thu, 17 Sep 2026 14:30:13 +0100 Subject: [PATCH 30/40] Try to fix system test w import studies from raw message context --- orthanc/orthanc-anon/plugin/pixl.py | 34 ++++++++++++---------- pixl_core/src/core/queue/subscriber.py | 36 ++++++++++++++++-------- pixl_core/tests/queue/test_subscriber.py | 15 ++++++---- 3 files changed, 53 insertions(+), 32 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 857b4a0da..4555920df 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -45,9 +45,9 @@ from decouple import config from loguru import logger from opentelemetry import trace +from opentelemetry.instrumentation.pika import PikaInstrumentor from opentelemetry.instrumentation.requests import RequestsInstrumentor from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor -from opentelemetry.propagate import extract from pixl_dcmd._database import engine as pixl_db_engine from pixl_dcmd._database import record_skip_reasons_for_study from pixl_dcmd.dicom_helpers import get_study_info @@ -90,6 +90,10 @@ configure_tracing() SQLAlchemyInstrumentor().instrument(engine=pixl_db_engine) RequestsInstrumentor().instrument() +# orthanc-anon runs as a plugin inside Orthanc rather than via `opentelemetry-instrument`, +# so pika isn't auto-instrumented and we need to do it explicitly to pick up the trace +# context propagated from the message publisher. +PikaInstrumentor().instrument() tracer = trace.get_tracer("pixl.orthanc_anon") configure_metrics() @@ -233,25 +237,25 @@ def OnHeartBeat(output, uri, **request) -> Any: # noqa: ARG001 output.AnswerBuffer("OK\n", "text/plain") -def process_anonymisation_message(message: AnonymisationMessage) -> None: +def process_anonymisation_message( + message: AnonymisationMessage, parent_context: Context | None +) -> None: """ Import studies from Orthanc Raw. Offload to a thread pool executor to avoid blocking the Orthanc main thread. - """ - # Extract the trace context injected into the request headers by the caller, and pass it to - # the thread pool job so the import continues the same trace - headers = {key.lower(): value for key, value in requests.request.get("headers", {}).items()} - parent_context = extract(headers) - data = { - "resource_ids": message.resource_ids, - "series_uids": message.series_uids, - "study_uids": message.study_uids, - "project_name": message.project_name, - "parent_context": parent_context, - } - executor.submit(_import_studies_from_raw, data) + :param parent_context: Trace context extracted from the queue message headers by + AnonymisationPixlConsumer, to continue the trace from the message's publisher. + """ + executor.submit( + _import_studies_from_raw, + message.resource_ids, + message.study_uids, + message.project_name, + message.series_uids, + parent_context, + ) def consume_anonymisation_queue() -> None: diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index 82989cc63..5997c9476 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -23,6 +23,7 @@ import aio_pika import pika from decouple import config +from opentelemetry.context import get_current from core.exceptions import ( PixlDiscardError, @@ -39,6 +40,9 @@ from typing import Self from aio_pika.abc import AbstractIncomingMessage + from opentelemetry.context import Context + from pika.adapters.blocking_connection import BlockingChannel + from pika.spec import Basic, BasicProperties from core.token_buffer.tokens import TokenBucket @@ -151,11 +155,11 @@ class AnonymisationPixlConsumer(PixlQueueInterface): def __init__( self, queue_name: str, - callback: Callable[[AnonymisationMessage], Awaitable[None]], + callback: Callable[[AnonymisationMessage, Context | None], None], ) -> None: """Creating connection to RabbitMQ queue""" super().__init__(queue_name=queue_name) - self._callback = callback + self._callback: Callable[[AnonymisationMessage, Context | None], None] = callback @property def _url(self) -> str: @@ -175,34 +179,44 @@ def __enter__(self) -> Self: ) return self - def _process_message(self, message: Any) -> None: - - pixl_message: AnonymisationMessage = deserialise(message.body) + def _process_message( + self, + channel: BlockingChannel, + method: Basic.Deliver, + properties: BasicProperties, # noqa: ARG002 + body: bytes, + ) -> None: + pixl_message: AnonymisationMessage = deserialise(body) + # PikaInstrumentor wraps this callback and extracts the trace context from the + # message headers into the current context, so we just need to read it back here. + parent_context = get_current() logger.debug("Picked up from queue: {}", pixl_message.identifier) try: - self._callback(pixl_message) + self._callback(pixl_message, parent_context) except PixlRequeueMessageError as requeue: logger.trace("Requeue message: {} from {}", pixl_message.identifier, requeue) time.sleep(1) - message.reject(requeue=True) + channel.basic_reject(delivery_tag=method.delivery_tag, requeue=True) except PixlOutOfHoursError as nack_requeue: logger.trace( "Nack and requeue message: {} from {}", pixl_message.identifier, nack_requeue ) time.sleep(10) - message.nack(requeue=True) + channel.basic_nack(delivery_tag=method.delivery_tag, requeue=True) except PixlDiscardError as exception: logger.warning("Failed message {}: {}", pixl_message.identifier, exception) - (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin + # ack so that we can see rate of message processing in rabbitmq admin + channel.basic_ack(delivery_tag=method.delivery_tag) except Exception: # noqa: BLE001 logger.exception( "Failed to process {}. Not re-queuing message", pixl_message.identifier, ) - (message.ack()) # ack so that we can see rate of message processing in rabbitmq admin + # ack so that we can see rate of message processing in rabbitmq admin + channel.basic_ack(delivery_tag=method.delivery_tag) else: logger.success("Finished message {}", pixl_message.identifier) - message.ack() + channel.basic_ack(delivery_tag=method.delivery_tag) def run(self) -> None: """Processes messages from queue.""" diff --git a/pixl_core/tests/queue/test_subscriber.py b/pixl_core/tests/queue/test_subscriber.py index 5420d228a..1d6e99bf1 100644 --- a/pixl_core/tests/queue/test_subscriber.py +++ b/pixl_core/tests/queue/test_subscriber.py @@ -14,7 +14,7 @@ from __future__ import annotations import asyncio -from unittest.mock import AsyncMock, Mock +from unittest.mock import ANY, AsyncMock, Mock import pytest @@ -85,16 +85,19 @@ def test_run_anon() -> None: @pytest.mark.usefixtures("run_containers") def test_process_message_anon(mock_anon_message) -> None: - """Checks that a received message is passed to the callback.""" + """Checks that a received message is passed to the callback and acked.""" callback = Mock() with AnonymisationPixlConsumer( queue_name=TEST_QUEUE_ANON, callback=callback, ) as consumer: - message = Mock() - message.body = mock_anon_message.serialise() + channel = Mock() + method = Mock(delivery_tag=1) + properties = Mock(headers={}) + body = mock_anon_message.serialise() - consumer._process_message(message) + consumer._process_message(channel, method, properties, body) - callback.assert_called_once_with(mock_anon_message) + callback.assert_called_once_with(mock_anon_message, ANY) + channel.basic_ack.assert_called_once_with(delivery_tag=1) From 664b269a5be9c061e1d2de2e425d435359d7da86 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Thu, 17 Sep 2026 15:54:01 +0100 Subject: [PATCH 31/40] Fix queue-declaration argument mismatch in cli --- cli/src/pixl_cli/_config.py | 15 +++++++++++++++ cli/src/pixl_cli/_message_processing.py | 14 +++++++++++--- cli/src/pixl_cli/main.py | 7 ++++++- pixl_core/src/core/queue/subscriber.py | 3 +++ 4 files changed, 35 insertions(+), 4 deletions(-) diff --git a/cli/src/pixl_cli/_config.py b/cli/src/pixl_cli/_config.py index 999b273e8..9a19a3e07 100644 --- a/cli/src/pixl_cli/_config.py +++ b/cli/src/pixl_cli/_config.py @@ -81,6 +81,21 @@ def base_url(self) -> str: } +PRIORITY_QUEUES = {"imaging-primary", "imaging-secondary"} +MAX_QUEUE_PRIORITY = 5 + + +def max_priority_for_queue(queue_name: str) -> int | None: + """ + Maximum message priority for a queue, or None if it isn't a priority queue. + + Must match the value used by whatever consumer declares the queue (e.g. + pixl_imaging's PixlConsumer), since RabbitMQ rejects redeclaring an existing + queue with different arguments. + """ + return MAX_QUEUE_PRIORITY if queue_name in PRIORITY_QUEUES else None + + def api_config_for_queue(queue_name: str) -> APIConfig: """Configuration for an API associated with a queue""" api_name = { diff --git a/cli/src/pixl_cli/_message_processing.py b/cli/src/pixl_cli/_message_processing.py index c31418766..f846003d8 100644 --- a/cli/src/pixl_cli/_message_processing.py +++ b/cli/src/pixl_cli/_message_processing.py @@ -26,7 +26,7 @@ from decouple import config from loguru import logger -from pixl_cli._config import SERVICE_SETTINGS +from pixl_cli._config import SERVICE_SETTINGS, max_priority_for_queue from pixl_cli._database import exported_images_for_project, filter_exported_or_skipped_or_add_to_db if TYPE_CHECKING: @@ -133,7 +133,11 @@ def _message_count(queues_to_populate: list[str]) -> int: messages_in_queues = 0 for queue in queues_to_count: - with PixlBlockingInterface(queue_name=queue, **SERVICE_SETTINGS["rabbitmq"]) as rabbitmq: + with PixlBlockingInterface( + queue_name=queue, + max_priority=max_priority_for_queue(queue), + **SERVICE_SETTINGS["rabbitmq"], + ) as rabbitmq: messages_in_queues += rabbitmq.message_count return messages_in_queues @@ -157,7 +161,11 @@ def populate_queue_and_db( messages_df = filter_exported_or_skipped_or_add_to_db(messages_df) messages = messages_from_df(messages_df) - with PixlProducer(queue_name=queue, **SERVICE_SETTINGS["rabbitmq"]) as producer: + with PixlProducer( + queue_name=queue, + max_priority=max_priority_for_queue(queue), + **SERVICE_SETTINGS["rabbitmq"], + ) as producer: producer.publish(messages, priority=messages_priority) output_messages.extend(messages) diff --git a/cli/src/pixl_cli/main.py b/cli/src/pixl_cli/main.py index d0efb4bdd..16ef4085a 100644 --- a/cli/src/pixl_cli/main.py +++ b/cli/src/pixl_cli/main.py @@ -35,6 +35,7 @@ SERVICE_SETTINGS, api_config_for_queue, config, + max_priority_for_queue, ) from pixl_cli._database import exported_images_for_project from pixl_cli._docker_commands import dc @@ -349,7 +350,11 @@ def stop(queues: str, purge: bool) -> None: # noqa: FBT001 bool argument _update_extract_rate(queue_name=queue, rate=0) if purge: logger.info("Purging queue {}", queue) - with PixlProducer(queue_name=queue, **SERVICE_SETTINGS["rabbitmq"]) as producer: + with PixlProducer( + queue_name=queue, + max_priority=max_priority_for_queue(queue), + **SERVICE_SETTINGS["rabbitmq"], + ) as producer: producer.clear_queue() diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index 5997c9476..eed3b3d32 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -116,6 +116,9 @@ async def _process_message(self, message: AbstractIncomingMessage) -> None: port=config("RABBITMQ_PORT", cast=int), username=config("RABBITMQ_USERNAME"), password=config("RABBITMQ_PASSWORD"), + # Must match the max_priority PixlConsumer declares this queue with, since + # RabbitMQ rejects redeclaring an existing queue with different arguments. + max_priority=5, ) as producer: producer.publish([pixl_message], priority=message.priority) except PixlOutOfHoursError as nack_requeue: From b163d1cb64d7c854b6f02cc4db1cec800a451b30 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Thu, 17 Sep 2026 17:02:47 +0100 Subject: [PATCH 32/40] Add retry to AnonymisationPixlConsumer --- docker-compose.yml | 2 ++ pixl_core/src/core/queue/subscriber.py | 23 +++++++++++++++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index a78df44f3..b192c76ea 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -178,6 +178,8 @@ services: depends_on: postgres: condition: service_healthy + queue: + condition: service_healthy healthcheck: test: [ diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index eed3b3d32..3f713e0a3 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -164,13 +164,24 @@ def __init__( super().__init__(queue_name=queue_name) self._callback: Callable[[AnonymisationMessage, Context | None], None] = callback - @property - def _url(self) -> str: - return f"amqp://{self._username}:{self._password}@{self._host}:{self._port}/" - def __enter__(self) -> Self: - """Establishes connection to queue.""" - self._connection = pika.BlockingConnection(pika.URLParameters(self._url)) + """ + Establishes connection to queue. + + Unlike PixlConsumer (which uses aio_pika.connect_robust and so retries the + initial connection automatically), pika's BlockingConnection has no built-in + retry, so we configure one here. Without it, a transient failure to connect + (e.g. RabbitMQ not quite ready yet at startup) kills the consumer thread + permanently, since nothing else restarts it. + """ + params = pika.ConnectionParameters( + host=self._host, + port=self._port, + credentials=pika.PlainCredentials(self._username, self._password), + connection_attempts=10, + retry_delay=5, + ) + self._connection = pika.BlockingConnection(params) self._channel = self._connection.channel() # Set number of messages in flight max_in_flight = config("PIXL_MAX_MESSAGES_IN_FLIGHT", cast=int) From 7d96ef91e7a61874254d01c4511df60f47faaeaf Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 18 Sep 2026 10:53:36 +0100 Subject: [PATCH 33/40] Add copilot suggestions --- docker-compose.yml | 9 ++++++--- orthanc/orthanc-anon/plugin/pixl.py | 31 +++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 9 deletions(-) diff --git a/docker-compose.yml b/docker-compose.yml index b192c76ea..847580027 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -95,6 +95,9 @@ services: args: PIXL_PACKAGE_DIR: hasher <<: *build-args-common + depends_on: + queue: + condition: service_healthy environment: <<: [*proxy-common, *pixl-common-env, *otel-common] OTEL_SERVICE_NAME: "hasher-api" @@ -130,7 +133,7 @@ services: command: /run/secrets restart: always environment: - <<: [*pixl-db, *proxy-common, *pixl-common-env, *azure-keyvault, *otel-common] + <<: [*pixl-db, *proxy-common, *pixl-common-env, *azure-keyvault, *otel-common, *pixl-rabbit-mq] OTEL_SERVICE_NAME: "orthanc-anon" ORTHANC_NAME: "PIXL: Anon" ORTHANC_USERNAME: ${ORTHANC_ANON_USERNAME} @@ -186,7 +189,7 @@ services: "CMD-SHELL", "/probes/test-aliveness.py --user=$ORTHANC_USERNAME --pwd=$ORTHANC_PASSWORD", ] - start_period: 10s + start_period: 90s retries: 10 interval: 3s timeout: 2s @@ -245,7 +248,7 @@ services: "CMD-SHELL", "/probes/test-aliveness.py --user=$ORTHANC_USERNAME --pwd=$ORTHANC_PASSWORD", ] - start_period: 10s + start_period: 90s retries: 10 interval: 3s timeout: 2s diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 4555920df..b7c0bead0 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -32,6 +32,7 @@ from typing import TYPE_CHECKING, cast from zipfile import ZipFile +import pika import pydicom import requests from core.exceptions import PixlDiscardError, PixlSkipInstanceError @@ -258,13 +259,31 @@ def process_anonymisation_message( ) +RABBITMQ_RECONNECT_DELAY_SECONDS = 5 + + def consume_anonymisation_queue() -> None: - """Consume anonymisation requests from RabbitMQ and submit them for processing.""" - with AnonymisationPixlConsumer( - queue_name="anonymisation", - callback=process_anonymisation_message, - ) as consumer: - consumer.run() + """ + Consume anonymisation requests from RabbitMQ and submit them for processing. + + Runs for the lifetime of the process. AnonymisationPixlConsumer only retries the + initial connection; if RabbitMQ becomes unavailable afterwards (e.g. a restart), + pika.BlockingConnection raises out of consumer.run() and would otherwise kill this + thread permanently, since nothing else restarts it. So reconnect here instead. + """ + while True: + try: + with AnonymisationPixlConsumer( + queue_name="anonymisation", + callback=process_anonymisation_message, + ) as consumer: + consumer.run() + except pika.exceptions.AMQPConnectionError: + logger.exception( + "Anonymisation consumer lost connection to RabbitMQ; reconnecting in {} seconds", + RABBITMQ_RECONNECT_DELAY_SECONDS, + ) + sleep(RABBITMQ_RECONNECT_DELAY_SECONDS) consumer_thread = threading.Thread( From 7dffc130c3a73d6eddf2a90e89b01cf4b90023c9 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 18 Sep 2026 13:51:48 +0100 Subject: [PATCH 34/40] Remove redundant input for anon message type from async consumer --- pixl_core/src/core/queue/subscriber.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index 3f713e0a3..43d01a5dc 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -49,7 +49,7 @@ from loguru import logger -class PixlConsumer[PixlMessage: (ImagingRequestMessage, AnonymisationMessage)](PixlQueueInterface): +class PixlConsumer[PixlMessage: ImagingRequestMessage](PixlQueueInterface): """Connector to RabbitMQ. Consumes messages from a queue""" def __init__( From 9ce76795e7c239614da28cbfaf056c0f496c3438 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Fri, 18 Sep 2026 13:54:02 +0100 Subject: [PATCH 35/40] Improve test coverage for subscriber covering error handling --- pixl_core/tests/conftest.py | 28 ++++++++++- pixl_core/tests/queue/test_subscriber.py | 63 ++++++++++++++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/pixl_core/tests/conftest.py b/pixl_core/tests/conftest.py index d867c021f..e6ced2255 100644 --- a/pixl_core/tests/conftest.py +++ b/pixl_core/tests/conftest.py @@ -19,6 +19,7 @@ import shlex from pathlib import Path from typing import TYPE_CHECKING +from unittest.mock import AsyncMock, Mock import pytest import requests @@ -37,10 +38,11 @@ from core.db.models import Base, Extract, Image from core.logging import OTelSink from core.queue.models import AnonymisationMessage, ImagingRequestMessage +from core.queue.subscriber import AnonymisationPixlConsumer if TYPE_CHECKING: import subprocess - from collections.abc import Generator + from collections.abc import Callable, Generator pytest_plugins = "pytest_pixl" @@ -247,6 +249,30 @@ def mock_anon_message() -> AnonymisationMessage: ) +@pytest.fixture +def mock_incoming_message() -> Callable[..., Mock]: + """Factory for a mock aio_pika incoming message with async ack/nack/reject.""" + + def _make(body: bytes, priority: int = 1) -> Mock: + message = Mock(body=body, priority=priority) + message.reject = AsyncMock() + message.ack = AsyncMock() + message.nack = AsyncMock() + return message + + return _make + + +@pytest.fixture +def anon_consumer() -> Callable[..., AnonymisationPixlConsumer]: + """Factory for an AnonymisationPixlConsumer, without connecting to a broker.""" + + def _make(queue_name: str, callback: Mock) -> AnonymisationPixlConsumer: + return AnonymisationPixlConsumer(queue_name=queue_name, callback=callback) + + return _make + + @pytest.fixture def log_exporter() -> InMemoryLogRecordExporter: """In-memory exporter capturing the OTel log records the sink emits.""" diff --git a/pixl_core/tests/queue/test_subscriber.py b/pixl_core/tests/queue/test_subscriber.py index 1d6e99bf1..2a40ccfc4 100644 --- a/pixl_core/tests/queue/test_subscriber.py +++ b/pixl_core/tests/queue/test_subscriber.py @@ -18,6 +18,11 @@ import pytest +from core.exceptions import ( + PixlDiscardError, + PixlOutOfHoursError, + PixlRequeueMessageError, +) from core.queue.producer import PixlProducer from core.queue.subscriber import AnonymisationPixlConsumer, PixlConsumer from core.token_buffer.tokens import TokenBucket @@ -30,6 +35,16 @@ class ExpectedTestError(Exception): """Expected error for testing.""" +# Shared by both PixlConsumer and AnonymisationPixlConsumer error-handling tests below, +# so the two consumers' behaviour for a given error can't silently drift apart. +ERROR_HANDLING_CASES = [ + pytest.param(PixlRequeueMessageError, "reject", {"requeue": True}, id="requeue"), + pytest.param(PixlOutOfHoursError, "nack", {"requeue": True}, id="out_of_hours"), + pytest.param(PixlDiscardError, "ack", {}, id="discard"), + pytest.param(ExpectedTestError, "ack", {}, id="unexpected"), +] + + @pytest.mark.asyncio @pytest.mark.usefixtures("run_containers") @pytest.mark.xfail( @@ -101,3 +116,51 @@ def test_process_message_anon(mock_anon_message) -> None: callback.assert_called_once_with(mock_anon_message, ANY) channel.basic_ack.assert_called_once_with(delivery_tag=1) + + +@pytest.mark.asyncio +@pytest.mark.parametrize(("error", "method_name", "expected_kwargs"), ERROR_HANDLING_CASES) +async def test_process_message_error_handling( # noqa: PLR0913 + monkeypatch, + mock_message, + mock_incoming_message, + error, + method_name, + expected_kwargs, +) -> None: + """Each error type from the callback results in the correct ack/nack/reject call.""" + monkeypatch.setattr(asyncio, "sleep", AsyncMock()) + callback = AsyncMock(side_effect=error) + token_bucket = Mock(has_token=Mock(return_value=True)) + + consumer = PixlConsumer( + queue_name=TEST_QUEUE, + token_bucket=token_bucket, + token_bucket_key="primary", # noqa: S106 + callback=callback, + ) + message = mock_incoming_message(mock_message.serialise()) + + await consumer._process_message(message) + + getattr(message, method_name).assert_awaited_once_with(**expected_kwargs) + + +@pytest.mark.parametrize(("error", "method_name", "expected_kwargs"), ERROR_HANDLING_CASES) +def test_process_message_anon_error_handling( # noqa: PLR0913 + monkeypatch, mock_anon_message, anon_consumer, error, method_name, expected_kwargs +) -> None: + """Each error type from the callback results in the correct ack/nack/reject call.""" + monkeypatch.setattr("core.queue.subscriber.time.sleep", Mock()) + callback = Mock(side_effect=error) + consumer = anon_consumer(TEST_QUEUE_ANON, callback) + + channel = Mock() + method = Mock(delivery_tag=1) + properties = Mock(headers={}) + + consumer._process_message(channel, method, properties, mock_anon_message.serialise()) + + getattr(channel, f"basic_{method_name}").assert_called_once_with( + delivery_tag=1, **expected_kwargs + ) From 1ad2a0d79f41598eb0c4e699b216b32a3f717705 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Tue, 22 Sep 2026 10:44:19 +0100 Subject: [PATCH 36/40] Simplify max-priority config args and give Anon subscriber max-priority --- cli/src/pixl_cli/_config.py | 15 --------------- cli/src/pixl_cli/_message_processing.py | 14 +++----------- cli/src/pixl_cli/main.py | 7 +------ pixl_core/src/core/queue/subscriber.py | 4 +--- 4 files changed, 5 insertions(+), 35 deletions(-) diff --git a/cli/src/pixl_cli/_config.py b/cli/src/pixl_cli/_config.py index 9a19a3e07..999b273e8 100644 --- a/cli/src/pixl_cli/_config.py +++ b/cli/src/pixl_cli/_config.py @@ -81,21 +81,6 @@ def base_url(self) -> str: } -PRIORITY_QUEUES = {"imaging-primary", "imaging-secondary"} -MAX_QUEUE_PRIORITY = 5 - - -def max_priority_for_queue(queue_name: str) -> int | None: - """ - Maximum message priority for a queue, or None if it isn't a priority queue. - - Must match the value used by whatever consumer declares the queue (e.g. - pixl_imaging's PixlConsumer), since RabbitMQ rejects redeclaring an existing - queue with different arguments. - """ - return MAX_QUEUE_PRIORITY if queue_name in PRIORITY_QUEUES else None - - def api_config_for_queue(queue_name: str) -> APIConfig: """Configuration for an API associated with a queue""" api_name = { diff --git a/cli/src/pixl_cli/_message_processing.py b/cli/src/pixl_cli/_message_processing.py index f846003d8..c31418766 100644 --- a/cli/src/pixl_cli/_message_processing.py +++ b/cli/src/pixl_cli/_message_processing.py @@ -26,7 +26,7 @@ from decouple import config from loguru import logger -from pixl_cli._config import SERVICE_SETTINGS, max_priority_for_queue +from pixl_cli._config import SERVICE_SETTINGS from pixl_cli._database import exported_images_for_project, filter_exported_or_skipped_or_add_to_db if TYPE_CHECKING: @@ -133,11 +133,7 @@ def _message_count(queues_to_populate: list[str]) -> int: messages_in_queues = 0 for queue in queues_to_count: - with PixlBlockingInterface( - queue_name=queue, - max_priority=max_priority_for_queue(queue), - **SERVICE_SETTINGS["rabbitmq"], - ) as rabbitmq: + with PixlBlockingInterface(queue_name=queue, **SERVICE_SETTINGS["rabbitmq"]) as rabbitmq: messages_in_queues += rabbitmq.message_count return messages_in_queues @@ -161,11 +157,7 @@ def populate_queue_and_db( messages_df = filter_exported_or_skipped_or_add_to_db(messages_df) messages = messages_from_df(messages_df) - with PixlProducer( - queue_name=queue, - max_priority=max_priority_for_queue(queue), - **SERVICE_SETTINGS["rabbitmq"], - ) as producer: + with PixlProducer(queue_name=queue, **SERVICE_SETTINGS["rabbitmq"]) as producer: producer.publish(messages, priority=messages_priority) output_messages.extend(messages) diff --git a/cli/src/pixl_cli/main.py b/cli/src/pixl_cli/main.py index 05001a59f..94a8af475 100644 --- a/cli/src/pixl_cli/main.py +++ b/cli/src/pixl_cli/main.py @@ -35,7 +35,6 @@ SERVICE_SETTINGS, api_config_for_queue, config, - max_priority_for_queue, ) from pixl_cli._database import exported_images_for_project from pixl_cli._docker_commands import dc @@ -351,11 +350,7 @@ def stop(queues: str, purge: bool) -> None: # noqa: FBT001 bool argument _update_extract_rate(queue_name=queue, rate=0) if purge: logger.info("Purging queue {}", queue) - with PixlProducer( - queue_name=queue, - max_priority=max_priority_for_queue(queue), - **SERVICE_SETTINGS["rabbitmq"], - ) as producer: + with PixlProducer(queue_name=queue, **SERVICE_SETTINGS["rabbitmq"]) as producer: producer.clear_queue() diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index 43d01a5dc..c4574a117 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -116,9 +116,6 @@ async def _process_message(self, message: AbstractIncomingMessage) -> None: port=config("RABBITMQ_PORT", cast=int), username=config("RABBITMQ_USERNAME"), password=config("RABBITMQ_PASSWORD"), - # Must match the max_priority PixlConsumer declares this queue with, since - # RabbitMQ rejects redeclaring an existing queue with different arguments. - max_priority=5, ) as producer: producer.publish([pixl_message], priority=message.priority) except PixlOutOfHoursError as nack_requeue: @@ -190,6 +187,7 @@ def __enter__(self) -> Self: self._queue = self._channel.queue_declare( queue=self.queue_name, durable=True, + arguments={"x-max-priority": 5}, ) return self From 78076370ecc4ec62e25747fd710f45f073e342e8 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Tue, 22 Sep 2026 11:27:26 +0100 Subject: [PATCH 37/40] Simplify max-priority config args and give Anon subscriber max-priority --- pixl_core/src/core/queue/_base.py | 25 +----------------------- pixl_core/tests/queue/test_subscriber.py | 2 +- 2 files changed, 2 insertions(+), 25 deletions(-) diff --git a/pixl_core/src/core/queue/_base.py b/pixl_core/src/core/queue/_base.py index 20a217f81..aa9c19922 100644 --- a/pixl_core/src/core/queue/_base.py +++ b/pixl_core/src/core/queue/_base.py @@ -52,26 +52,6 @@ def __init__( class PixlBlockingInterface(PixlQueueInterface): - def __init__( # noqa: PLR0913 - self, - queue_name: str, - host: str = "localhost", - port: int = 5672, - username: str = "guest", - password: str = "guest", # noqa: S107 - max_priority: int | None = None, - ) -> None: - """ - RabbitMQ interface using a blocking connection. - - :param max_priority: If set, declares the queue as a priority queue with this - maximum priority. Must match the value used wherever else this queue is - declared, since RabbitMQ rejects redeclaring an existing queue with - different arguments. - """ - super().__init__(queue_name, host, port, username, password) - self._max_priority = max_priority - def __enter__(self) -> Any: """Establishes connection to RabbitMQ service.""" params = pika.ConnectionParameters( @@ -85,13 +65,10 @@ def __enter__(self) -> Any: if self._channel is None or self._channel.is_closed: self._channel = self._connection.channel() - arguments = ( - {"x-max-priority": self._max_priority} if self._max_priority is not None else None - ) self._queue = self._channel.queue_declare( queue=self.queue_name, durable=True, - arguments=arguments, + arguments={"x-max-priority": 5}, ) logger.debug("Connected to {}", self.queue_name) diff --git a/pixl_core/tests/queue/test_subscriber.py b/pixl_core/tests/queue/test_subscriber.py index 2a40ccfc4..de5898a7a 100644 --- a/pixl_core/tests/queue/test_subscriber.py +++ b/pixl_core/tests/queue/test_subscriber.py @@ -52,7 +52,7 @@ class ExpectedTestError(Exception): ) async def test_create(mock_message) -> None: """Checks consume is working.""" - with PixlProducer(queue_name=TEST_QUEUE, max_priority=5) as producer: + with PixlProducer(queue_name=TEST_QUEUE) as producer: producer.publish(messages=[mock_message], priority=1) consume = AsyncMock() From b03f5072d6298d56305a743a1831095c5a9d1d5e Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Tue, 22 Sep 2026 16:26:07 +0100 Subject: [PATCH 38/40] Remove None as an option for parent context --- orthanc/orthanc-anon/plugin/pixl.py | 6 ++---- pixl_core/src/core/queue/subscriber.py | 4 ++-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index c22b0e33e..59d475a29 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -238,9 +238,7 @@ def OnHeartBeat(output, uri, **request) -> Any: # noqa: ARG001 output.AnswerBuffer("OK\n", "text/plain") -def process_anonymisation_message( - message: AnonymisationMessage, parent_context: Context | None -) -> None: +def process_anonymisation_message(message: AnonymisationMessage, parent_context: Context) -> None: """ Import studies from Orthanc Raw. @@ -298,7 +296,7 @@ def _import_studies_from_raw( study_uids: list[str], project_name: str, series_to_keep: list[str], - parent_context: Context | None = None, + parent_context: Context, ) -> None: """ Import studies from Orthanc Raw. diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index c4574a117..560b4f193 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -155,11 +155,11 @@ class AnonymisationPixlConsumer(PixlQueueInterface): def __init__( self, queue_name: str, - callback: Callable[[AnonymisationMessage, Context | None], None], + callback: Callable[[AnonymisationMessage, Context], None], ) -> None: """Creating connection to RabbitMQ queue""" super().__init__(queue_name=queue_name) - self._callback: Callable[[AnonymisationMessage, Context | None], None] = callback + self._callback: Callable[[AnonymisationMessage, Context], None] = callback def __enter__(self) -> Self: """ From fb914cb57e30a4c4fbe0bbae1f6ee7fa8de818f4 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Tue, 22 Sep 2026 16:31:05 +0100 Subject: [PATCH 39/40] Replace Message with ImagingRequestMessage in docs --- pixl_imaging/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pixl_imaging/README.md b/pixl_imaging/README.md index 24371d0c4..c2c7c51c5 100644 --- a/pixl_imaging/README.md +++ b/pixl_imaging/README.md @@ -9,7 +9,7 @@ The imaging API has two queues: - `imaging-secondary`, for querying PACS The imaging API uses RabbitMQ to expose a single HTTP endpoint that expects a JSON-formatted message structured as -defined by the [`Message`](../pixl_core/src/core/queue/message.py) class in `pixl_core/queue`. +defined by the [`ImagingRequestMessage`](../pixl_core/src/core/queue/message.py) class in `pixl_core/queue`. Users should send messages to the `imaging-primary` queue only. On arrival of the input message, the imaging API will query the VNA for the requested study. If the study does not exist in the VNA, the input message will be sent From a79d946609bfda1a9e7773ef3df1637d8fae9712 Mon Sep 17 00:00:00 2001 From: ruaridhg Date: Wed, 23 Sep 2026 13:38:55 +0100 Subject: [PATCH 40/40] AnonymisationPixlConsumer changed to async so ack does not get sent back until process finished --- orthanc/orthanc-anon/plugin/pixl.py | 50 ++++++++----- pixl_core/src/core/queue/_base.py | 4 ++ pixl_core/src/core/queue/subscriber.py | 92 ++++++++---------------- pixl_core/tests/queue/test_subscriber.py | 83 +++++++++++---------- 4 files changed, 105 insertions(+), 124 deletions(-) diff --git a/orthanc/orthanc-anon/plugin/pixl.py b/orthanc/orthanc-anon/plugin/pixl.py index 59d475a29..a73a9d467 100644 --- a/orthanc/orthanc-anon/plugin/pixl.py +++ b/orthanc/orthanc-anon/plugin/pixl.py @@ -21,6 +21,7 @@ from __future__ import annotations +import asyncio import json import os import threading @@ -32,7 +33,7 @@ from typing import TYPE_CHECKING, cast from zipfile import ZipFile -import pika +import aio_pika import pydicom import requests from core.exceptions import PixlDiscardError, PixlSkipInstanceError @@ -46,7 +47,7 @@ from decouple import config from loguru import logger from opentelemetry import trace -from opentelemetry.instrumentation.pika import PikaInstrumentor +from opentelemetry.instrumentation.aio_pika import AioPikaInstrumentor from opentelemetry.instrumentation.requests import RequestsInstrumentor from opentelemetry.instrumentation.sqlalchemy import SQLAlchemyInstrumentor from pixl_dcmd._database import engine as pixl_db_engine @@ -92,9 +93,9 @@ SQLAlchemyInstrumentor().instrument(engine=pixl_db_engine) RequestsInstrumentor().instrument() # orthanc-anon runs as a plugin inside Orthanc rather than via `opentelemetry-instrument`, -# so pika isn't auto-instrumented and we need to do it explicitly to pick up the trace +# so aio-pika isn't auto-instrumented and we need to do it explicitly to pick up the trace # context propagated from the message publisher. -PikaInstrumentor().instrument() +AioPikaInstrumentor().instrument() tracer = trace.get_tracer("pixl.orthanc_anon") configure_metrics() @@ -238,16 +239,22 @@ def OnHeartBeat(output, uri, **request) -> Any: # noqa: ARG001 output.AnswerBuffer("OK\n", "text/plain") -def process_anonymisation_message(message: AnonymisationMessage, parent_context: Context) -> None: +async def process_anonymisation_message( + message: AnonymisationMessage, parent_context: Context +) -> None: """ Import studies from Orthanc Raw. - Offload to a thread pool executor to avoid blocking the Orthanc main thread. + Offload to the thread pool executor, so we don't block the event loop from consuming + further messages, but await completion so the message is only acked by + AnonymisationPixlConsumer once processing has actually finished. :param parent_context: Trace context extracted from the queue message headers by AnonymisationPixlConsumer, to continue the trace from the message's publisher. """ - executor.submit( + loop = asyncio.get_running_loop() + await loop.run_in_executor( + executor, _import_studies_from_raw, message.resource_ids, message.study_uids, @@ -260,32 +267,39 @@ def process_anonymisation_message(message: AnonymisationMessage, parent_context: RABBITMQ_RECONNECT_DELAY_SECONDS = 5 -def consume_anonymisation_queue() -> None: +async def consume_anonymisation_queue() -> None: """ Consume anonymisation requests from RabbitMQ and submit them for processing. - Runs for the lifetime of the process. AnonymisationPixlConsumer only retries the - initial connection; if RabbitMQ becomes unavailable afterwards (e.g. a restart), - pika.BlockingConnection raises out of consumer.run() and would otherwise kill this - thread permanently, since nothing else restarts it. So reconnect here instead. + Runs for the lifetime of the process. Once connected, aio_pika's robust connection + automatically reconnects (and re-registers the consumer) if RabbitMQ becomes + unavailable, so we only need to retry here if the initial connection attempt fails + (e.g. RabbitMQ not quite ready yet at startup). """ while True: try: - with AnonymisationPixlConsumer( + async with AnonymisationPixlConsumer( queue_name="anonymisation", callback=process_anonymisation_message, ) as consumer: - consumer.run() - except pika.exceptions.AMQPConnectionError: + await consumer.run() + # Keep this coroutine (and so the event loop) alive for the lifetime of + # the process, since messages are delivered via the running loop. + await asyncio.Event().wait() + except aio_pika.exceptions.AMQPConnectionError: logger.exception( - "Anonymisation consumer lost connection to RabbitMQ; reconnecting in {} seconds", + "Anonymisation consumer failed to connect to RabbitMQ; retrying in {} seconds", RABBITMQ_RECONNECT_DELAY_SECONDS, ) - sleep(RABBITMQ_RECONNECT_DELAY_SECONDS) + await asyncio.sleep(RABBITMQ_RECONNECT_DELAY_SECONDS) + + +def _run_anonymisation_consumer() -> None: + asyncio.run(consume_anonymisation_queue()) consumer_thread = threading.Thread( - target=consume_anonymisation_queue, + target=_run_anonymisation_consumer, daemon=True, ) consumer_thread.start() diff --git a/pixl_core/src/core/queue/_base.py b/pixl_core/src/core/queue/_base.py index aa9c19922..0c28b76b7 100644 --- a/pixl_core/src/core/queue/_base.py +++ b/pixl_core/src/core/queue/_base.py @@ -50,6 +50,10 @@ def __init__( self._channel: Any = None self._queue: Any = None + @property + def _url(self) -> str: + return f"amqp://{self._username}:{self._password}@{self._host}:{self._port}/" + class PixlBlockingInterface(PixlQueueInterface): def __enter__(self) -> Any: diff --git a/pixl_core/src/core/queue/subscriber.py b/pixl_core/src/core/queue/subscriber.py index 560b4f193..0d5bba5cb 100644 --- a/pixl_core/src/core/queue/subscriber.py +++ b/pixl_core/src/core/queue/subscriber.py @@ -17,11 +17,9 @@ from __future__ import annotations import asyncio -import time from typing import TYPE_CHECKING, Any import aio_pika -import pika from decouple import config from opentelemetry.context import get_current @@ -41,8 +39,6 @@ from aio_pika.abc import AbstractIncomingMessage from opentelemetry.context import Context - from pika.adapters.blocking_connection import BlockingChannel - from pika.spec import Basic, BasicProperties from core.token_buffer.tokens import TokenBucket @@ -68,10 +64,6 @@ def __init__( self.token_bucket_key = token_bucket_key self._callback: Callable[[PixlMessage], Awaitable[None]] = callback - @property - def _url(self) -> str: - return f"amqp://{self._username}:{self._password}@{self._host}:{self._port}/" - async def __aenter__(self) -> Self: """Establishes connection to queue.""" self._connection = await aio_pika.connect_robust(self._url) @@ -155,94 +147,66 @@ class AnonymisationPixlConsumer(PixlQueueInterface): def __init__( self, queue_name: str, - callback: Callable[[AnonymisationMessage, Context], None], + callback: Callable[[AnonymisationMessage, Context], Awaitable[None]], ) -> None: """Creating connection to RabbitMQ queue""" super().__init__(queue_name=queue_name) - self._callback: Callable[[AnonymisationMessage, Context], None] = callback - - def __enter__(self) -> Self: - """ - Establishes connection to queue. + self._callback: Callable[[AnonymisationMessage, Context], Awaitable[None]] = callback - Unlike PixlConsumer (which uses aio_pika.connect_robust and so retries the - initial connection automatically), pika's BlockingConnection has no built-in - retry, so we configure one here. Without it, a transient failure to connect - (e.g. RabbitMQ not quite ready yet at startup) kills the consumer thread - permanently, since nothing else restarts it. - """ - params = pika.ConnectionParameters( - host=self._host, - port=self._port, - credentials=pika.PlainCredentials(self._username, self._password), - connection_attempts=10, - retry_delay=5, - ) - self._connection = pika.BlockingConnection(params) - self._channel = self._connection.channel() + async def __aenter__(self) -> Self: + """Establishes connection to queue.""" + self._connection = await aio_pika.connect_robust(self._url) + self._channel = await self._connection.channel() # Set number of messages in flight max_in_flight = config("PIXL_MAX_MESSAGES_IN_FLIGHT", cast=int) logger.info("Pika will consume up to {} messages concurrently", max_in_flight) - self._channel.basic_qos(prefetch_count=max_in_flight) - self._queue = self._channel.queue_declare( - queue=self.queue_name, + await self._channel.set_qos(prefetch_count=max_in_flight) + self._queue = await self._channel.declare_queue( + self.queue_name, durable=True, arguments={"x-max-priority": 5}, ) return self - def _process_message( - self, - channel: BlockingChannel, - method: Basic.Deliver, - properties: BasicProperties, # noqa: ARG002 - body: bytes, - ) -> None: - pixl_message: AnonymisationMessage = deserialise(body) - # PikaInstrumentor wraps this callback and extracts the trace context from the + async def _process_message(self, message: AbstractIncomingMessage) -> None: + pixl_message: AnonymisationMessage = deserialise(message.body) + # AioPikaInstrumentor wraps this callback and extracts the trace context from the # message headers into the current context, so we just need to read it back here. parent_context = get_current() logger.debug("Picked up from queue: {}", pixl_message.identifier) try: - self._callback(pixl_message, parent_context) + # Awaiting the callback here (rather than firing-and-forgetting the work) means + # the message is only acked once processing has actually finished, so a crash + # part-way through doesn't silently lose the message. + await self._callback(pixl_message, parent_context) except PixlRequeueMessageError as requeue: logger.trace("Requeue message: {} from {}", pixl_message.identifier, requeue) - time.sleep(1) - channel.basic_reject(delivery_tag=method.delivery_tag, requeue=True) + await asyncio.sleep(1) + await message.reject(requeue=True) except PixlOutOfHoursError as nack_requeue: logger.trace( "Nack and requeue message: {} from {}", pixl_message.identifier, nack_requeue ) - time.sleep(10) - channel.basic_nack(delivery_tag=method.delivery_tag, requeue=True) + await asyncio.sleep(10) + await message.nack(requeue=True) except PixlDiscardError as exception: logger.warning("Failed message {}: {}", pixl_message.identifier, exception) # ack so that we can see rate of message processing in rabbitmq admin - channel.basic_ack(delivery_tag=method.delivery_tag) + await message.ack() except Exception: # noqa: BLE001 logger.exception( "Failed to process {}. Not re-queuing message", pixl_message.identifier, ) # ack so that we can see rate of message processing in rabbitmq admin - channel.basic_ack(delivery_tag=method.delivery_tag) + await message.ack() else: logger.success("Finished message {}", pixl_message.identifier) - channel.basic_ack(delivery_tag=method.delivery_tag) - - def run(self) -> None: - """Processes messages from queue.""" - self._channel.basic_consume( - queue=self.queue_name, - on_message_callback=self._process_message, - auto_ack=False, - ) - self._channel.start_consuming() + await message.ack() - def __exit__(self, *args: object, **kwargs: Any) -> None: - """Requirement for the context manager""" - if self._channel is not None and self._channel.is_open: - self._channel.close() + async def run(self) -> None: + """Processes messages from queue asynchronously.""" + await self._queue.consume(self._process_message) - if self._connection is not None and self._connection.is_open: - self._connection.close() + async def __aexit__(self, *args: object, **kwargs: Any) -> None: + """Requirement for the asynchronous context manager""" diff --git a/pixl_core/tests/queue/test_subscriber.py b/pixl_core/tests/queue/test_subscriber.py index de5898a7a..dafcdc0e6 100644 --- a/pixl_core/tests/queue/test_subscriber.py +++ b/pixl_core/tests/queue/test_subscriber.py @@ -23,7 +23,7 @@ PixlOutOfHoursError, PixlRequeueMessageError, ) -from core.queue.producer import PixlProducer +from core.queue.producer import AnonymisationProducer, PixlProducer from core.queue.subscriber import AnonymisationPixlConsumer, PixlConsumer from core.token_buffer.tokens import TokenBucket @@ -76,46 +76,43 @@ async def test_create(mock_message) -> None: raise ExpectedTestError +@pytest.mark.asyncio @pytest.mark.usefixtures("run_containers") -def test_run_anon() -> None: +async def test_run_anon(mock_anon_message) -> None: """Checks that the consumer starts consuming messages.""" - callback = Mock() + with AnonymisationProducer(queue_name=TEST_QUEUE_ANON) as producer: + producer.publish(messages=[mock_anon_message]) - with AnonymisationPixlConsumer( + callback = AsyncMock() + async with AnonymisationPixlConsumer( queue_name=TEST_QUEUE_ANON, callback=callback, ) as consumer: - consumer._channel.basic_consume = Mock() - consumer._channel.start_consuming = Mock() - - consumer.run() - - consumer._channel.basic_consume.assert_called_once_with( - queue=TEST_QUEUE_ANON, - on_message_callback=consumer._process_message, - auto_ack=False, - ) - consumer._channel.start_consuming.assert_called_once() + # Create a Task to run consumer.run in the background + task = asyncio.create_task(consumer.run()) + # Wait for a short time to allow consumer.run to start and pick up the message + await asyncio.sleep(1) + # Cancel before assertion so the task doesn't hang + task.cancel() + # need to close the connection and channel + await consumer._channel.close() + await consumer._connection.close() + callback.assert_called_once_with(mock_anon_message, ANY) -@pytest.mark.usefixtures("run_containers") -def test_process_message_anon(mock_anon_message) -> None: +@pytest.mark.asyncio +async def test_process_message_anon( + mock_anon_message, mock_incoming_message, anon_consumer +) -> None: """Checks that a received message is passed to the callback and acked.""" - callback = Mock() - - with AnonymisationPixlConsumer( - queue_name=TEST_QUEUE_ANON, - callback=callback, - ) as consumer: - channel = Mock() - method = Mock(delivery_tag=1) - properties = Mock(headers={}) - body = mock_anon_message.serialise() + callback = AsyncMock() + consumer = anon_consumer(TEST_QUEUE_ANON, callback) + message = mock_incoming_message(mock_anon_message.serialise()) - consumer._process_message(channel, method, properties, body) + await consumer._process_message(message) - callback.assert_called_once_with(mock_anon_message, ANY) - channel.basic_ack.assert_called_once_with(delivery_tag=1) + callback.assert_awaited_once_with(mock_anon_message, ANY) + message.ack.assert_awaited_once() @pytest.mark.asyncio @@ -146,21 +143,23 @@ async def test_process_message_error_handling( # noqa: PLR0913 getattr(message, method_name).assert_awaited_once_with(**expected_kwargs) +@pytest.mark.asyncio @pytest.mark.parametrize(("error", "method_name", "expected_kwargs"), ERROR_HANDLING_CASES) -def test_process_message_anon_error_handling( # noqa: PLR0913 - monkeypatch, mock_anon_message, anon_consumer, error, method_name, expected_kwargs +async def test_process_message_anon_error_handling( # noqa: PLR0913 + monkeypatch, + mock_anon_message, + mock_incoming_message, + anon_consumer, + error, + method_name, + expected_kwargs, ) -> None: """Each error type from the callback results in the correct ack/nack/reject call.""" - monkeypatch.setattr("core.queue.subscriber.time.sleep", Mock()) - callback = Mock(side_effect=error) + monkeypatch.setattr(asyncio, "sleep", AsyncMock()) + callback = AsyncMock(side_effect=error) consumer = anon_consumer(TEST_QUEUE_ANON, callback) + message = mock_incoming_message(mock_anon_message.serialise()) - channel = Mock() - method = Mock(delivery_tag=1) - properties = Mock(headers={}) - - consumer._process_message(channel, method, properties, mock_anon_message.serialise()) + await consumer._process_message(message) - getattr(channel, f"basic_{method_name}").assert_called_once_with( - delivery_tag=1, **expected_kwargs - ) + getattr(message, method_name).assert_awaited_once_with(**expected_kwargs)