fix: Improve OpenAPI operation parser type safety and code organization #3002
Conversation
This commit introduces a new SqliteMemoryService that provides persistent storage for agent memory using SQLite with async operations via aiosqlite. Features: - Persistent storage with SQLite database - Async operations using aiosqlite for non-blocking database access - Keyword-based search matching InMemoryMemoryService behavior - User isolation by app_name and user_id - Automatic database initialization with optimized indexes - Additional utility methods: clear_memory() and get_memory_stats() - Duplicate entry prevention with UNIQUE constraints - Content filtering for empty or invalid events Implementation follows existing ADK patterns: - Extends BaseMemoryService with @OverRide decorators - Similar interface to InMemoryMemoryService and VertexAi services - Comprehensive test coverage with 24 test cases - Code formatted with project standards (isort + pyink) Dependencies: - Added aiosqlite>=0.20.0 dependency to pyproject.toml - Updated memory module __init__.py to export SqliteMemoryService Testing: - Complete test suite in tests/unittests/memory/test_sqlite_memory_service.py - All 24 tests passing covering initialization, CRUD operations, search, user isolation, memory management, and edge cases
Address reviewer feedback with significant performance and robustness improvements: **Performance Optimizations:** - Implement SQLite FTS5 (Full-Text Search) for efficient database-level search - Replace Python-based keyword matching with native FTS5 MATCH queries - Add lazy one-time database initialization with asyncio.Lock - Remove redundant _init_db() calls from every method **Robustness Improvements:** - Replace generic Exception handling with specific aiosqlite.Error - Add proper error handling during database initialization - Improve query preparation with FTS5-specific escaping - Add database triggers to keep FTS5 table synchronized **New Features:** - FTS5 virtual table for scalable full-text search - Query preparation function with special character handling - Enhanced error recovery and graceful degradation **Testing:** - Add 8 new test cases covering FTS5 functionality - Test lazy initialization behavior - Test FTS5 query preparation and edge cases - Test comprehensive error handling scenarios - All 32 tests passing with improved coverage This addresses the performance bottleneck in search_memory() and ensures the service scales efficiently with large datasets while maintaining reliability through proper error handling.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
- Move GITHUB_TOKEN validation from module import time to function call time - This prevents ValueError during module imports when GITHUB_TOKEN is not yet set - Update utils.py to use lazy token loading via get_github_token() function - Apply fix to both adk_pr_triaging_agent and adk_triaging_agent - Fixes CI/CD issue where triaging bots fail during workflow initialization
This reverts commit 47eb886.
- Fix line length formatting for test function name - Ensure compliance with project style guidelines - All 34 tests still passing after formatting Addresses reviewer feedback for PR google#2768
- Add explicit Dict and Any imports from typing module - Replace dict return type with Dict[str, Any] for Python 3.9 compatibility - Ensures compatibility across Python 3.9-3.13 versions - Fixes CI test failures on Python 3.9
- Add development-only Python 3.9 compatibility testing scripts - Prevents accidental inclusion of temporary testing files - Files: check_py39_compatibility.py, test_runtime_compatibility.py, test_py39_docker.py, test_simple_py39.py, test_core_py39.py
- Changed from creating asyncio.Lock() in __init__ to lazy initialization - Prevents 'RuntimeError: There is no current event loop' in Python 3.9 - Lock is now created only when needed in _ensure_initialized() - Maintains thread safety and proper initialization semantics - Fixes all SQLite memory service unit test failures in CI Tested with Docker across Python 3.9-3.12, all versions pass.
- Added test_sqlite_docker.py and test_real_sqlite_docker.py - These are development-only testing files for validating the Python 3.9 fix - Keep the repo clean by excluding temporary testing scripts
…API operation parser - Updated OperationParser to use 'object' as default return type instead of 'Any' - This provides better type consistency for OpenAPI operations - Improves schema validation and type inference
- Updated comment on line 167 to refer to 'object' instead of 'Any' - Fixed test assertions in test_operation_parser.py to expect 'Dict[str, Any]' instead of 'Any' - Updated three test functions: test_process_return_value_no_2xx, test_process_return_value_no_content, test_process_return_value_no_schema - All tests now pass with the new default return schema type of 'object'
- Moved 're' import from inside _prepare_fts_query function to top of file with other imports - Follows Python best practices for import organization - Improves code readability and maintainability
Summary of ChangesHello @Raman369AI, 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 significant enhancements to the ADK Python library by improving type safety within the OpenAPI operation parser and integrating a new 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
|
adk-bot
added
bot triaged
services
|
Response from ADK Triaging Agent Hello @Raman369AI, thank you for your contribution! To help us track and manage contributions effectively, could you please create a GitHub issue for this PR and link it in the description? According to our contribution guidelines, all PRs, other than small documentation or typo fixes, should have an associated issue. This will help reviewers better understand the context and purpose of your changes. Thanks! |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new SqliteMemoryService for persistent memory storage using SQLite with FTS5 for full-text search capabilities. The implementation is robust, featuring asynchronous operations, proper database initialization with triggers, and comprehensive error handling. The PR also includes an extensive test suite for the new service, ensuring its correctness and reliability. Additionally, a minor but important improvement is made to the OpenAPI operation parser by changing the default return schema type for better type safety, with corresponding test updates. Overall, these are high-quality changes that enhance the library's functionality and maintainability. I have one suggestion to improve test hygiene by cleaning up a temporary file created during testing.
| service2 = SqliteMemoryService(db_path=temp_db_path + '_good') | ||
| await service2._ensure_initialized() # Initialize successfully first | ||
|
|
||
| # Now corrupt the database file after initialization | ||
| with open(service2._db_path, 'w') as f: | ||
| f.write('corrupted after init') | ||
|
|
||
| # Should handle error gracefully in get_memory_stats | ||
| stats = await service2.get_memory_stats() | ||
| assert 'error' in stats | ||
| assert stats['total_entries'] == 0 |
There was a problem hiding this comment.
This part of the test creates a database file at temp_db_path + '_good', but it doesn't clean it up afterwards. This can lead to test artifacts being left on the file system. The temp_db_path fixture only cleans up the original temporary file it created. It's good practice to ensure all temporary files created during tests are removed. You can use a try...finally block to ensure cleanup.
| service2 = SqliteMemoryService(db_path=temp_db_path + '_good') | |
| await service2._ensure_initialized() # Initialize successfully first | |
| # Now corrupt the database file after initialization | |
| with open(service2._db_path, 'w') as f: | |
| f.write('corrupted after init') | |
| # Should handle error gracefully in get_memory_stats | |
| stats = await service2.get_memory_stats() | |
| assert 'error' in stats | |
| assert stats['total_entries'] == 0 | |
| db_path_good = temp_db_path + '_good' | |
| service2 = SqliteMemoryService(db_path=db_path_good) | |
| try: | |
| await service2._ensure_initialized() # Initialize successfully first | |
| # Now corrupt the database file after initialization | |
| with open(service2._db_path, 'w') as f: | |
| f.write('corrupted after init') | |
| # Should handle error gracefully in get_memory_stats | |
| stats = await service2.get_memory_stats() | |
| assert 'error' in stats | |
| assert stats['total_entries'] == 0 | |
| finally: | |
| Path(db_path_good).unlink(missing_ok=True) |
|
Closing this PR as the changes will be included in PR #2768 which already contains the SqliteMemoryService feature. Our improvements (OpenAPI parser type safety and import organization) will be part of that larger feature PR. |
3 participants
Summary
This PR addresses several improvements to enhance type safety and code organization in the ADK Python library:
Changes Made
1. OpenAPI Operation Parser Type Safety
2. Comment Consistency
3. Test Updates
Tests Updated:
4. Import Organization
Testing
✅ All tests passing:
Impact
Verification
The changes have been thoroughly tested and all existing tests continue to pass with the updated behavior. The type system improvements provide better safety while maintaining full backward compatibility.
This is a low-risk improvement that enhances the robustness and maintainability of the codebase.