[MRG] Add UPS and BWM to qrscp and UPS to findscu #837

sjswerdloff · 2023-06-16T15:23:12Z

Reference issue

Tasks

Unit tests added that reproduce issue or prove feature is working
Fix or feature added: Add UPS to findscu, Add minimal support for UPS and BWM to qrscp sufficient to allow testing of findscu
Documentation and examples updated (if relevant)
Unit tests passing and coverage at 100% after adding fix/feature (I think same coverage % as before)
Type annotations updated and passing with mypy (findscu has same issues as before my PR)
Apps updated and tested (if relevant)

…ontexts, bypassing query of database and returning empty list for MWL and UPS queries

previous commit fixed lack of UnifiedProcedurePresentationContexts but forgot to put in comment

added UPS SOP Classes/Presentation Contexts to test_findscu because they are showing up in qrscp now corrected search directory in test_storescu for it's recursive search for dicom files

… but with success)

sjswerdloff · 2023-06-16T15:59:53Z

When I use a lesser version of Sphinx, everything is fine in terms of building docs.
I thought I posted that in a previous comment.
When I try to update various dependencies that are listed in complaints by pip about latest Sphinx (7.0.1), they uninstall Spring 7 and install Sphinx 5.x

sjswerdloff · 2023-06-16T16:01:36Z

When I use a lesser version of Sphinx, everything is fine in terms of building docs.
I thought I posted that in a previous comment.
When I try to update various dependencies that are listed in complaints by pip about latest Sphinx (7.0.1), they uninstall Spring 7 and install Sphinx 5.x

but now I get:
WARNING: extlinks: Sphinx-6.0 will require a caption string to contain exactly one '%s' and all other '%' need to be escaped as '%%'.

which is a bit more informative.
But I don't think I added anything that is any different from what was there before (I basically did copy/paste/modify)

mrbean-bremen

Thanks, that looks quite good to me! I will wait some time if somebody else will have a look (@darcymason ?), and for the linter issue to be handled before merging this.

mrbean-bremen · 2023-06-16T17:00:39Z

pynetdicom/apps/qrscp/handlers.py

-            yield 0xC322, None
-
-        yield 0xFF00, response
+    if model.name not in ['Unified Procedure Step - Pull SOP Class' ,'Modality Worklist Information Model - FIND'] :


Probably better to use the keyword, which will surely not change, e.g.

if model.keyword not in ("UnifiedProcedureStepPull", "ModalityWorklistInformationModelFind"):

and maybe revert the logic for readability - that else branch is way down there

Great feedback. I am on it.

…ghtly older

darcymason · 2023-06-17T14:57:03Z

I will wait some time if somebody else will have a look (@darcymason ?), and for the linter issue to be handled before merging this.

I've had a quick look without significant comments - will try to do a proper review in the next couple of days.

using model.keyword rather than model.name, and inverted logic so that the action taken based on the truth of the conditional is immediately visible to the reviewer.

The lint failure is for long present spelling of these words, presumably following the dictate that the Kings English be used I tried using the --builtin 'en-GB_to_en-US' but that didn't work, so I just used the specific words and their spelling.

pydicom already has typing information in it

codecov · 2023-06-22T18:47:45Z

Codecov Report

Merging #837 (1b87f23) into master (78d4b95) will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master      #837   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           28        28           
  Lines         8639      8639           
=========================================
  Hits          8639      8639

mrbean-bremen · 2023-06-22T18:50:28Z

The checks now pass, only waiting for the review by @darcymason.

darcymason · 2023-06-19T13:20:26Z

docs/apps/qrscp.rst

+In addition, the ``qrscp`` application implements a Service Class Provider (SCP) for the
+:dcm:`Basic Modality Worklist<part04/chapter_K.html>`, and :dcm:`Unified Procedure Step<part04/Chapter_CC>`
+service classes, but currently will only return empty results (0 records)
+
 .. warning::


In reviewing again, I'm wondering about the 'safety' of returning empty results... I don't know much about DICOM communications (as opposed to files) and pynetdicom's internals, but is this a potential problem for another pynetdicom library user, who tries to use this service assuming it would work fully?

Empty results are a legitimate and frequent outcome when issuing a C-FIND-RQ
This happens alot in IHE-RO TDW-II profile because the SCU implementations often poll every five minutes and there isn't a match at the time.
It also happens for typical Q/R and for BWM/MWL.

However, I am intending to provide a means for the tester supplying UPS to provide responses to matching queries.
I was going to do that separately from qrscp, and do it in a personal but public repo with TDW-II plus examples.

The reason I put in the successful empty response was so there could be a basic communication test. There wasn't one for BWM/WLM. Which was already supported by findscu. If you want me to remove the minimal support for UPS CFIND from qrscp and will still approve the changes for findscu, I will create a new PR and cancel this one. But if I have to implement a fully functional example, then we should remove support for BWM/MWL from findscu until it too has a fully functional test available.

I will be happy to transfer any and all of my personal repo work on UPS to pynetdicom apps once I am happy with it. I realise that counts as big bang PRs, but it can be done one paired set of apps at a time (mutual testing... It takes two to tango). I have that minimal pair already for UPS Watch, although it is not "feature complete".
I'm also happy to maintain ownership of my TDW-II/UPS working examples.

If you want me to remove the minimal support for UPS CFIND from qrscp and will still approve the changes for findscu, I will create a new PR and cancel this one.

Your first statement that this is legitimate and happens commonly is enough for me, especially with the additional abilities available soon. As to whether in your own repo, or contributed here, I'll let @mrbean-bremen comment on that one, I don't know enough to have an opinion.

Awesome.
There are a handful of RT 3rd party vendors eager to see the example of a findscu that supports UPS. Having this in the main (master) branch will make it easier for them.
Thank you

mrbean-bremen · 2023-06-23T05:09:10Z

If you want me to remove the minimal support for UPS CFIND from qrscp and will still approve the changes for findscu, I will create a new PR and cancel this one.

I'm fine with this - while I'm also not an expert in DICOM communication, I think it is a good addition - thank you!

sjswerdloff · 2023-06-23T05:54:17Z

If you want me to remove the minimal support for UPS CFIND from qrscp and will still approve the changes for findscu, I will create a new PR and cancel this one.

I'm fine with this - while I'm also not an expert in DICOM communication, I think it is a good addition - thank you!

WooHoo!

sjswerdloff added 14 commits June 3, 2023 19:44

add support for Unified Procedure Step to findscu

a992e5e

adding sample UPS Query as DICOM binary file

ab5f0c1

cleanup

d39cc07

incorporated minimal support for MWL and UPS model and presentation c…

0645bf5

…ontexts, bypassing query of database and returning empty list for MWL and UPS queries

Sample MWL CFind Query as DICOM binary

d3d82e5

removed trailing whitespace in import of QueryRetrieve

dfe1c7c

previous commit fixed lack of UnifiedProcedurePresentationContexts but forgot to put in comment

fixed attribute hex values

05e1d22

fixed attribute hex valuesw

2d7f476

Merge branch 'add_ups_mwl_to_qrscp' into add_ups_to_findscu

73d5a98

eliminated trailing whitespace in import statement

a50f3c5

removed dicom files that were interfering with tests

3f6bdff

added UPS SOP Classes/Presentation Contexts to test_findscu because they are showing up in qrscp now corrected search directory in test_storescu for it's recursive search for dicom files

add test of UPS option (-U)

d409981

add UPS to documentation for findscu

9055be9

included support for MWL and UPS, such as it is (no records returned,…

aeb234f

… but with success)

sjswerdloff changed the title ~~[WIP] Add UPS and BWM to qrscp and UPS to findscu~~ [MRG] Add UPS and BWM to qrscp and UPS to findscu Jun 16, 2023

sjswerdloff marked this pull request as ready for review June 16, 2023 15:38

corrected description by adding second grave accent prior to qrscp

81c1db4

applied black

4d1f80d

mrbean-bremen approved these changes Jun 17, 2023

View reviewed changes

black is the new black. used black 23.2 instead of something just sli…

2569643

…ghtly older

sjswerdloff added 4 commits June 17, 2023 17:29

updated per comment from @mrbean-bremen

2704834

using model.keyword rather than model.name, and inverted logic so that the action taken based on the truth of the conditional is immediately visible to the reviewer.

eliminated direct access to pydicom for typing

4a69f8d

pydicom already has typing information in it

Merge branch 'master' into add_ups_mwl_to_qrscp

3117344

sjswerdloff mentioned this pull request Jun 22, 2023

Intermittent failing test TestState08::test_evt14 #827

Closed

Merge branch 'master' into add_ups_mwl_to_qrscp

1b87f23

darcymason approved these changes Jun 22, 2023

View reviewed changes

mrbean-bremen merged commit a5beba7 into pydicom:master Jun 23, 2023
12 checks passed

sjswerdloff deleted the add_ups_mwl_to_qrscp branch June 24, 2023 03:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MRG] Add UPS and BWM to qrscp and UPS to findscu #837

[MRG] Add UPS and BWM to qrscp and UPS to findscu #837

sjswerdloff commented Jun 16, 2023

sjswerdloff commented Jun 16, 2023

sjswerdloff commented Jun 16, 2023

mrbean-bremen left a comment

mrbean-bremen Jun 16, 2023

sjswerdloff Jun 17, 2023

darcymason commented Jun 17, 2023

codecov bot commented Jun 22, 2023 •

edited

Loading

mrbean-bremen commented Jun 22, 2023

darcymason Jun 19, 2023

sjswerdloff Jun 22, 2023

darcymason Jun 22, 2023

sjswerdloff Jun 23, 2023

mrbean-bremen commented Jun 23, 2023

sjswerdloff commented Jun 23, 2023

[MRG] Add UPS and BWM to qrscp and UPS to findscu #837

[MRG] Add UPS and BWM to qrscp and UPS to findscu #837

Conversation

sjswerdloff commented Jun 16, 2023

Reference issue

Tasks

sjswerdloff commented Jun 16, 2023

sjswerdloff commented Jun 16, 2023

mrbean-bremen left a comment

Choose a reason for hiding this comment

mrbean-bremen Jun 16, 2023

Choose a reason for hiding this comment

sjswerdloff Jun 17, 2023

Choose a reason for hiding this comment

darcymason commented Jun 17, 2023

codecov bot commented Jun 22, 2023 • edited Loading

Codecov Report

mrbean-bremen commented Jun 22, 2023

darcymason Jun 19, 2023

Choose a reason for hiding this comment

sjswerdloff Jun 22, 2023

Choose a reason for hiding this comment

darcymason Jun 22, 2023

Choose a reason for hiding this comment

sjswerdloff Jun 23, 2023

Choose a reason for hiding this comment

mrbean-bremen commented Jun 23, 2023

sjswerdloff commented Jun 23, 2023

codecov bot commented Jun 22, 2023 •

edited

Loading