fix: use load_artifact instead of file_data in rewind artifact restoration#4979
Open
yuting0624 wants to merge 3 commits intogoogle:mainfrom
Open
fix: use load_artifact instead of file_data in rewind artifact restoration#4979yuting0624 wants to merge 3 commits intogoogle:mainfrom
yuting0624 wants to merge 3 commits intogoogle:mainfrom
Conversation
…ation (google#4932) _compute_artifact_delta_for_rewind constructed a file_data reference (types.Part(file_data=...)) to restore artifacts to their historical version. However, GcsArtifactService raises NotImplementedError for file_data parts, and FileArtifactService has no file_data handling path, causing rewind_async to crash for all non-InMemory artifact services. Fix: use load_artifact to read the historical version's actual bytes, then save_artifact with inline_data — which all artifact service implementations support. This is consistent with how the 'artifact did not exist at rewind point' case already works (lines 750-753). Fixes google#4932
src/google/adk/runners.py
Outdated
Comment on lines
+768
to
+773
| artifact = types.Part( | ||
| inline_data=types.Blob( | ||
| mime_type=loaded.inline_data.mime_type, | ||
| data=loaded.inline_data.data, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
(minor) load_artifact already returns a types.Part with inline_data populated, so the loaded part could be passed directly to save_artifact without re-wrapping into a new types.Part
Author
There was a problem hiding this comment.
Good catch! Updated to pass the loaded Part directly to save_artifact without re-wrapping. Much cleaner, thanks!
Comment on lines
+775
to
+780
| # Fallback: artifact couldn't be loaded, mark as empty. | ||
| artifact = types.Part( | ||
| inline_data=types.Blob( | ||
| mime_type='application/octet-stream', data=b'' | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Minor suggestion: it might be worth logging a warning when the fallback is hit. A silent empty-blob replacement could be hard to debug if artifact storage is inconsistent
Author
There was a problem hiding this comment.
Agreed, added a logger.warning with the filename and version so it is easy to trace. Both suggestions addressed in the latest push.
rohityan
added
request clarification
…rning log - Pass load_artifact result directly to save_artifact instead of re-wrapping into a new types.Part (review by @lucasbarzotto-axonify) - Add logger.warning when fallback to empty blob is hit, to aid debugging of inconsistent artifact storage
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
3 participants
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
Fixes #4932
Runner._compute_artifact_delta_for_rewindconstructed afile_datareference to restore artifacts to their historical version during rewind. However:GcsArtifactServiceraisesNotImplementedErrorforfile_datapartsFileArtifactServicehas nofile_datahandling path (falls through to validation error)This caused
rewind_asyncto crash for any application not usingInMemoryArtifactService.Fix
Use
load_artifact(version=target_version)to read the historical version's actual bytes, thensave_artifactwithinline_data— which all artifact service implementations support.This is consistent with how the "artifact did not exist at rewind point" case already works (lines 750–753 use
inline_data).Changes
src/google/adk/runners.py: Replacefile_datareference construction withload_artifact+inline_datapattern (+18/-3 lines)