feat: add S3ArtifactService with native async and atomic versioning#115
feat: add S3ArtifactService with native async and atomic versioning#115miyannishar wants to merge 4 commits intogoogle:mainfrom
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
1fb5f81 to
b64b9b4
Compare
Adds S3ArtifactService to the artifacts module, providing: - Native async I/O via aioboto3 (no asyncio.to_thread wrappers) - Atomic versioning using S3 IfNoneMatch conditional writes - Session-scoped and user-scoped artifact namespacing - Custom metadata (JSON-serialised in S3 user-metadata) - Batch delete (1000 keys per request) for efficient cleanup - Paginated listing for large artifact collections - Parallel head_object calls in list_artifact_versions - Optional [s3] dependency group (aioboto3>=13.0.0) - Comprehensive test suite with full async mock infrastructure Closes google#37 Closes google#71
- Remove section comment banners (# --- heading --- patterns) - Simplify module and class docstrings to one-liners - Move Args section to __init__ docstring (matches RedisSessionService) - Shorten method docstrings to single line - Remove explanatory inline comments - Clean up blank lines left from removals
1822a8d to
25ebe3e
Compare
|
@gemini-cli /review |
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
d54f607 to
1107029
Compare
|
btw, could you send out the CI fix in a separate PR? Thanks! |
|
@gemini-cli /review |
|
🤖 Hi @DeanChensj, I've received your request, and I'm working on it now! You can track my progress in the logs for more details. |
There was a problem hiding this comment.
Thanks for the PR! The implementation of S3ArtifactService using aioboto3 and atomic versioning is solid.
I've left a few comments regarding:
- Potential race condition in
_get_sessioninitialization. - High concurrency in
list_artifact_versionswhen gatheringhead_objecttasks. - S3 metadata size limits when flattening custom metadata.
- An out-of-scope change in
redis_session_service.py.
Overall, the test coverage is excellent and the design is well-aligned with the project's optional dependency pattern.
Adds S3-backed artifact storage with: - Atomic versioning via IfNoneMatch conditional writes - Async-safe session initialization with asyncio.Lock - Bounded concurrent head_object calls (semaphore=10) - S3 metadata size validation (2KB limit) - User-scoped artifact namespace support - Full test coverage with mocked S3 client
1107029 to
f3b48a3
Compare
|
Done! Moved the CI fix (Redis temp-state filtering + Also addressed all the review comments in the latest push:
|
Summary
Adds
S3ArtifactService— aBaseArtifactServiceimplementation backed by Amazon S3, providing production-ready artifact storage for AWS deployments.This supersedes #36 (my earlier attempt, closed for inactivity) and #29 (by @wmsnp). The final implementation takes the best from both: native async from #29 and the optional dependency pattern + test suite from #36.
Changes
src/google/adk_community/artifacts/s3_artifact_service.py— full implementationtests/unittests/artifacts/test_s3_artifact_service.py— 13 unit tests with async mock infrastructure (no AWS credentials required)src/google/adk_community/artifacts/__init__.py— exportS3ArtifactServicesrc/google/adk_community/artifacts/README.md— usage documentationpyproject.toml— adds3optional dependency groupsrc/google/adk_community/__init__.py— register artifacts subpackageDesign Decisions
aioboto3for native async (noasyncio.to_threadwrappers)IfNoneMatch="*"onput_objectfor atomic versioningaioboto3only required viapip install google-adk-community[s3]delete_objectsin 1000-key chunks per S3 limitshead_objectcalls viaasyncio.gatheraws_configsUsage
Test Plan
Closes #37
Closes #71