650 anonymisation reads from queue

[ruaridhg]

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

[codecov]

Codecov Report

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

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.30%   +0.88%     
==========================================
  Files          81       81              
  Lines        3972     4066      +94     
==========================================
+ Hits         3512     3631     +119     
+ 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.

[stefpiatek]

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

[ruaridhg]

@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

[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?

[ruaridhg]

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]

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

[p-j-smith]

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

ah yeah, good point, we want to keep the threading then

[p-j-smith]

We'll need to make sure we wait for de-identification to finish before we ack the message. To avoid blocking the thread, we could switch to an async consumer and use asyncios run_in_executor to submit the work to our threadpool. Unless there's a better way @stefpiatek?

[p-j-smith]

looks good @ruaridhg, the two consumer classes could probably be merged into one now, but not blocking