fix(auditor): connect aggregated artifacts download - #1141
Open
parkanzky wants to merge 2 commits into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughPlatform result routes now support custom async handlers. The auditor adds a GET route that downloads configured Garak artifacts as a temporary gzip tar archive, with 404 handling, cleanup scheduling, route registration, and tests. ChangesAuditor artifact download
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuditorRoute
participant AsyncNeMoPlatform
participant ArchiveStorage
Client->>AuditorRoute: Request job artifacts
AuditorRoute->>AsyncNeMoPlatform: List and download Garak results
AuditorRoute->>ArchiveStorage: Create temporary artifacts.tar.gz
AuditorRoute-->>Client: Return archive or HTTP 404
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@plugins/nemo-auditor/src/nemo_auditor/jobs/artifacts_route.py`:
- Around line 38-55: Update the artifact aggregation flow around
download_from_result_info and tar.add to synchronously clean all accumulated
temporary result directories and agg_tmp when either operation fails, then
re-raise the exception. Schedule background cleanup tasks only after the archive
is created successfully, and add a test covering cleanup after a failed
download.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 643b16a0-3748-4df1-83f6-d66734bec9b9
📒 Files selected for processing (6)
packages/nemo_platform_plugin/src/nemo_platform_plugin/jobs/api_factory.pyplugins/nemo-auditor/src/nemo_auditor/jobs/artifacts_route.pyplugins/nemo-auditor/src/nemo_auditor/jobs/audit.pyplugins/nemo-auditor/src/nemo_auditor/service.pyplugins/nemo-auditor/tests/test_artifacts_route.pyplugins/nemo-auditor/tests/test_service.py
Comment on lines
+38
to
+55
| tmp_dirs = [] | ||
| for result in relevant: | ||
| filename, tmp_dir_path = await download_from_result_info( | ||
| result_name=result.name, | ||
| job_name=job, | ||
| workspace=workspace, | ||
| artifact_url=result.artifact_url, | ||
| files_sdk=sdk, | ||
| ) | ||
| tmp_dirs.append((filename, tmp_dir_path)) | ||
| background_tasks.add_task(tmp_dir_path.cleanup_tmp_dir) | ||
|
|
||
| agg_tmp = Path(tempfile.mkdtemp()) | ||
| tar_path = agg_tmp / "artifacts.tar.gz" | ||
| with tarfile.open(tar_path, "w:gz") as tar: | ||
| for filename, tmp_dir_path in tmp_dirs: | ||
| tar.add(tmp_dir_path.path, arcname=filename) | ||
| background_tasks.add_task(lambda: shutil.rmtree(agg_tmp, ignore_errors=True)) |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Clean up temporary directories on failure.
If download_from_result_info or tar.add raises, the endpoint returns no FileResponse. FastAPI does not run these scheduled background tasks. The successful downloads then leak temporary directories.
Use try/except to clean accumulated result directories and agg_tmp synchronously before re-raising. Schedule background cleanup only after archive creation succeeds. Add a failed-download test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@plugins/nemo-auditor/src/nemo_auditor/jobs/artifacts_route.py` around lines
38 - 55, Update the artifact aggregation flow around download_from_result_info
and tar.add to synchronously clean all accumulated temporary result directories
and agg_tmp when either operation fails, then re-raise the exception. Schedule
background cleanup tasks only after the archive is created successfully, and add
a test covering cleanup after a failed download.
Signed-off-by: Paul A. Parkanzky <[email protected]>
Signed-off-by: Paul A. Parkanzky <[email protected]>
parkanzky
force-pushed
the
nvbug6562262-aggregate-artifacts
branch
from
August 7, 2026 14:48
2d25d16 to
650d1f5
Compare
Contributor
Contributor
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
QA found that aggregated artifacts were not available for download while SDK included download path. Added aggregated artifacts to download path.
Changes
Connected the aggregate download artifact API path. Results in tgz file. This was already in the docs, so made code match.
Type of Change
Quality Gates
Verification
Signed-off-by:traileruv run pre-commit run -apasses, or any blocked checks are identified belowTargeted validation:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation