feat: Implement a vertex based task store#752
feat: Implement a vertex based task store#752gaborfeher wants to merge 7 commits intoa2aproject:mainfrom
Conversation
Summary of ChangesHello, 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 introduces a significant new feature by integrating Google Cloud Vertex AI as a backend for task storage. This change enhances the system's persistence capabilities, moving beyond the existing in-memory solution to a robust, cloud-based task management system. The core implementation involves a new Highlights
Changelog
Activity
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.
Code Review
This pull request introduces a new TaskStore implementation based on Google Cloud Vertex AI, along with the necessary type converters and integration tests. The changes are well-structured, with a clear separation of concerns between the store and the data conversion logic. The addition of comprehensive integration tests that run against a sandbox environment is a great way to ensure correctness.
I've identified a few areas for improvement, including removing hardcoded test configurations, addressing some copy-paste errors in docstrings, removing debug print statements, and clarifying the handling of task history. There's also a dependency on a non-standard git reference in pyproject.toml that might cause issues in the long run. All comments align with the provided rules or are not covered by them, and thus no comments were dropped or modified.
ad1e1ce to
fc58455
Compare
ishymko
left a comment
There was a problem hiding this comment.
Nice!
Added a few non-critical double-checking comments around conversion logic and testing.
| if isinstance(part.root, DataPart): | ||
| data_bytes = json.dumps(part.root.data).encode('utf-8') | ||
| return types.Part( | ||
| inline_data=types.Blob( | ||
| mime_type='application/json', data=data_bytes | ||
| ) | ||
| ) |
There was a problem hiding this comment.
I believe this isn't going to roundtrip back to the A2A DataPart? Maybe application/json MIME type should be used as a signal when converting back (one can have a FileWithBytes representing the same though but it feels that this use case is going to be less popular)?
There was a problem hiding this comment.
Yes, that's correct. So are you saying that it's better to turn FileWithBytes of type application/json into DataPart than vice versa?
There was a problem hiding this comment.
Maybe some roundtrip tests can be added here (or existing replaced with roundtrip)?
I see that test_vertex_task_store.py does save-read tests but as it requires real Vertex it's not going to be executed in CI.
Create a task store adapter that uses the Vertex Managed Task Store as a backend.
Fixes #751