650 anonymisation reads from queue by ruaridhg · Pull Request #663 · SAFEHR-data/PIXL

ruaridhg · 2026-08-21T13:02:53Z

Description

Fixes PIX-68

Fixes #650: Adds new queue "anonymisation" in pixl_core/anon_queue without async functionality, within orthanc adds anon_queue (to replace POST call which hasn't been removed yet), adds "anonymisation" to queue_to_count within the cli and adds corresponding tests within cli and pixl_core.

Type of change

Please delete options accordingly to the description.

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

Suggested Checklist

I have performed a self-review of my own code.
I have made corresponding changes to the documentation.
My changes generate no new warnings.
I have commented my code, particularly in hard-to-understand areas.
I have passed on my local host device. (see further details at the CONTRIBUTING document)
Make sure your branch is up-to-date with main branch. See CONTRIBUTING for a general example to syncronise your branch with the main branch.
I have requested review to this PR.
I have addressed and marked as resolved all the review comments in my PR.
Finally, I have selected squash and merge

…aming of queue objects

codecov · 2026-08-21T13:04:24Z

Codecov Report

❌ Patch coverage is 97.10145% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.32%. Comparing base (e69648d) to head (fb914cb).

Files with missing lines	Patch %	Lines
pixl_core/src/core/queue/producer.py	89.47%	2 Missing ⚠️
pixl_imaging/src/pixl_imaging/main.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #663      +/-   ##
==========================================
+ Coverage   88.41%   89.32%   +0.90%     
==========================================
  Files          81       81              
  Lines        3972     4074     +102     
==========================================
+ Hits         3512     3639     +127     
+ Misses        460      435      -25

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

…async

stefpiatek

Ah nice, thanks Ruaridh, appreciate cracking this out. Some structural changes and then happy to a more fine-grained review

stefpiatek · 2026-09-10T15:44:45Z

    # 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()}
+    headers = {key.lower(): value for key, value in requests.request.get("headers", {}).items()}


Oh hello, I don't think requests will be able to get any headers. You'll probably need to look at was was in the original headers or ask @p-j-smith

Will have a look and ask @p-j-smith tomorrow if I can't figure it out! I've reduced redundancy in the consumers, producers etc. so hopefully the code is cleaner now (the tests pass apart from the system test)

ah yeah, so previously the requests auto-instrumentation library injected the headers when calling the ImportStudiesFromRaw endpoint. Now though, the pika auto-instrumentation will handle this - it's installed in the imaging-api Docker Image, and is started when we spin up the container, so this happens automatically.

We can't change the orthanc-anon entrypoint to start the auto-instrumentation libraries, so we need to explicitly call PikaInstrumentor().instrument() so that we can then extract the parent context. We already do this in the CLI:

PIXL/cli/src/pixl_cli/main.py

Line 30 in e29f4b1

from opentelemetry.instrumentation.pika import PikaInstrumentor

PIXL/cli/src/pixl_cli/main.py

Line 74 in e29f4b1

PikaInstrumentor().instrument()

and we'll need to do the same for orthanc-anon. Then when you need to get the parent context, there is a opentelemetry.context.get_current function you can call to get the parent_context

stefpiatek

Thanks Ruaridh. Quick review for some feedback, haven't looked in detail at tests. But expect system test is failing because of the requests call that probably fails.

Co-authored-by: Stef Piatek <s.piatek@ucl.ac.uk>

p-j-smith · 2026-09-17T10:02:35Z

+    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,
+    }


_import_studies_from_raw doesn't take a data argument, I think you'll need to keep the same arguments as before (and keep the same names, e.g. study_resource_ids rather than `resource_ids)

linear-code · 2026-09-17T11:00:20Z

PIX-68

ruaridhg · 2026-09-17T16:17:02Z

@p-j-smith ~~more of a review on what's going wrong as the system test is still failing~~ system test is passing, just working on code coverage now

…ds-from-queue

p-j-smith

looking good. There's also the question of whether we want to make the new queue async, but for a future PR?

p-j-smith · 2026-09-21T09:50:14Z

+                # Must match the max_priority PixlConsumer declares this queue with, since
+                # RabbitMQ rejects redeclaring an existing queue with different arguments.
+                max_priority=5,


is it necessary to pass max_priority here? What happens if you leave the argument out?

p-j-smith · 2026-09-21T09:56:32Z



+PRIORITY_QUEUES = {"imaging-primary", "imaging-secondary"}
+MAX_QUEUE_PRIORITY = 5


is there any harm in also setting the max-priority for the anonymisation queue? It would make it a priority queue, but I assume there's a default priority so we wouldn't need to pass it in or have a way to override it. Only because there's a fair bit of config / logic needed throughout the code to make the anonymisation queue a non-priority queue

Yeah it might be worth leaving in, the original issue stated to leave it out but it has made things more complicated.

ruaridhg · 2026-09-22T08:59:43Z

looking good. There's also the question of whether we want to make the new queue async, but for a future PR?

Yeah a future PR would be good, I think it would be easier to switch from sync to async later on as more could be combined but having the structure for sync is maybe useful to include for now

stefpiatek

Thanks Ruaridh, looking good to me. A couple of questions and a suggestion for documentation

stefpiatek · 2026-09-22T14:30:40Z



-class PixlConsumer(PixlQueueInterface):
+class PixlConsumer[PixlMessage: ImagingRequestMessage](PixlQueueInterface):


Love to see some type generics

Though we're not subclassing this, so might be worth just using the exact ImagingRequestMessage type within the class

stefpiatek · 2026-09-22T14:35:48Z

looking good. There's also the question of whether we want to #663 (comment), but for a future PR?

I don't think we do want this, because the de-id is an intensive process we'd probably want to spawn child processes to handle it so we're not bound by the same process

ruaridhg added 7 commits August 21, 2026 09:57

Add anon_queue to pixl_core with different message and no priority

26ab1ef

Remove async functionality from anon_queue subscriber

735d67b

Replace POST with anon queue in pixl_imaging/_orthanc.py and update n…

91b891b

…aming of queue objects

AnonymisationProducer publishes [message] instead of message

6c5b362

Clean up anon_queue subscriber

0e0ee07

Add RabbitMQ anon_queue in orthanc-anon plugin

48cee58

Add anonymisation queue to _message_count()

250f775

ruaridhg linked an issue Aug 21, 2026 that may be closed by this pull request

Anonymisation reads from queue #650

Open

3 tasks

Merge branch 'main' into 650-anonymisation-reads-from-queue

c485ea2

ruaridhg added 8 commits August 21, 2026 14:41

Added tests for anon_queue and modified subscriber to move away from …

0e3c3d1

…async

Remove SERVICE_SETTINGS[rabbitmq] from AnonymisationProducer

050ddfb

Add anonymisation queue to cli tests

e093a01

Remove anonymisation queue being populated from cli

47830a7

Added __init__.py to both queues tests so differentiate same names

124246e

Renamed anon_queue test queues to fix name conflict

2dd0829

Add test to increase coverage including anonymisation in queue_names

2f22ef9

Add test to cover new orthanc queue connection

1eb4a9d

ruaridhg marked this pull request as ready for review August 21, 2026 14:53

ruaridhg requested review from HChughtai, p-j-smith and stefpiatek August 21, 2026 15:02

stefpiatek reviewed Aug 24, 2026

View reviewed changes

Comment thread pixl_core/src/core/anon_queue/_base.py

Comment thread pixl_core/tests/queue/__init__.py

Comment thread pixl_core/tests/patient_queue/__init__.py Outdated

Comment thread orthanc/orthanc-anon/plugin/pixl.py

Comment thread pixl_core/tests/anon_queue/test_subscriber.py

ruaridhg and others added 4 commits September 3, 2026 13:28

Merge branch 'main' into 650-anonymisation-reads-from-queue

af4a6e6

Combine 2 queues into core.queue

6586344

Update imports to match new core.queue

d5bbd00

Remove ImportStudiesFromRaw and API call

d3e4b3f

stefpiatek reviewed Sep 9, 2026

View reviewed changes

Comment thread orthanc/orthanc-anon/plugin/pixl.py Outdated

ruaridhg added 2 commits September 9, 2026 15:45

Create models.py for ImagingRequest message and AnonymisationMessage

2e61304

Add parent_context back to process_anon_message

93c608f

stefpiatek reviewed Sep 10, 2026

View reviewed changes

stefpiatek requested changes Sep 10, 2026

View reviewed changes

Comment thread pixl_core/src/core/queue/_base.py Outdated

Comment thread pixl_core/src/core/queue/message.py Outdated

Comment thread pixl_core/src/core/queue/subscriber.py Outdated

ruaridhg and others added 4 commits September 16, 2026 12:20

Update pixl_core/src/core/queue/subscriber.py

942a15f

Co-authored-by: Stef Piatek <s.piatek@ucl.ac.uk>

Address PR review comments - reduce redundancy

24e118c

Rename tests

c145c6b

Fix unit tests

fe71ad3

ruaridhg requested a review from stefpiatek September 16, 2026 12:37

p-j-smith reviewed Sep 17, 2026

View reviewed changes

ruaridhg and others added 4 commits September 17, 2026 14:30

Try to fix system test w import studies from raw message context

09a171d

Merge branch 'main' into 650-anonymisation-reads-from-queue

5855db6

Fix queue-declaration argument mismatch in cli

664b269

Add retry to AnonymisationPixlConsumer

b163d1c

ruaridhg requested a review from p-j-smith September 17, 2026 16:16

ruaridhg added 4 commits September 18, 2026 10:53

Add copilot suggestions

7d96ef9

Remove redundant input for anon message type from async consumer

7dffc13

Improve test coverage for subscriber covering error handling

9ce7679

Merge remote-tracking branch 'origin/main' into 650-anonymisation-rea…

e97ae06

…ds-from-queue

ruaridhg force-pushed the 650-anonymisation-reads-from-queue branch from 607eedb to e97ae06 Compare September 18, 2026 13:33

p-j-smith requested changes Sep 21, 2026

View reviewed changes

ruaridhg and others added 3 commits September 22, 2026 10:00

Merge branch 'main' into 650-anonymisation-reads-from-queue

2126628

Simplify max-priority config args and give Anon subscriber max-priority

1ad2a0d

Simplify max-priority config args and give Anon subscriber max-priority

7807637

ruaridhg requested a review from p-j-smith September 22, 2026 10:38

stefpiatek approved these changes Sep 22, 2026

View reviewed changes

ruaridhg added 2 commits September 22, 2026 16:26

Remove None as an option for parent context

b03f507

Replace Message with ImagingRequestMessage in docs

fb914cb



		PRIORITY_QUEUES = {"imaging-primary", "imaging-secondary"}
		MAX_QUEUE_PRIORITY = 5



		class PixlConsumer(PixlQueueInterface):
		class PixlConsumer[PixlMessage: ImagingRequestMessage](PixlQueueInterface):

Conversation

ruaridhg commented Aug 21, 2026 • edited by stefpiatek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Type of change

Suggested Checklist

Uh oh!

codecov Bot commented Aug 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stefpiatek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stefpiatek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

linear-code Bot commented Sep 17, 2026

Uh oh!

ruaridhg commented Sep 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

p-j-smith left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ruaridhg commented Sep 22, 2026

Uh oh!

stefpiatek left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stefpiatek commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ruaridhg commented Aug 21, 2026 •

edited by stefpiatek

Loading

codecov Bot commented Aug 21, 2026 •

edited

Loading

stefpiatek left a comment •

edited

Loading

ruaridhg commented Sep 17, 2026 •

edited

Loading

stefpiatek commented Sep 22, 2026 •

edited

Loading