-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fix/artifact dict handling 3495 #4524
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
454c2eb
Accept dict-shaped artifacts in artifact services; add tests (fixes #…
Fangmbeng c3a76fe
refactor: centralize dict-to-Part conversion in BaseArtifactService; …
Fangmbeng fec8e25
chore: remove DICT_ARTIFACTS_FIX.md from tracking and add to gitignore
Fangmbeng 1276aa7
fix(artifacts): accept dict-shaped artifacts in InMemoryArtifactServi…
Fangmbeng 283ef1e
WIP before reverting bad commits
Fangmbeng 0bd955f
Revert "chore: remove DICT_ARTIFACTS_FIX.md from tracking and add to …
Fangmbeng File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # Fix for Issue #3622: Accept dict-shaped artifacts in InMemoryArtifactService | ||
|
|
||
| ## Summary | ||
| Fixed the artifact services to accept dict-shaped (serialized) artifacts in addition to `types.Part` objects. This allows users to pass artifacts as dictionaries, which are automatically converted to `types.Part` objects internally. | ||
|
|
||
| ## Changes Made | ||
|
|
||
| ### 1. Base Artifact Service (`base_artifact_service.py`) | ||
| - Updated the `save_artifact()` method signature to accept `types.Part | dict[str, Any]` | ||
| - Updated the docstring to clarify that dict-shaped artifacts are now supported | ||
|
|
||
| ### 2. InMemoryArtifactService (`in_memory_artifact_service.py`) | ||
| - Updated the `save_artifact()` method to: | ||
| - Accept `types.Part | dict[str, Any]` parameter type | ||
| - Added conversion logic: `if isinstance(artifact, dict): artifact = types.Part.model_validate(artifact)` | ||
| - This deserialization happens before any artifact processing | ||
|
|
||
| ### 3. GcsArtifactService (`gcs_artifact_service.py`) | ||
| - Updated the async `save_artifact()` method to: | ||
| - Accept `types.Part | dict[str, Any]` parameter type | ||
| - Added conversion logic before threading to sync method | ||
| - The internal `_save_artifact()` method processes the already-converted `types.Part` object | ||
|
|
||
| ### 4. FileArtifactService (`file_artifact_service.py`) | ||
| - Updated the async `save_artifact()` method to: | ||
| - Accept `types.Part | dict[str, Any]` parameter type | ||
| - Added conversion logic before threading to sync method | ||
| - The internal `_save_artifact_sync()` method processes the already-converted `types.Part` object | ||
|
|
||
| ### 5. ForwardingArtifactService (`_forwarding_artifact_service.py`) | ||
| - Updated the `save_artifact()` method to: | ||
| - Accept `types.Part | dict[str, Any]` parameter type | ||
| - Added conversion logic before forwarding to the parent tool context | ||
|
|
||
| ### 6. Test Suite (`test_artifact_service.py`) | ||
| - Added `test_save_load_dict_shaped_artifact()` test to verify dict-shaped artifacts can be saved and loaded across all service types (IN_MEMORY, GCS, FILE) | ||
| - Added `test_save_text_dict_shaped_artifact()` test to verify text-based dict-shaped artifacts work correctly in InMemoryArtifactService | ||
|
|
||
| ## How It Works | ||
|
|
||
| When a dictionary is passed to `save_artifact()`: | ||
| 1. The method checks if the artifact is a dictionary using `isinstance(artifact, dict)` | ||
| 2. If it is, it converts it to a `types.Part` object using `types.Part.model_validate(artifact)` | ||
| 3. The rest of the method processes the converted `types.Part` object as usual | ||
|
|
||
| ## Example Usage | ||
|
|
||
| ```python | ||
| # Before (still supported) | ||
| artifact = types.Part(text="Hello, World!") | ||
| await service.save_artifact(..., artifact=artifact) | ||
|
|
||
| # After (now also supported) | ||
| artifact_dict = {"text": "Hello, World!"} | ||
| await service.save_artifact(..., artifact=artifact_dict) | ||
|
|
||
| # Also works with inline data | ||
| artifact_dict = { | ||
| "inline_data": { | ||
| "data": "dGVzdF9kYXRh", # base64 encoded | ||
| "mime_type": "text/plain", | ||
| } | ||
| } | ||
| await service.save_artifact(..., artifact=artifact_dict) | ||
| ``` | ||
|
|
||
| ## Backward Compatibility | ||
|
|
||
| ✅ **Fully backward compatible** - All existing code using `types.Part` objects will continue to work exactly as before. |
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
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
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
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
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
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
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
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
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment on line 860 is not entirely accurate. The
FileArtifactServicepreserves text artifacts as text, whileGcsArtifactServiceconverts them toinline_datawith bytes.InMemoryArtifactServicealso preserves text.The current assertion is a bit too general and could mask incorrect behavior in one of the services. To make the test more robust and clear about the expected behavior of each service, I suggest adding specific assertions for each
service_type.