Skip to content

feat: add S3ArtifactService with native async and atomic versioning - #115

Merged
DeanChensj merged 7 commits into
google:mainfrom
miyannishar:feat/s3-artifact-service-v2
May 16, 2026
Merged

feat: add S3ArtifactService with native async and atomic versioning#115
DeanChensj merged 7 commits into
google:mainfrom
miyannishar:feat/s3-artifact-service-v2

Conversation

@miyannishar

@miyannisharmiyannishar commented Apr 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds S3ArtifactService — a BaseArtifactService implementation 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 implementation
  • tests/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 — export S3ArtifactService
  • src/google/adk_community/artifacts/README.md — usage documentation
  • pyproject.toml — add s3 optional dependency group
  • src/google/adk_community/__init__.py — register artifacts subpackage

Design Decisions

ConcernApproach
Async I/Oaioboto3 for native async (no asyncio.to_thread wrappers)
Race conditionsIfNoneMatch="*" on put_object for atomic versioning
Optional dependencyaioboto3 only required via pip install google-adk-community[s3]
MetadataJSON-serialised in S3 user-metadata with safe decode fallback
Batch operationsdelete_objects in 1000-key chunks per S3 limits
Version listingParallel head_object calls via asyncio.gather
S3-compatibleWorks with MinIO, DigitalOcean Spaces, etc. via aws_configs

Usage

fromgoogle.adk_community.artifactsimportS3ArtifactServiceartifact_service=S3ArtifactService(
bucket_name="my-adk-artifacts",
aws_configs={"region_name": "us-east-1"},
)

Test Plan

pip install google-adk-community[s3,test]
pytest tests/unittests/artifacts/ -v
  • 13 unit tests covering CRUD, versioning, user-scoped artifacts, metadata, empty artifacts, text encoding, and conflict retry
  • All tests pass without AWS credentials (full async mock infrastructure)
  • Existing tests unaffected

Closes#37
Closes#71

@google-cla

Copy link
Copy Markdown

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.

This was referenced Apr 18, 2026
@miyannishar
miyannisharforce-pushed the feat/s3-artifact-service-v2 branch 2 times, most recently from 1822a8d to 25ebe3eCompareApril 18, 2026 23:12
@DeanChensj

Copy link
Copy Markdown
Collaborator

@gemini-cli /review

@github-actions

Copy link
Copy Markdown

🤖 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.

@miyannishar
miyannisharforce-pushed the feat/s3-artifact-service-v2 branch from d54f607 to 1107029CompareMay 5, 2026 02:07
@DeanChensj

Copy link
Copy Markdown
Collaborator

btw, could you send out the CI fix in a separate PR? Thanks!

@googlegoogle deleted a comment from github-actionsBotMay 5, 2026
@googlegoogle deleted a comment from github-actionsBotMay 5, 2026
@DeanChensj

Copy link
Copy Markdown
Collaborator

@gemini-cli /review

@github-actions

Copy link
Copy Markdown

🤖 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.

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the PR! The implementation of S3ArtifactService using aioboto3 and atomic versioning is solid.

I've left a few comments regarding:

  1. Potential race condition in _get_session initialization.
  2. High concurrency in list_artifact_versions when gathering head_object tasks.
  3. S3 metadata size limits when flattening custom metadata.
  4. 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.

Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/sessions/redis_session_service.py
@miyannishar
miyannisharforce-pushed the feat/s3-artifact-service-v2 branch from 1107029 to f3b48a3CompareMay 5, 2026 07:38
@miyannishar

Copy link
Copy Markdown
ContributorAuthor

Done! Moved the CI fix (Redis temp-state filtering + --extra s3 in workflow) to a separate PR: #125. This PR now only contains the S3ArtifactService implementation.

Also addressed all the review comments in the latest push:

  • asyncio.Lock for async-safe session init
  • ✅ S3 metadata 2KB size validation
  • asyncio.Semaphore(10) for bounded head_object concurrency

@DeanChensj

Copy link
Copy Markdown
Collaborator

@gemini-cli /review

@github-actions

Copy link
Copy Markdown

🤖 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.

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The S3ArtifactService is a solid addition. The use of aioboto3 for native async and the implementation of atomic versioning with IfNoneMatch show good attention to production requirements. My comments focus on optimizing listing performance and ensuring consistency in the data types returned. Nice work!

Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py Outdated
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
@DeanChensj

Copy link
Copy Markdown
Collaborator

name = 'aioboto3', import_ = <function _gcd_import at 0x7fedb01d04a0>
> ???
E ModuleNotFoundError: No module named 'aioboto3'
<frozen importlib._bootstrap>:1324: ModuleNotFoundError

Could you address the test failures?

@DeanChensj

Copy link
Copy Markdown
Collaborator

@gemini-cli /review

@github-actions

Copy link
Copy Markdown

🤖 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.

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Thanks for the contribution! This is a solid implementation of the S3ArtifactService.

I've left a few comments regarding:

  1. Infinite retry potential in save_artifact.
  2. Large task creation in list_artifact_versions.
  3. Minor best practices like __init__.py in tests.

Overall, the code is well-structured and the use of S3 conditional writes for atomic versioning is a great choice.

Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
@miyannishar

Copy link
Copy Markdown
ContributorAuthor

@DeanChensj the ModuleNotFoundError: No module named 'aioboto3' is caused by the CI workflow only installing the test extra (uv sync --extra test), while aioboto3 lives in the s3 optional dependency group.

The simplest fix is adding --extra s3 to the install step in .github/workflows/unit-tests.yaml:

uv sync --extra test --extra s3

Since that file lives in the upstream repo, I can open a separate follow-up PR — same pattern as the Redis CI fix in #125. Let me know if that's the right approach.

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
Closesgoogle#37Closesgoogle#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
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
- Metadata size check now accounts for S3 x-amz-meta- prefix overhead
(~11 bytes per key) in the 2KB limit calculation.
- Replace unconventional iter(int, 1) infinite loop with while True and
add exponential backoff (100ms, 200ms, ... capped at 5s) between
version conflict retries.
- list_artifact_keys now uses S3 Delimiter='/' with CommonPrefixes for
O(unique-keys) efficiency instead of listing every version object.
- load_artifact returns Part.from_text() for text/* content types so
consumers can check part.text consistently.
The S3 artifact tests require aioboto3 and botocore which are only
installed with the [s3] extra. CI runs with --extra test only, so
these tests fail with ModuleNotFoundError on collection.
Adding pytest.importorskip() at module level gracefully skips the
entire test file when the S3 dependencies aren't available.
@miyannishar
miyannisharforce-pushed the feat/s3-artifact-service-v2 branch from a3402d4 to 5cc19dfCompareMay 14, 2026 04:41
@DeanChensj

Copy link
Copy Markdown
Collaborator

@gemini-cli /review

@github-actions

Copy link
Copy Markdown

🤖 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.

@github-actionsgithub-actionsBot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Overall, the implementation of S3ArtifactService is very solid and follows best practices for async S3 integration and atomic versioning. I've added a few comments regarding robustness in listing operations and S3 metadata key handling.

Key points:

  • The use of IfNoneMatch for atomic versioning is excellent.
  • The optional dependency pattern is correctly implemented.
  • The test suite is comprehensive and well-structured with its own mock S3 infrastructure.

One recommendation is to improve the error handling in list_artifact_versions to ensure that a single failing head_object call doesn't crash the entire listing.

Comment threadpyproject.toml
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
Comment threadsrc/google/adk_community/artifacts/s3_artifact_service.py
@DeanChensj

Copy link
Copy Markdown
Collaborator

Thanks @miyannishar , I think the PR is already in a good shape!

@DeanChensj

Copy link
Copy Markdown
Collaborator

Could you address the remaining comments?

- Consolidate aioboto3 import: cache module ref on self._aioboto3 in
__init__, remove duplicate import in _get_session
- Default save_max_retries to 10 (safe cap) instead of -1 (infinite)
- Normalize metadata keys to lowercase in _flatten_metadata with
case-collision warning (S3 metadata keys are case-insensitive)
- Handle ClientError in _head helper: return None for objects deleted
between list_objects_v2 and head_object calls, skip in result loop
@miyannishar

Copy link
Copy Markdown
ContributorAuthor

@DeanChensj Addressed all remaining comments in 0f0d11f:

  • _head helper resilience — catches ClientError (NoSuchKey/404) and returns None so a single deleted object doesn't fail the entire list_artifact_versions call
  • Metadata key case-normalisation_flatten_metadata now lowercases all keys and logs a warning on case-collision, preventing silent S3 header conflicts
  • aioboto3 import consolidation — cached as self._aioboto3 in __init__, removed the duplicate import in _get_session
  • save_max_retries default — changed from -1 (infinite) to 10 (safe cap); -1 still available as opt-in

@DeanChensj
DeanChensj merged commit d07ad02 into google:mainMay 16, 2026
20 checks passed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add S3ArtifactService for Amazon S3 artifact storage support Add S3ArtifactService for Amazon S3 artifact storage support

2 participants

@miyannishar@DeanChensj