-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix(artifacts): accept dict-shaped artifacts in InMemoryArtifactService (#3495) #3622
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
base: main
Are you sure you want to change the base?
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. |
Summary of ChangesHello @Fangmbeng, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses an issue where the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request makes InMemoryArtifactService more robust by adding support for dictionary-shaped artifacts, which is a great improvement for interoperability. The changes in artifact_util.py and in_memory_artifact_service.py correctly handle both object and dict forms. A new focused unit test is also added to cover these new cases.
My review includes suggestions to improve code consistency and maintainability by reusing helper functions, removing duplicated code in tests, and improving error handling. I've also suggested an enhancement to the new tests to cover more edge cases related to empty artifacts.
| # Support both object-like `types.Part` and plain `dict` shapes. | ||
| file_data = None | ||
| if isinstance(artifact, dict): | ||
| file_data = artifact.get("file_data") | ||
| else: | ||
| file_data = getattr(artifact, "file_data", None) | ||
|
|
||
| if not file_data: | ||
| return False | ||
|
|
||
| file_uri = file_data.get("file_uri") if isinstance(file_data, dict) else getattr(file_data, "file_uri", None) | ||
| return bool(file_uri and isinstance(file_uri, str) and file_uri.startswith("artifact://")) |
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 logic to safely access fields from both dict and object shapes is a bit verbose and repeats the access pattern. To improve readability and reduce duplication, you could define a local helper function.
| # Support both object-like `types.Part` and plain `dict` shapes. | |
| file_data = None | |
| if isinstance(artifact, dict): | |
| file_data = artifact.get("file_data") | |
| else: | |
| file_data = getattr(artifact, "file_data", None) | |
| if not file_data: | |
| return False | |
| file_uri = file_data.get("file_uri") if isinstance(file_data, dict) else getattr(file_data, "file_uri", None) | |
| return bool(file_uri and isinstance(file_uri, str) and file_uri.startswith("artifact://")) | |
| # Support both object-like `types.Part` and plain `dict` shapes. | |
| def _get_field(obj, name): | |
| if isinstance(obj, dict): | |
| return obj.get(name) | |
| return getattr(obj, name, None) | |
| file_data = _get_field(artifact, "file_data") | |
| if not file_data: | |
| return False | |
| file_uri = _get_field(file_data, "file_uri") | |
| return bool(file_uri and isinstance(file_uri, str) and file_uri.startswith("artifact://")) |
| if inline_data is not None: | ||
| # inline_data may be a dict or an object | ||
| artifact_version.mime_type = ( | ||
| inline_data.get("mime_type") | ||
| if isinstance(inline_data, dict) | ||
| else inline_data.mime_type | ||
| ) | ||
| elif text is not None: | ||
| artifact_version.mime_type = "text/plain" | ||
| elif artifact.file_data is not None: | ||
| elif file_data is not None: | ||
| # If the artifact is an artifact-ref we validate the referenced URI. | ||
| if artifact_util.is_artifact_ref(artifact): | ||
| if not artifact_util.parse_artifact_uri(artifact.file_data.file_uri): | ||
| raise ValueError( | ||
| f"Invalid artifact reference URI: {artifact.file_data.file_uri}" | ||
| ) | ||
| # If it's a valid artifact URI, we store the artifact part as-is. | ||
| # And we don't know the mime type until we load it. | ||
| file_uri = ( | ||
| file_data.get("file_uri") if isinstance(file_data, dict) else file_data.file_uri | ||
| ) | ||
| if not artifact_util.parse_artifact_uri(file_uri): | ||
| raise ValueError(f"Invalid artifact reference URI: {file_uri}") | ||
| # Valid artifact URI: keep part as-is; mime type may be resolved later. | ||
| else: | ||
| artifact_version.mime_type = artifact.file_data.mime_type | ||
| artifact_version.mime_type = ( | ||
| file_data.get("mime_type") if isinstance(file_data, dict) else file_data.mime_type | ||
| ) |
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.
You can simplify the extraction of mime_type and file_uri by consistently using the _get_field helper function you've defined earlier. This will make the code more concise and less repetitive.
if inline_data is not None:
# inline_data may be a dict or an object
artifact_version.mime_type = _get_field(inline_data, "mime_type")
elif text is not None:
artifact_version.mime_type = "text/plain"
elif file_data is not None:
# If the artifact is an artifact-ref we validate the referenced URI.
if artifact_util.is_artifact_ref(artifact):
file_uri = _get_field(file_data, "file_uri")
if not artifact_util.parse_artifact_uri(file_uri):
raise ValueError(f"Invalid artifact reference URI: {file_uri}")
# Valid artifact URI: keep part as-is; mime type may be resolved later.
else:
artifact_version.mime_type = _get_field(file_data, "mime_type")| try: | ||
| if a == types.Part() or a == types.Part(text=""): | ||
| return True | ||
| inline = getattr(a, "inline_data", None) | ||
| if inline and not getattr(inline, "data", None): | ||
| return True | ||
| except Exception: | ||
| return False |
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.
| # Stub google.genai.types if not available | ||
| try: | ||
| from google.genai import types as types_mod # type: ignore | ||
| except Exception: | ||
| genai = _types.ModuleType("google.genai") | ||
| types_mod = _types.ModuleType("google.genai.types") | ||
| class Part: | ||
| def __init__(self, inline_data=None, text=None, file_data=None): | ||
| self.inline_data = inline_data | ||
| self.text = text | ||
| self.file_data = file_data | ||
|
|
||
| def __eq__(self, other): | ||
| if not isinstance(other, Part): | ||
| return False | ||
| return ( | ||
| (self.inline_data == other.inline_data) | ||
| and (self.text == other.text) | ||
| and (self.file_data == other.file_data) | ||
| ) | ||
| class Content: | ||
| pass | ||
| types_mod.Part = Part | ||
| types_mod.Content = Content | ||
| sys.modules["google.genai"] = genai | ||
| sys.modules["google.genai.types"] = types_mod |
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.
|
|
||
| # list keys should include the user-scoped filename | ||
| keys = await svc.list_artifact_keys(app_name="app", user_id="user") | ||
| assert "user:avatar.png" in keys |
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.
This is a good start for testing dict-shaped artifacts. To improve test coverage, consider adding tests for the new _is_empty logic in load_artifact. Specifically, you could add test cases that save various forms of empty artifacts (e.g., {}, {'text': ''}, {'inline_data': {'data': b''}}) and then assert that load_artifact correctly returns None for them.
|
Hi @Fangmbeng , Thank you for your contribution! |
Fixes #3495 — make InMemoryArtifactService robust to dict-shaped artifacts commonly produced by AgentSpace uploads. Adds unit test that covers dict and object artifact shapes.\n\nChanges: \n- in_memory_artifact_service: safe field extraction for dict/object artifacts; fallback mime-type.\n- artifact_util: is_artifact_ref handles dict shapes.\n- tests: focused unit test for dict/object artifact flows.\n