Migrate fetch_artifacts.py to use artifact_backend and artifact_manager#3019
Merged
Conversation
Refactors fetch_artifacts.py to use the ArtifactBackend interface instead of directly accessing S3 implementation details. This improves separation of concerns and makes the code more testable. Changes: - Remove BucketMetadata class (duplicated S3Backend path construction) - Remove module-level S3 client (now uses backend.s3_client internally) - Update list_s3_artifacts() to use S3Backend.list_artifacts() - Update ArtifactDownloadRequest to use generic ArtifactBackend type - Simplify download_artifact() to use backend.download_artifact() API - Add retry logic (3 retries, exponential backoff) to S3Backend.download_artifact() - Update tests to mock S3Backend instead of raw paginator 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Makes the function fully backend-agnostic by: - Renaming from list_s3_artifacts to list_artifacts_for_group - Changing type hint from S3Backend to ArtifactBackend - Simplifying logging to use backend.base_uri instead of S3-specific attributes 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Removes duplicate download logic from fetch_artifacts.py by reusing existing code from artifact_manager.py which already has retry logic. Changes: - Remove ArtifactDownloadRequest class (use artifact_manager.DownloadRequest) - Remove local download_artifact() function (use artifact_manager.download_artifact) - Rename get_artifact_download_requests() to get_download_requests() - Update extract_artifact() to take Path instead of request object - Revert retry logic from S3Backend.download_artifact() (not needed now) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
This test was mocking the backend to raise ClientError, then verifying that list_artifacts_for_group() propagates the exception. Since there's no exception handling in that function, this was just testing that Python propagates exceptions - not useful. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Member
Author
|
Ping? |
HereThereBeDragons
approved these changes
Jan 30, 2026
1 task
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
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.
Motivation
While working on #3000, I found that this code is directly handling S3 paths like so:
I'm trying to move that code to a central location and to make it possible to swap out the "s3 cloud backend for a "local backend" for easier testing. Moving string manipulation code is easy enough, but this file also directly communicated with S3:
That would be harder to refactor... if we didn't already have this abstraction in
ArtifactBackend. So, I'm first migrating this code to use that class.Technical Details
I also considered making
fetch_artifacts.pyjust callartifact_manager.pydirectly, but they have different roles:artifact_manager.pyis topology-aware and is currently just used for multi-arch buildsfetch_artifacts.pyhas regex-based fetching and is currently used by developers to fetch explicit lists of artifacts and by CI workflows to download subsets of artifacts for testsTest Plan
fetch_artifacts.pyscript indirectly viabuild_tools/install_rocm_from_artifacts.pyTest Result
CI jobs run the script with no issues, e.g. https://github.com/ROCm/TheRock/actions/runs/21192582915/job/60969434096?pr=3019#step:6:280
Submission Checklist