Conversation
Summary of ChangesHello @jinliyl, 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 new pure-Python local memory store, providing a lightweight, in-memory solution with disk persistence for managing memory chunks and file metadata. Alongside this, a robust disk-based caching mechanism has been integrated into embedding models, which will drastically reduce redundant API calls and enhance overall performance. These changes offer greater flexibility in memory backend choices and optimize embedding generation processes. 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
|
![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 pure-Python LocalMemoryStore, which is a great addition for setups without external dependencies. The implementation also adds a persistent on-disk cache for embeddings, which will significantly improve performance by reducing redundant API calls. The code is well-structured and the new functionality is integrated cleanly. I've left a few comments with suggestions to improve the performance and memory efficiency of the new LocalMemoryStore's persistence mechanism, fix some overly verbose logging, and correct a minor bug in the test cleanup logic. Overall, this is a solid contribution.
| await self._save_chunks() | ||
| await self._save_metadata() |
There was a problem hiding this comment.
Calling _save_chunks() and _save_metadata() on every write operation (like upsert_file) is inefficient as it rewrites the entire persistence files each time. This can cause significant performance degradation with a larger number of chunks. It's better to rely on the close() method to persist the final state. Please consider removing these immediate save calls from upsert_file, delete_file, delete_file_chunks, and upsert_chunks.
| async def _load_chunks(self) -> None: | ||
| """Load chunks from JSONL file into memory.""" | ||
| if not self._chunks_file.exists(): | ||
| return | ||
| try: | ||
| data = await self._run_sync_in_executor( | ||
| self._chunks_file.read_text, | ||
| encoding="utf-8", | ||
| ) | ||
| self._chunks = {} | ||
| for line in data.strip().split("\n"): | ||
| if not line: | ||
| continue | ||
| rec = json.loads(line) | ||
| chunk_id = rec["id"] | ||
| self._chunks[chunk_id] = _ChunkRecord(**rec) | ||
| logger.debug(f"Loaded {len(self._chunks)} chunks from {self._chunks_file}") | ||
| except Exception as e: | ||
| logger.warning(f"Failed to load chunks from {self._chunks_file}: {e}") | ||
|
|
||
| async def _save_chunks(self) -> None: | ||
| """Persist chunks to JSONL file.""" | ||
| try: | ||
| lines = [] | ||
| for rec in self._chunks.values(): | ||
| chunk_dict = { | ||
| "id": rec.id, | ||
| "path": rec.path, | ||
| "source": rec.source, | ||
| "start_line": rec.start_line, | ||
| "end_line": rec.end_line, | ||
| "text": rec.text, | ||
| "hash": rec.hash, | ||
| "embedding": rec.embedding, | ||
| "updated_at": rec.updated_at, | ||
| } | ||
| lines.append(json.dumps(chunk_dict, ensure_ascii=False)) | ||
| data = "\n".join(lines) | ||
| await self._run_sync_in_executor( | ||
| self._chunks_file.write_text, | ||
| data, | ||
| encoding="utf-8", | ||
| ) | ||
| logger.debug(f"Saved {len(self._chunks)} chunks to {self._chunks_file}") | ||
| except Exception as e: | ||
| logger.error(f"Failed to save chunks to {self._chunks_file}: {e}") |
There was a problem hiding this comment.
The _load_chunks and _save_chunks methods are not memory-efficient. _load_chunks reads the entire file into memory with read_text(), and _save_chunks builds the entire file content in memory before writing. For large stores, this can lead to high memory usage. Consider refactoring these to process the file line-by-line to improve scalability.
| logger.info("\n=== Vector Search Results ===") | ||
| for i, r in enumerate(vector_results[:10], 1): | ||
| snippet_preview = (r.snippet[:100] + "...") if len(r.snippet) > 100 else r.snippet | ||
| logger.info(f"{i}. Score: {r.score:.4f} | Snippet: {snippet_preview}") | ||
|
|
||
| logger.info("\n=== Keyword Search Results ===") | ||
| for i, r in enumerate(keyword_results[:10], 1): | ||
| snippet_preview = (r.snippet[:100] + "...") if len(r.snippet) > 100 else r.snippet | ||
| logger.info(f"{i}. Score: {r.score:.4f} | Snippet: {snippet_preview}") |
There was a problem hiding this comment.
The logging of search results and snippets at the INFO level can be very verbose and might expose sensitive content from user files in production logs. It's better to use DEBUG level for this kind of detailed diagnostic information. This also applies to the merged hybrid results logging further down.
| logger.info("\n=== Vector Search Results ===") | |
| for i, r in enumerate(vector_results[:10], 1): | |
| snippet_preview = (r.snippet[:100] + "...") if len(r.snippet) > 100 else r.snippet | |
| logger.info(f"{i}. Score: {r.score:.4f} | Snippet: {snippet_preview}") | |
| logger.info("\n=== Keyword Search Results ===") | |
| for i, r in enumerate(keyword_results[:10], 1): | |
| snippet_preview = (r.snippet[:100] + "...") if len(r.snippet) > 100 else r.snippet | |
| logger.info(f"{i}. Score: {r.score:.4f} | Snippet: {snippet_preview}") | |
| logger.debug("\n=== Vector Search Results ===") | |
| for i, r in enumerate(vector_results[:10], 1): | |
| snippet_preview = (r.snippet[:100] + "...") if len(r.snippet) > 100 else r.snippet | |
| logger.debug(f"{i}. Score: {r.score:.4f} | Snippet: {snippet_preview}") | |
| logger.debug("\n=== Keyword Search Results ===") | |
| for i, r in enumerate(keyword_results[:10], 1): | |
| snippet_preview = (r.snippet[:100] + "...") if len(r.snippet) > 100 else r.snippet | |
| logger.debug(f"{i}. Score: {r.score:.4f} | Snippet: {snippet_preview}") |
tests/test_memory_store.py
Outdated
| if db_dir.exists(): | ||
| shutil.rmtree(db_dir) | ||
| logger.info(f"✓ Cleaned up directory: {db_dir}") | ||
| for suffix in ("_chunks.json", "_file_metadata.json"): |
There was a problem hiding this comment.
There's a small bug in the cleanup logic for LocalMemoryStore. The chunks are persisted to a file named _chunks.jsonl (JSON Lines format), but the cleanup code is looking for _chunks.json. This will cause the test to leave the chunks file behind.
| for suffix in ("_chunks.json", "_file_metadata.json"): | |
| for suffix in ("_chunks.jsonl", "_file_metadata.json"): |
1 participant
No description provided.