Skip to content

Conversation

@milanmlft
Copy link
Member

@milanmlft milanmlft commented Jul 8, 2025

Closes #606

Adds the TreApiUploader class to handle uploads to the ARC TRE API ingress

@milanmlft milanmlft requested review from Copilot and stefpiatek July 8, 2025 16:29

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jul 9, 2025

Codecov Report

❌ Patch coverage is 70.66667% with 22 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.54%. Comparing base (8ca52a9) to head (fede2e3).

Files with missing lines Patch % Lines
pixl_core/src/core/uploader/_treapi.py 69.86% 22 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #608      +/-   ##
==========================================
- Coverage   87.89%   87.54%   -0.35%     
==========================================
  Files          76       77       +1     
  Lines        3619     3694      +75     
==========================================
+ Hits         3181     3234      +53     
- Misses        438      460      +22     

☔ View full report in Codecov by Sentry.
📢 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.

@milanmlft milanmlft force-pushed the milanmlft/606-add-api-uploader branch from 6215016 to dabc3df Compare July 9, 2025 11:42
@milanmlft milanmlft requested a review from Copilot July 9, 2025 15:00
@milanmlft milanmlft mentioned this pull request Jul 9, 2025
13 tasks
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces support for uploading to the ARC TRE API, complete with a new Uploader subclass and accompanying tests, and also refactors some existing FTPS tests and registration.

  • Implements TreApiUploader and a _create_zip_archive helper to handle DICOM and Parquet uploads via the TRE API.
  • Adds a full test suite for the TRE API uploader (test_treapi.py) and adjusts FTPS tests to use a single uid.
  • Registers TreApiUploader in the uploader factory and fixes a typo in the module doc.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File Description
pixl_core/src/core/uploader/_treapi.py New TreApiUploader class with send, upload, flush methods and zip creation helper
pixl_core/tests/uploader/test_treapi.py Unit tests covering token validation, DICOM/Parquet upload flows, and the zip utility
pixl_core/src/core/uploader/init.py Added TreApiUploader to get_uploader factory and corrected a typo
pixl_core/tests/uploader/test_ftps.py Refactored test_update_exported_and_save to use a single uid variable
Comments suppressed due to low confidence (2)

pixl_core/src/core/uploader/_treapi.py:167

  • Add tests for the upload failure path when response.status_code is not HTTP_CREATED to ensure RuntimeError is raised with the expected message.
            if response.status_code != HTTP_CREATED:

pixl_core/src/core/uploader/_treapi.py:197

  • Add tests for the flush failure path when response.status_code is not HTTP_CREATED to validate error handling in flush().
            if response.status_code != HTTP_CREATED:

milanmlft and others added 3 commits July 9, 2025 16:06
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
We should be able to test private members
@milanmlft milanmlft marked this pull request as ready for review July 9, 2025 15:26
@milanmlft
Copy link
Member Author

Codecov Report

Attention: Patch coverage is 66.66667% with 26 lines in your changes missing coverage. Please review.

Project coverage is 85.07%. Comparing base (42284de) to head (e1cab35).

Files with missing lines Patch % Lines
pixl_core/src/core/uploader/_treapi.py 66.23% 26 Missing ⚠️
Additional details and impacted files

☔ View full report in Codecov by Sentry. 📢 Have feedback on the report? Share it here.
🚀 New features to boost your workflow:

Patch coverage is low, mainly because I don't want to mock out the whole TRE API... 🫠
Though I agree that the current tests are a bit limited in what they actually tests.

@stefpiatek
Copy link
Contributor

stefpiatek commented Dec 30, 2025

Failing when uploading a 150 MB zip of parquet files either with an SSL error or bad gateway. Had a quick try of a couple of things to debug. Unsure if anything was taken down over christmas on the TRE side. Manually copied over file to TRE so not a huge rush.

  • timeout is longer than it takes to error so don't think its a timeout issue
  • tried seeking to start of byte stream before uploading but had the same errors
export-api-1  | 2025-12-30T10:58:43.788497128Z   File "/app/pixl_export/src/pixl_export/main.py", line 83, in export_patient_data
export-api-1  | 2025-12-30T10:58:43.788500322Z     parquet_export.upload()
export-api-1  | 2025-12-30T10:58:43.788508226Z   File "/app/pixl_core/src/core/exports.py", line 140, in upload
export-api-1  | 2025-12-30T10:58:43.788510891Z     uploader.upload_parquet_files(self)
export-api-1  | 2025-12-30T10:58:43.788513362Z   File "/app/pixl_core/src/core/uploader/_treapi.py", line 105, in upload_parquet_files
export-api-1  | 2025-12-30T10:58:43.788515913Z     self.send_via_api(BytesIO(zip_file.read_bytes()), zip_file.name)
export-api-1  | 2025-12-30T10:58:43.788518448Z   File "/app/pixl_core/src/core/uploader/_treapi.py", line 124, in send_via_api
export-api-1  | 2025-12-30T10:58:43.788521087Z     self._upload_file(data, filename)
export-api-1  | 2025-12-30T10:58:43.788523530Z   File "/app/pixl_core/src/core/uploader/_treapi.py", line 169, in _upload_file
export-api-1  | 2025-12-30T10:58:43.788526053Z     raise RuntimeError(msg) from e
export-api-1  | 2025-12-30T10:58:43.788528556Z RuntimeError: Failed to upload file 2025-12-11t16-27-58-026761.zip: HTTPSConnectionPool(host='api.tre.arc.ucl.ac.uk', port=443): Max retries exceeded with url: /v0/airlock/upload/2025-12-11t16-27-58-026761.zip (Caused by SSLError(SSLEOFError(8, 'EOF occurred in violation of protocol (_ssl.c:2406)')))
export-api-1  | 2025-12-30T10:59:47.745606118Z   File "/app/pixl_export/src/pixl_export/main.py", line 83, in export_patient_data
export-api-1  | 2025-12-30T10:59:47.745608722Z     parquet_export.upload()
export-api-1  | 2025-12-30T10:59:47.745611210Z   File "/app/pixl_core/src/core/exports.py", line 140, in upload
export-api-1  | 2025-12-30T10:59:47.745613848Z     uploader.upload_parquet_files(self)
export-api-1  | 2025-12-30T10:59:47.745616368Z   File "/app/pixl_core/src/core/uploader/_treapi.py", line 105, in upload_parquet_files
export-api-1  | 2025-12-30T10:59:47.745618992Z     self.send_via_api(BytesIO(zip_file.read_bytes()), zip_file.name)
export-api-1  | 2025-12-30T10:59:47.745621550Z   File "/app/pixl_core/src/core/uploader/_treapi.py", line 124, in send_via_api
export-api-1  | 2025-12-30T10:59:47.745624125Z     self._upload_file(data, filename)
export-api-1  | 2025-12-30T10:59:47.745626561Z   File "/app/pixl_core/src/core/uploader/_treapi.py", line 169, in _upload_file
export-api-1  | 2025-12-30T10:59:47.745629105Z     raise RuntimeError(msg) from e
export-api-1  | 2025-12-30T10:59:47.745631651Z RuntimeError: Failed to upload file 2025-12-11t16-27-58-026761.zip: 502 Server Error: Bad Gateway for url: https://api.tre.arc.ucl.ac.uk/v0/airlock/upload/2025-12-11t16-27-58-026761.zip

"""
zip_content = get_study_zip_archive(study_id)
self.send_via_api(zip_content, study_tags.pseudo_anon_image_id)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be

Suggested change
self.send_via_api(zip_content, study_tags.pseudo_anon_image_id)
self.send_via_api(zip_content, f"{study_tags.pseudo_anon_image_id}.zip")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Uploader for ARC TRE

6 participants