Conversation
There was a problem hiding this comment.
Pull request overview
Adds result-focused APIs and supporting utilities for discovering BindCraft artifacts in S3, persisting them to the DB, and returning pre-signed report/download/snapshot links to the frontend.
Changes:
- Added
/api/results/{run_id}/report,/downloads, and/snapshotsendpoints (plus expanded settings/logs behavior) with S3/Seqera error mapping. - Implemented BindCraft artifact discovery + link generation in
app/services/results_utils.pyand integrated output syncing into score calculation. - Added/updated Pydantic response models and expanded test coverage + README endpoint documentation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
app/routes/workflow/results.py |
Adds result download/report/snapshot routes and maps S3 errors to HTTP responses. |
app/services/results_utils.py |
Implements artifact classification, S3 discovery, DB persistence, and pre-signed link generation helpers. |
app/services/job_utils.py |
Refactors to reuse results utilities and sync outputs before score computation. |
app/schemas/workflows.py |
Introduces response models for downloads/snapshots/report payloads. |
tests/test_routes_results.py |
Adds route-level tests for new endpoints and error handling behavior. |
tests/test_services_job_utils.py |
Updates score tests for new sync behavior and adds tests around BindCraft output syncing + snapshot/report helpers. |
README.md |
Updates documented API endpoints to include the new /api/results/* routes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
* Initial plan * refactor: rename _s3_uri_to_key to s3_uri_to_key making it public Co-authored-by: vtnphan <53896516+vtnphan@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: vtnphan <53896516+vtnphan@users.noreply.github.com>
|
@marius-mather this PR was re-opened and ready to review now. I added the content type to the report for the front end rendering yesterday. Thank you!! ^^~ |
marius-mather
left a comment
There was a problem hiding this comment.
Looks good overall, it just looks like there could be a lot of expensive operations for something that might need to be called/repeated a lot. Can you think about how to store the report somewhere so it doesn't have to be synced/generated every time you need to download it?
Pull Request
Summary
SBP-179 Add result APIs for report, download, snapshot access, tighten BindCraft output discovery to the DB
run.idS3 layout, and refactor result-specific artifact logic intoresults_utils.Changes
app/routes/workflow/results.pyfor:app/schemas/workflows.pyfor result downloads and snapshot responsesworkflow_runs.id-based S3 path layout/reportreturns only the HTML animation/report link/downloadsexcludes snapshot PNGs/snapshotsreturns snapshot PNG links onlyapp/services/job_utils.pytoapp/services/results_utils.pyapp/services/job_utils.py, reusing shared result sync helpers where neededREADME.mdendpoint documentation to reflect the current APIHow to Test
GET /api/results/{run_id}/reportreturns the HTML animation/report link onlyGET /api/results/{run_id}/downloadsreturns report/CSV/PDB links and excludes snapshotsGET /api/results/{run_id}/snapshotsreturns snapshot PNG links onlyType of change
Checklist