Revert "Chore/papi 4760 stop using download url"#514
Conversation
nahua-aignx
requested a review
from helmut-hoffer-von-ankershoffen
as a code owner
|
Claude finished @nahua-aignx's task in 5m 6s —— View job PR Review Complete ✅Reviewing revert PR #514: Revert "Chore/papi 4760 stop using download url" SummaryThis PR cleanly reverts PR #507 which attempted to migrate from using the deprecated Verification Results✅ All Critical Checks Passed
What's Being RevertedRemoved Code (Going Back To)platform/resources/runs.py (-177 lines):
application/_download.py (-53 lines):
application/_service.py (-14 lines):
platform/init.py (-1 export):
Removed Tests (-910 lines)tests/platform/resources/runs_test.py (-487 lines):
tests/application/download_test.py (-368 lines):
tests/application/service_test.py (-55 lines):
Review Findings✅ No Issues FoundThis is a clean, correct revert with no issues identified:
Architecture Compliance ✅
Why This Revert Is Safe
CI/CD ImpactExpected CI Behavior
RecommendationAlready applied: PR has label Medical Device Software ComplianceThis revert maintains compliance requirements:
Recommendations✅ Approve This PRThis is a well-executed revert:
Future Work (After PAPI-4868)When re-reverting later:
ApprovalStatus: ✅ APPROVED (already approved by @olivermeyer) My Review: ✅ APPROVED
|
|
There was a problem hiding this comment.
Pull request overview
This PR reverts prior work (#507) that stopped using download_url for output artifacts, restoring the SDK’s previous behavior of consuming artifact.download_url directly for previews and downloads until the new backend endpoint (PAPI-4868) is production-ready.
Changes:
- Removes the
Artifacthelper and the “resolve presigned URL via redirect” logic from the platformRunresource. - Reverts application download flow to rely on
artifact.download_url(removing the callback-based resolver path). - Updates GUI artifact preview/download actions to use
artifact.download_urldirectly.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/aignostics/platform/resources/runs.py |
Removes Artifact and switches item artifact downloads back to artifact.download_url. |
src/aignostics/platform/__init__.py |
Stops exporting Artifact from the platform module. |
src/aignostics/application/_download.py |
Removes URL resolver/callback support and downloads artifacts via artifact.download_url. |
src/aignostics/application/_service.py |
Removes passing the artifact URL resolver callback into download_available_items. |
src/aignostics/application/_gui/_page_application_run_describe.py |
Uses artifact.download_url for preview/download; adjusts download button handling. |
tests/aignostics/platform/resources/runs_test.py |
Drops tests covering Artifact and presigned URL resolution behavior. |
tests/aignostics/application/service_test.py |
Removes test that validated the callback closure delegation. |
tests/aignostics/application/download_test.py |
Removes most tests for result-artifact downloading utilities, leaving only URL-to-file tests. |
Comments suppressed due to low confidence (2)
src/aignostics/application/_download.py:268
download_item_artifactpassesartifact.download_urldirectly intodownload_file_with_progresswithout validating it's set. If an output artifact is present but hasdownload_url=None(or missing), this will fail later insiderequests.getwith a confusing error. Add an explicit check (and either skip with a warning or raise a clear ValueError) before attempting the download.
metadata = artifact.metadata or {}
metadata_checksum = metadata.get("checksum_base64_crc32c", "") or metadata.get("checksum_crc32c", "")
if not metadata_checksum:
message = f"No checksum metadata found for artifact {artifact.name}"
logger.error(message)
raise ValueError(message)
artifact_path = (
destination_directory
/ f"{prefix}{sanitize_path_component(artifact.name)}{get_file_extension_for_artifact(artifact)}"
)
if artifact_path.exists():
checksum = crc32c.CRC32CHash()
with open(artifact_path, "rb") as f:
while chunk := f.read(APPLICATION_RUN_FILE_READ_CHUNK_SIZE):
checksum.update(chunk)
existing_checksum = base64.b64encode(checksum.digest()).decode("ascii")
if existing_checksum == metadata_checksum:
logger.trace("File {} already exists with correct checksum", artifact_path)
return
download_file_with_progress(
progress,
artifact.download_url,
artifact_path,
metadata_checksum,
download_progress_queue,
download_progress_callable,
)
src/aignostics/platform/resources/runs.py:407
downloaded_at_least_one_artifactis only set toTruein the "file does not exist" branch. If an existing file has a checksum mismatch (the "Resume download" path), the code still downloads but will print/log "already present" at the end, which is misleading. Set the flag whenever a download is actually performed (both fresh and resumed).
if file_path.exists():
file_checksum = calculate_file_crc32c(file_path)
if file_checksum != checksum:
logger.trace("Resume download for {} to {}", artifact.name, file_path)
print(f"> Resume download for {artifact.name} to {file_path}") if print_status else None
else:
continue
else:
downloaded_at_least_one_artifact = True
logger.trace("Download for {} to {}", artifact.name, file_path)
print(f"> Download for {artifact.name} to {file_path}") if print_status else None
# if file is not there at all or only partially downloaded yet
download_file(artifact.download_url, str(file_path), checksum)
if downloaded_at_least_one_artifact:
logger.trace("Downloaded results for item: {} to {}", item.external_id, item_dir)
print(f"Downloaded results for item: {item.external_id} to {item_dir}") if print_status else None
else:
logger.trace("Results for item: {} already present in {}", item.external_id, item_dir)
print(f"Results for item: {item.external_id} already present in {item_dir}") if print_status else None
| @@ -365,24 +364,19 @@ async def start_download() -> None: | |||
| else: | |||
| ui.notify("Download completed.", type="positive") | |||
| show_in_file_manager(str(results_folder)) | |||
| except Exception as e: | |||
| logger.exception( | |||
| "Download failed for run {} (qupath_project={}, marimo={}, folder={})", | |||
| run.run_id, | |||
| current_qupath_project, | |||
| current_marimo, | |||
| current_folder, | |||
| ) | |||
| except ValueError as e: | |||
| ui.notify(f"Download failed: {e}", type="negative", multi_line=True) | |||
| finally: | |||
| progress_timer.deactivate() | |||
| progress_state["queue"] = None | |||
| download_button.props(remove="loading") | |||
| download_button.enable() | |||
| download_item_status.set_visibility(False) | |||
| download_item_progress.set_visibility(False) | |||
| download_artifact_status.set_visibility(False) | |||
| download_artifact_progress.set_visibility(False) | |||
| return | |||
| progress_timer.deactivate() | |||
| progress_state["queue"] = None | |||
| download_button.props(remove="loading") | |||
| download_button.enable() | |||
| download_item_status.set_visibility(False) | |||
| download_item_progress.set_visibility(False) | |||
| download_artifact_status.set_visibility(False) | |||
| download_artifact_progress.set_visibility(False) | |||
There was a problem hiding this comment.
In start_download, the cleanup that re-enables the download button / removes the loading state is no longer in a finally. If Service.application_run_download_static raises (including the handled ValueError path), the function returns early without resetting the UI, leaving the button stuck disabled/loading and progress widgets potentially visible. Restore a try/finally (or ensure cleanup runs on all exit paths) and consider catching broader exceptions for user-facing notification while still logging details.
| from aignostics.application._download import download_url_to_file_with_progress, extract_filename_from_url | ||
| from aignostics.application._models import DownloadProgress, DownloadProgressState |
There was a problem hiding this comment.
This change drops most unit coverage for result artifact downloading (e.g., download_available_items, download_item_artifact, checksum handling, and skip/resume behavior). These paths are still core to ApplicationService.application_run_download, so removing the tests risks regressions and may impact the project's coverage gate. Consider restoring the relevant tests (updated to match the reverted API) rather than removing them wholesale.
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| progress_timer.deactivate() | ||
| progress_state["queue"] = None | ||
| download_button.props(remove="loading") | ||
| download_button.enable() | ||
| download_item_status.set_visibility(False) | ||
| download_item_progress.set_visibility(False) | ||
| download_artifact_status.set_visibility(False) | ||
| download_artifact_progress.set_visibility(False) | ||
| return | ||
| progress_timer.deactivate() | ||
| progress_state["queue"] = None | ||
| download_button.props(remove="loading") | ||
| download_button.enable() | ||
| download_item_status.set_visibility(False) | ||
| download_item_progress.set_visibility(False) | ||
| download_artifact_status.set_visibility(False) | ||
| download_artifact_progress.set_visibility(False) |
There was a problem hiding this comment.
Bug: The start_download function only handles ValueError, leaving the UI in a permanently disabled state if other common exceptions (e.g., network errors) occur during download.
Severity: HIGH
Suggested Fix
Restore the finally block to ensure UI cleanup code (disabling the timer, clearing the queue, and resetting the button state) runs regardless of whether the download succeeds or fails. Broaden the except block to catch other potential exceptions like RuntimeError and requests.HTTPError, not just ValueError.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: src/aignostics/application/_gui/_page_application_run_describe.py#L367-L379
Potential issue: In the `start_download` function, the exception handling was changed to
only catch `ValueError`. Other common exceptions that can occur during the download
process, such as `RuntimeError` or `requests.HTTPError` from network failures, are not
caught. When one of these unhandled exceptions occurs, the cleanup logic is skipped.
This leaves the UI in a permanently stuck state: the download button remains disabled
and in a loading state, and the progress timer continues to run indefinitely. The user
cannot retry the download and must reload the page to restore functionality.
Did we get this right? 👍 / 👎 to inform future reviews.
Codecov Report❌ Patch coverage is
|
Reverts #507
We can re-revert this later after https://aignx.atlassian.net/browse/PAPI-4868 is merged and released to production.
A SAMIA API bug would prevent the new download feature in python-sdk from working. So the decision is to revert the feature addition, fix SAMIA, release SAMIA to production, then re-introduce the feature again.