Check the study UID in Orthanc Raw is the same as in VNA / PACS [force-system-test]

p-j-smith commented 2 months ago

Description

Related to #486.

For #486, we need to compare the SOPInstanceUIDs in Orthanc Raw against those in the VNA / PACS. However, when retrieving data, the StudyInstanceUID, and SeriesInstanceUID, and SOPInstanceUID values are modified by Orthanc Raw.

Fixes #496

keep StudyInstanceUID, SeriesInstanceUID, and SOPInstanceUID when setting the project name private tag in Orthanc Raw. Also set Force=True to allow us to keep these UIDs
set OverwriteInstance: True in the PIXL Raw config so that if an instance with an existing SOPInstanceUID is uploaded, the previous instance is overwritten (otherwise we'd need to manually delete the old instance after setting the project name tag)
Add test to check the StudyInstanceUID, SeriesInstanceUID, and SOPInstanceUID are correct in Orthanc raw after retrieving a study and setting the project name tag
update the config for Orthanc Raw in the imaging API tests to be the same as the production config

Type of change

Please delete options accordingly to the description.

[x] 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

[x] 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 PR review to UCLH-Foundtry/arc-dev
[ ] I have adddressed and marked as resolved all the review comments in my PR.
[ ] Finally, I have selected squash and merge

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 85.90%. Comparing base (d71ce8f) to head (5c0f5b3). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #495 +/- ## ========================================== + Coverage 85.79% 85.90% +0.10% ========================================== Files 86 86 Lines 3991 4022 +31 ========================================== + Hits 3424 3455 +31 Misses 567 567 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

p-j-smith commented 2 months ago

I'm not sure why the system tests are being skipped? I tried rerunning them but they were skipped again

mxochicale commented 2 months ago

I'm not sure why the system tests are being skipped? I tried rerunning them but they were skipped again

Does this tests need to be passing locally by going to (tests and then ./run-system-test.sh)? As I tried and I have this log:

2024-09-04 16:07:54.866 | ERROR    | system_test:_check_dcm_tags_from_zip:179 - TEMPORARILY IGNORE: tag value b'test-extract-uclh-omop-cdm' should be of type str, but is of type bytes
============================================================================================= short test summary info =============================================================================================
FAILED system_test.py::TestFtpsUpload::test_ftps_dicom_upload - assert {('ANONYMIZED', 'AP')} == {('ANONYMIZED...'include123')}
===================================================================================== 1 failed, 4 passed in 374.44s (0:06:14) =====================================================================================
(pixlVE) mxochicale@precision-5570:~/Desktop/saferHR/repositories/PIXL/test$ ./run-system-test.sh stop

p-j-smith commented 2 months ago

Does this tests need to be passing locally by going to (tests and then ./run-system-test.sh)? As I tried and I have this log:

2024-09-04 16:07:54.866 | ERROR | system_test:_check_dcm_tags_from_zip:179 - TEMPORARILY IGNORE: tag value b'test-extract-uclh-omop-cdm' should be of type str, but is of type bytes ============================================================================================= short test summary info ============================================================================================= FAILED system_test.py::TestFtpsUpload::test_ftps_dicom_upload - assert {('ANONYMIZED', 'AP')} == {('ANONYMIZED...'include123')} ===================================================================================== 1 failed, 4 passed in 374.44s (0:06:14) ===================================================================================== (pixlVE) mxochicale@precision-5570:~/Desktop/saferHR/repositories/PIXL/test$ ./run-system-test.sh stop

Oh that's this issue, which I thought would be fixed as part of https://github.com/SAFEHR-data/PIXL/pull/489 😢 Perhaps we should further increase PIXL_DICOM_TRANSFER_TIMEOUT and ORTHANC_RAW_EXTRA_STABLE_SECONDS?

mxochicale commented 2 months ago

Oh that's this issue, which I thought would be fixed as part of #489 😢 Perhaps we should further increase PIXL_DICOM_TRANSFER_TIMEOUT and ORTHANC_RAW_EXTRA_STABLE_SECONDS?

Thanks, just wanted to do a quick test and tried PIXL_DICOM_TRANSFER_TIMEOUT=400 and ORTHANC_RAW_EXTRA_STABLE_SECONDS=400 but seems to produce some errors. I leave logs in case are useful.


../pytest-pixl/src/pytest_pixl/helpers.py:111: AssertionError
============================================================================================= short test summary info =============================================================================================
ERROR system_test.py::TestFtpsUpload::test_ftps_parquet_upload - AssertionError: Condition at_least_n_studies_exported was not achieved even after 211 seconds
ERROR system_test.py::TestFtpsUpload::test_ftps_radiology_linker_upload - AssertionError: Condition at_least_n_studies_exported was not achieved even after 211 seconds
ERROR system_test.py::TestFtpsUpload::test_ftps_dicom_upload - AssertionError: Condition at_least_n_studies_exported was not achieved even after 211 seconds
ERROR test_parquet_exports.py::test_public_parquet - AssertionError: Condition at_least_n_studies_exported was not achieved even after 211 seconds
===================================================================================== 1 passed, 4 errors in 357.37s (0:05:57)

stefpiatek commented 2 months ago

Thanks, just wanted to do a quick test and tried PIXL_DICOM_TRANSFER_TIMEOUT=400 and ORTHANC_RAW_EXTRA_STABLE_SECONDS=400 but seems to produce some errors. I leave logs in case are useful.

Ah this would be because the test waits 120 seconds for a study to be expected, but it will take 800 seconds (PIXL_DICOM_TRANSFER_TIMEOUT + ORTHANC_RAW_EXTRA_STABLE_SECONDS) before it will move from orthanc-raw to orthanc-anon

SAFEHR-data / PIXL