Pix 49 log skip error in db by ruaridhg · Pull Request #658 · SAFEHR-data/PIXL

ruaridhg · 2026-08-12T09:37:07Z

Description

Fixes #651: Replace _filter_exported_messages with _filter_exported_or_skipped_messages in the CLI.

Updated the database model to add skip_reasons column with JSONB and autogenerated alembic migration.

In orthanc-anon, pass skip_reasons to update_db_with_skip_failure_reason within pixl_dcmd which then calls record_skip_reasons_for_study in pixl_dcmd _database.py.

Added test_reimport_of_previously_skipped_image, test_update_db_with_skip_failure_reason and test_record_skip_reasons_for_study as well as test_clean_dicom_image_pixels_encapsulates_compressed_pixel_data (to increase code coverage)

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

…ailure_reason() within pixl_dcmd and orthanc-anon calls that func before raising PIXLDiscardError

…new column

linear-code · 2026-08-12T09:37:11Z

PIX-49

codecov · 2026-08-12T09:37:48Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 85.25%. Comparing base (d00b7c7) to head (b3942ad).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #658      +/-   ##
==========================================
- Coverage   88.09%   85.25%   -2.85%     
==========================================
  Files          81       77       -4     
  Lines        3890     3656     -234     
==========================================
- Hits         3427     3117     -310     
- Misses        463      539      +76

☔ 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.

…d docs for this

HChughtai

Nice, looks like it's almost there. The database aspects all look good, and I've left a bunch of comments that I think are worth sorting.

The only other thing to think about is about updating the system tests so that its tested end-to-end.

(And as mentioned in the meeting earlier this week, let's get @stefpiatek's review too whilst we're getting up to speed)

HChughtai · 2026-08-14T09:40:53Z

+        update_db_with_skip_failure_reason(
+            project_name=project_name,
+            study_info=study_info,
+            skip_reasons=dict(skipped_instance_counts),
+        )


If this fails (e.g due to a db issue), the wrong message gets passed on back to the calling def _anonymise_study_and_upload(

Right now, I think it'll fallback to except DBAPIError as e: which isn't catastrophic but means PixlDiscardError doesn't get raised and the wrong failure reason is recorded and sent via telemetry. Or if another error is raised it could get handled by the except Exception as e: with whatever error gets passed back. That means we lose the info saying "all instances have been skipped" entirely.

So I think worth wrapping in a try-except so that even if the database write has an issue, the current run and metrics aren't affected.

HChughtai · 2026-08-14T10:00:08Z



-def _filter_exported_messages(
+def _filter_exported_or_skipped_messages(


Good that we're making it clear here that there are now two reasons why a study may not be added to the imaging queue. However, we should propagate that change back up to def populate_queue_and_db( where it currently says logger.info("Filtering out exported images and uploading new ones to the database"), as well as to functions and docstrings in between.

And possibly out of scope for this PR, I think some additional logging about what studies are skipped and why may be useful - e.g. this study was skipped because it was exported already, or has been skipped for x reason. Or in this function just log the overall numbers for each reason.

HChughtai · 2026-08-14T10:29:13Z

+    PixlSession = sessionmaker(engine)
+    with PixlSession() as pixl_session, pixl_session.begin():
+        existing_image = get_unexported_image(project_slug, study_info, pixl_session)
+        existing_image.skip_reasons = skip_reasons


We should keep in mind for #659 that this is currently set, and then never unset. So when we allow skipped studies to be retried, we should ensure that the column in the database can be cleared.

HChughtai · 2026-08-14T10:39:18Z

    assert np.all(compare_clean_region_with_zeros)


+def test_clean_dicom_image_pixels_encapsulates_compressed_pixel_data(


Should this be in this PR?

HChughtai · 2026-08-14T10:49:11Z

+    # Filtering-level: previously skipped images are not queued again,
+    # nor are already-exported images
+    assert "234" not in output.accession_number.to_numpy()
+    assert "123" not in output.accession_number.to_numpy()


This would pass even if "345" was dropped too - e.g. if output was empty and everything was dropped. So worth asserting that the non-skipped case is retained and the length of output is what you expect.

HChughtai · 2026-08-14T10:54:20Z

+    extract = rows_in_session.query(Extract).one()
+    previously_skipped_image = (
+        rows_in_session.query(Image)
+        .filter(Image.extract == extract, Image.accession_number == "234")
+        .one()
+    )
+    skip_reasons = {"DICOM instance discarded as series has too few instances": 3}
+    previously_skipped_image.skip_reasons = skip_reasons
+    rows_in_session.commit()


I think setting up the data here is OK for this test, but if we start having multiple tests that need it (e.g. as part of #659) then we should look at moving this to a fixture that builds on def rows_in_session(db_session) -> Session:

HChughtai · 2026-08-14T11:05:56Z

+
+    update_db_with_skip_failure_reason("test-project", study_info, skip_reasons)
+
+    assert recorded_calls == [("test-project", study_info, skip_reasons)]


This isn't really testing anything as you're mocking the only function that update_db_with_skip_failure_reason is calling so no database calls are actually getting made. The assertion is just testing that the args are forwarded.

I'm assuming the function was set up like that to follow the existing structure in the code, and the test is here to improve reported coverage? My thoughts are that this test can be removed as you're doing the actual testing in def test_record_skip_reasons_for_study

ruaridhg added 3 commits August 7, 2026 15:34

Added record_skip_reasons_for_study() called by update_db_with_skip_f…

8028f31

…ailure_reason() within pixl_dcmd and orthanc-anon calls that func before raising PIXLDiscardError

added a skip_reasons JSONB column to Image database entry

951bc2e

Added an Alembic migration in pixl_imaging/alembic/versions/ for the …

7fd670c

…new column

ruaridhg and others added 4 commits August 12, 2026 15:20

Removed manual alembic file and added autogenerated file

74820dc

Merge branch 'main' into PIX-49-log-skip-error-in-DB

24b8ce6

Add filtering exported or skipped messages in cli and update tests an…

d94d8af

…d docs for this

Try to improve code coverage

b3942ad

ruaridhg marked this pull request as ready for review August 12, 2026 15:59

ruaridhg requested a review from HChughtai August 12, 2026 16:00

HChughtai reviewed Aug 14, 2026

View reviewed changes

Navigation Menu

Uh oh!

Pix 49 log skip error in db - #658

Pix 49 log skip error in db#658
ruaridhg wants to merge 7 commits into
mainfrom
PIX-49-log-skip-error-in-DB

ruaridhg commented Aug 12, 2026 •

edited

Loading

Uh oh!

linear-code Bot commented Aug 12, 2026

Uh oh!

codecov Bot commented Aug 12, 2026 •

edited

Loading

Uh oh!

HChughtai left a comment

Uh oh!

HChughtai Aug 14, 2026 •

edited

Loading

Uh oh!

HChughtai Aug 14, 2026

Uh oh!

HChughtai Aug 14, 2026

Uh oh!

HChughtai Aug 14, 2026

Uh oh!

HChughtai Aug 14, 2026

Uh oh!

HChughtai Aug 14, 2026

Uh oh!

HChughtai Aug 14, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants



		def _filter_exported_messages(
		def _filter_exported_or_skipped_messages(

		assert np.all(compare_clean_region_with_zeros)


		def test_clean_dicom_image_pixels_encapsulates_compressed_pixel_data(


		update_db_with_skip_failure_reason("test-project", study_info, skip_reasons)

		assert recorded_calls == [("test-project", study_info, skip_reasons)]

Uh oh!

Conversation

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

Uh oh!

Description

Type of change

Suggested Checklist

Uh oh!

linear-code Bot commented Aug 12, 2026

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

HChughtai left a comment

Choose a reason for hiding this comment

Uh oh!

HChughtai Aug 14, 2026 • 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!

HChughtai Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

HChughtai Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

HChughtai Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

HChughtai Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

HChughtai Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

HChughtai Aug 14, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ruaridhg commented Aug 12, 2026 •

edited

Loading

codecov Bot commented Aug 12, 2026 •

edited

Loading

HChughtai Aug 14, 2026 •

edited

Loading