Skip to content

Revert "Chore/papi 4760 stop using download url" #514

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nahua-aignx merged 1 commit into from
Mar 27, 2026
Merged

Revert "Chore/papi 4760 stop using download url" #514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 2 additions & 53 deletions src/aignostics/application/_download.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
import requests
from loguru import logger

from aignostics.platform import ItemOutput, ItemState, OutputArtifactElement, Run, generate_signed_url
from aignostics.platform import ItemOutput, ItemState, Run, generate_signed_url
from aignostics.utils import sanitize_path_component

from ._models import DownloadProgress, DownloadProgressState
Expand Down Expand Up @@ -146,44 +146,6 @@ def update_progress(
download_progress_queue.put_nowait(progress)


def _resolve_artifact_url(
artifact: OutputArtifactElement,
run_id: str,
get_artifact_download_url: Callable[[str, str], str] | None,
) -> str | None:
"""Resolve the download URL for an artifact.

Tries the new file endpoint first (via get_artifact_download_url), then falls back to
the deprecated download_url field on the artifact if the endpoint fails.

Args:
artifact: The artifact object with optional output_artifact_id and download_url.
run_id: The run ID, passed to get_artifact_download_url.
get_artifact_download_url: Callable for the new endpoint, or None to skip.

Returns:
str | None: The resolved download URL, or None if unavailable.

Raises:
Exception: Re-raises if the new endpoint fails and no fallback download_url exists.
"""
if get_artifact_download_url and artifact.output_artifact_id:
try:
return get_artifact_download_url(run_id, artifact.output_artifact_id)
except Exception as e:
fallback_url: str | None = getattr(artifact, "download_url", None)
if fallback_url:
logger.warning(
"Failed to resolve download URL via file endpoint for artifact {} ({}). "
"Falling back to deprecated download_url field.",
artifact.output_artifact_id,
e,
)
return fallback_url
raise
return getattr(artifact, "download_url", None)


def download_available_items( # noqa: PLR0913, PLR0917
progress: DownloadProgress,
application_run: Run,
Expand All @@ -192,7 +154,6 @@ def download_available_items( # noqa: PLR0913, PLR0917
create_subdirectory_per_item: bool = False,
download_progress_queue: Any | None = None, # noqa: ANN401
download_progress_callable: Callable | None = None, # type: ignore[type-arg]
get_artifact_download_url: Callable[[str, str], str] | None = None,
) -> None:
"""Download items that are available and not yet downloaded.

Expand All @@ -204,9 +165,6 @@ def download_available_items( # noqa: PLR0913, PLR0917
create_subdirectory_per_item (bool): Whether to create a subdirectory for each item.
download_progress_queue (Queue | None): Queue for GUI progress updates.
download_progress_callable (Callable | None): Callback for CLI progress updates.
get_artifact_download_url (Callable[[str, str], str] | None): Callback that takes
(run_id, artifact_id) and returns a presigned download URL. If None, falls back
to artifact.download_url (deprecated).
"""
items = list(application_run.results())
progress.item_count = len(items)
Expand Down Expand Up @@ -243,16 +201,9 @@ def download_available_items( # noqa: PLR0913, PLR0917
progress.artifact = artifact
update_progress(progress, download_progress_callable, download_progress_queue)

# Resolve artifact download URL via the new file endpoint, with fallback
artifact_url = _resolve_artifact_url(artifact, application_run.run_id, get_artifact_download_url)
if not artifact_url:
logger.warning("No download URL available for artifact {}", artifact.output_artifact_id)
continue

download_item_artifact(
progress,
artifact,
artifact_url,
item_directory,
item.external_id if not create_subdirectory_per_item else "",
download_progress_queue,
Expand All @@ -265,7 +216,6 @@ def download_available_items( # noqa: PLR0913, PLR0917
def download_item_artifact( # noqa: PLR0913, PLR0917
progress: DownloadProgress,
artifact: Any, # noqa: ANN401
artifact_download_url: str,
destination_directory: Path,
prefix: str = "",
download_progress_queue: Any | None = None, # noqa: ANN401
Expand All @@ -276,7 +226,6 @@ def download_item_artifact( # noqa: PLR0913, PLR0917
Args:
progress (DownloadProgress): Progress tracking object for GUI or CLI updates.
artifact (Any): The artifact to download.
artifact_download_url (str): The presigned URL to download the artifact from.
destination_directory (Path): Directory to save the file.
prefix (str): Prefix for the file name, if needed.
download_progress_queue (Queue | None): Queue for GUI progress updates.
Expand Down Expand Up @@ -311,7 +260,7 @@ def download_item_artifact( # noqa: PLR0913, PLR0917

download_file_with_progress(
progress,
artifact_download_url,
artifact.download_url,
artifact_path,
metadata_checksum,
download_progress_queue,
Expand Down
112 changes: 25 additions & 87 deletions src/aignostics/application/_gui/_page_application_run_describe.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,8 @@
ui, # noq
)
from nicegui import run as nicegui_run
from nicegui.events import ClickEventArguments

from aignostics.platform import ItemOutput, ItemResult, ItemState, Run, RunState
from aignostics.platform import ItemOutput, ItemResult, ItemState, RunState
from aignostics.third_party.showinfm.showinfm import show_in_file_manager
from aignostics.utils import GUILocalFilePicker, get_user_data_directory

Expand Down Expand Up @@ -339,9 +338,9 @@ async def start_download() -> None:

# Activate the timer now that download is starting
progress_timer.activate()
download_button.disable()
download_button.props(add="loading")
try:
download_button.disable()
download_button.props(add="loading")
results_folder = await nicegui_run.cpu_bound(
Service.application_run_download_static,
run_id=run.run_id,
Expand All @@ -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)
Comment on lines 341 to +379
Copy link

Copilot AI Mar 27, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Comment on lines 369 to +379
Copy link
Copy Markdown

@sentry sentry bot Mar 27, 2026

Choose a reason for hiding this comment

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

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.


ui.separator()
with ui.row(align_items="end").classes("w-full justify-end"):
Expand Down Expand Up @@ -787,85 +781,29 @@ def render_item(item: ItemResult) -> None: # noqa: C901, PLR0912, PLR0915
icon=mime_type_to_icon(mime_type),
group="artifacts",
).classes("w-full"):
if artifact.output_artifact_id:
artifact_id = artifact.output_artifact_id
if artifact.download_url:
url = artifact.download_url
title = artifact.name
metadata = artifact.metadata

with ui.button_group():
if mime_type == "image/tiff":
preview_button = ui.button(
ui.button(
"Preview",
icon=mime_type_to_icon(mime_type),
on_click=lambda _, url=url, title=title: tiff_dialog_open(title, url),
)

async def _preview_tiff(
_: ClickEventArguments,
aid: str = artifact_id,
t: str = title,
_run: Run = run,
_btn: ui.button = preview_button,
) -> None:
try:
_btn.props(add="loading")
url = await nicegui_run.io_bound(
_run.get_artifact_download_url, aid
)
tiff_dialog_open(t, url)
except Exception as e:
ui.notify(f"Failed to resolve preview URL: {e}", type="warning")
finally:
_btn.props(remove="loading")

preview_button.on_click(_preview_tiff)

if mime_type == "text/csv":
preview_button = ui.button(
ui.button(
"Preview",
icon=mime_type_to_icon(mime_type),
on_click=lambda _, url=url, title=title: csv_dialog_open(title, url),
)
if url:
ui.button(
text="Download",
icon="cloud_download",
on_click=lambda _, url=url: webbrowser.open(url),
)

async def _preview_csv(
_: ClickEventArguments,
aid: str = artifact_id,
t: str = title,
_run: Run = run,
_btn: ui.button = preview_button,
) -> None:
try:
_btn.props(add="loading")
url = await nicegui_run.io_bound(
_run.get_artifact_download_url, aid
)
csv_dialog_open(t, url)
except Exception as e:
ui.notify(f"Failed to resolve preview URL: {e}", type="warning")
finally:
_btn.props(remove="loading")

preview_button.on_click(_preview_csv)

artifact_dl_button = ui.button(
text="Download",
icon="cloud_download",
)

async def _download_artifact(
_: ClickEventArguments,
aid: str = artifact_id,
_run: Run = run,
_btn: ui.button = artifact_dl_button,
) -> None:
try:
_btn.props(add="loading")
url = await nicegui_run.io_bound(_run.get_artifact_download_url, aid)
webbrowser.open(url)
except Exception as e:
ui.notify(f"Failed to resolve download URL: {e}", type="warning")
finally:
_btn.props(remove="loading")

artifact_dl_button.on_click(_download_artifact)
if metadata:
ui.button(
text="Schema",
Expand Down
14 changes: 0 additions & 14 deletions src/aignostics/application/_service.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1483,19 +1483,6 @@ def update_qupath_add_input_progress(qupath_add_input_progress: QuPathAddProgres
update_progress(progress, download_progress_callable, download_progress_queue)

downloaded_items: set[str] = set() # Track downloaded items to avoid re-downloading

def _get_artifact_download_url(run_id: str, artifact_id: str) -> str:
"""Resolve artifact download URL via the new API endpoint.

Args:
run_id (str): The run ID.
artifact_id (str): The artifact ID.

Returns:
str: The presigned download URL.
"""
return self._get_platform_client().run(run_id).get_artifact_download_url(artifact_id)

while True:
run_details = application_run.details() # (Re)load current run details
progress.run = run_details
Expand All @@ -1509,7 +1496,6 @@ def _get_artifact_download_url(run_id: str, artifact_id: str) -> str:
create_subdirectory_per_item,
download_progress_queue,
download_progress_callable,
get_artifact_download_url=_get_artifact_download_url,
)

if run_details.state == RunState.TERMINATED:
Expand Down
3 changes: 1 addition & 2 deletions src/aignostics/platform/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@
get_mime_type_for_artifact,
mime_type_to_file_ending,
)
from .resources.runs import LIST_APPLICATION_RUNS_MAX_PAGE_SIZE, LIST_APPLICATION_RUNS_MIN_PAGE_SIZE, Artifact, Run
from .resources.runs import LIST_APPLICATION_RUNS_MAX_PAGE_SIZE, LIST_APPLICATION_RUNS_MIN_PAGE_SIZE, Run

__all__ = [
"API_ROOT_DEV",
Expand Down Expand Up @@ -146,7 +146,6 @@
"Application",
"ApplicationSummary",
"ApplicationVersion",
"Artifact",
"Client",
"InputArtifact",
"InputArtifactData",
Expand Down
Loading
Loading