feat(extensions): add semantic memory extension for agent knowledge sharing#421
Closed
Subhajitdas99 wants to merge 1 commit intoGetBindu:mainfrom
Closed
feat(extensions): add semantic memory extension for agent knowledge sharing#421Subhajitdas99 wants to merge 1 commit intoGetBindu:mainfrom
Subhajitdas99 wants to merge 1 commit intoGetBindu:mainfrom
Conversation
📝 WalkthroughWalkthroughA new semantic memory extension package is introduced with vector-based storage and retrieval functionality. The package includes embedding generation via OpenAI API, in-memory entry storage, cosine similarity-based querying, and an optional dependency group for required libraries. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryFunc as query_memory()
participant EmbedFunc as get_embedding()
participant OpenAI as OpenAI API
participant Store as SemanticMemoryStore
Client->>QueryFunc: query_memory(query_text, store, top_k, threshold)
QueryFunc->>Store: get_all()
Store-->>QueryFunc: [MemoryEntry, ...]
QueryFunc->>EmbedFunc: get_embedding(query_text)
EmbedFunc->>OpenAI: embeddings request
OpenAI-->>EmbedFunc: embedding vector
EmbedFunc-->>QueryFunc: query_embedding
Note over QueryFunc: For each stored entry:<br/>compute cosine_similarity()<br/>filter by threshold<br/>sort by score desc
QueryFunc-->>Client: top_k results [MemoryEntry, ...]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
bindu/extensions/semantic_memory/__init__.py (1)
25-31: Consider sorting__all__for consistency.Ruff flags that
__all__is not sorted. This is a minor style preference but aligns with isort conventions.Suggested fix
__all__ = [ + "MemoryEntry", "SemanticMemoryStore", - "MemoryEntry", + "cosine_similarity", "get_embedding", "query_memory", - "cosine_similarity", ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/semantic_memory/__init__.py` around lines 25 - 31, The __all__ list in bindu/extensions/semantic_memory/__init__.py is not sorted; update the __all__ declaration so the exported names are alphabetically ordered (e.g., "cosine_similarity", "get_embedding", "MemoryEntry", "SemanticMemoryStore", "query_memory") to satisfy Ruff/isort style; locate the __all__ variable and reorder the string entries accordingly.bindu/extensions/semantic_memory/retriever.py (2)
28-29: Minor: Replace EN DASH with HYPHEN-MINUS in comments/docstrings.Ruff flags ambiguous EN DASH characters (
–) that should be standard hyphens (-). This prevents potential encoding issues.Suggested fix
-# Minimum cosine similarity to consider a memory relevant (0.0 – 1.0) +# Minimum cosine similarity to consider a memory relevant (0.0 - 1.0)And in the docstring at line 66:
- threshold: Minimum cosine similarity score (0.0–1.0). + threshold: Minimum cosine similarity score (0.0-1.0).Also applies to: 66-66
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/semantic_memory/retriever.py` around lines 28 - 29, Replace the ambiguous EN DASH characters with standard hyphen-minus in comments/docstrings: update the comment for DEFAULT_SIMILARITY_THRESHOLD and the docstring at the referenced location (around line with the docstring) to use "-" instead of "–" so encoding/linters stop flagging them; search for the EN DASH glyph in retriever.py and replace each occurrence with the ASCII hyphen-minus.
43-43: Addstrict=Truetozip()to catch dimension mismatches.If embeddings have different dimensions (e.g., mixing models),
zip()silently truncates to the shorter length, producing incorrect similarity scores. Addingstrict=Truewill raiseValueErroron length mismatch, making such bugs visible.Suggested fix
- dot = sum(x * y for x, y in zip(a, b)) + dot = sum(x * y for x, y in zip(a, b, strict=True))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/semantic_memory/retriever.py` at line 43, The line computing the dot product uses zip(a, b) which silently truncates mismatched embedding lengths; change it to zip(a, b, strict=True) in retriever.py (the expression "dot = sum(x * y for x, y in zip(a, b))") and, if desired, wrap that operation in a try/except catching ValueError to re-raise a clearer error that names the embeddings or model source so dimension mismatches are surfaced immediately.bindu/extensions/semantic_memory/embeddings.py (1)
62-66: Consider making the embedding model configurable.The model
text-embedding-3-smallis hardcoded. For flexibility, consider adding amodelparameter with this as the default, allowing users to switch models without code changes.Suggested signature change
-def get_embedding(text: str, api_key: str | None = None, base_url: str | None = None) -> List[float]: +def get_embedding( + text: str, + api_key: str | None = None, + base_url: str | None = None, + model: str = "text-embedding-3-small", +) -> List[float]:Then use
model=modelin theembeddings.create()call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bindu/extensions/semantic_memory/embeddings.py` around lines 62 - 66, The embedding model is hardcoded in the client.embeddings.create call; make it configurable by adding a model parameter (defaulting to "text-embedding-3-small") to the surrounding function or initializer and pass that parameter into client.embeddings.create as model=model so callers can override the embedding model without changing code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bindu/extensions/semantic_memory/memory_store.py`:
- Around line 29-38: The docstring for class SemanticMemoryStore incorrectly
claims "Thread-safe" while the implementation uses a plain list; update the
class docstring to remove or revise the "Thread-safe" claim and clearly state
that the current in-memory store is safe for single-threaded/async usage but is
not safe for concurrent access across threads, and mention how to make it
thread-safe (e.g., wrap accesses with threading.Lock) or recommend using a
vector DB for production persistence; reference the class name
SemanticMemoryStore and any mentions of MEMORY_STORE in the docstring to locate
and change the wording.
---
Nitpick comments:
In `@bindu/extensions/semantic_memory/__init__.py`:
- Around line 25-31: The __all__ list in
bindu/extensions/semantic_memory/__init__.py is not sorted; update the __all__
declaration so the exported names are alphabetically ordered (e.g.,
"cosine_similarity", "get_embedding", "MemoryEntry", "SemanticMemoryStore",
"query_memory") to satisfy Ruff/isort style; locate the __all__ variable and
reorder the string entries accordingly.
In `@bindu/extensions/semantic_memory/embeddings.py`:
- Around line 62-66: The embedding model is hardcoded in the
client.embeddings.create call; make it configurable by adding a model parameter
(defaulting to "text-embedding-3-small") to the surrounding function or
initializer and pass that parameter into client.embeddings.create as model=model
so callers can override the embedding model without changing code.
In `@bindu/extensions/semantic_memory/retriever.py`:
- Around line 28-29: Replace the ambiguous EN DASH characters with standard
hyphen-minus in comments/docstrings: update the comment for
DEFAULT_SIMILARITY_THRESHOLD and the docstring at the referenced location
(around line with the docstring) to use "-" instead of "–" so encoding/linters
stop flagging them; search for the EN DASH glyph in retriever.py and replace
each occurrence with the ASCII hyphen-minus.
- Line 43: The line computing the dot product uses zip(a, b) which silently
truncates mismatched embedding lengths; change it to zip(a, b, strict=True) in
retriever.py (the expression "dot = sum(x * y for x, y in zip(a, b))") and, if
desired, wrap that operation in a try/except catching ValueError to re-raise a
clearer error that names the embeddings or model source so dimension mismatches
are surfaced immediately.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b85132fc-939f-41c5-930a-bd7de4f8e265
📒 Files selected for processing (5)
bindu/extensions/semantic_memory/__init__.pybindu/extensions/semantic_memory/embeddings.pybindu/extensions/semantic_memory/memory_store.pybindu/extensions/semantic_memory/retriever.pypyproject.toml
Comment on lines
+29
to
+38
| class SemanticMemoryStore: | ||
| """Thread-safe in-memory store for agent knowledge. | ||
|
|
||
| Uses instance-level storage instead of a global list, making it | ||
| safe for use in async web workers where a shared global MEMORY_STORE | ||
| would be visible across all requests and agents. | ||
|
|
||
| For production use with persistence across restarts or workers, | ||
| replace this with a vector database backend (e.g. pgvector, Qdrant). | ||
| """ |
There was a problem hiding this comment.
Misleading "Thread-safe" claim in docstring.
The docstring claims this is "Thread-safe" but the implementation uses a plain list without synchronization primitives. While list.append() happens to be atomic in CPython due to the GIL, this is an implementation detail and not guaranteed. The store is safe for async (single-threaded) usage, but the docstring should be corrected to avoid confusion.
Suggested docstring fix
class SemanticMemoryStore:
- """Thread-safe in-memory store for agent knowledge.
+ """In-memory store for agent knowledge.
Uses instance-level storage instead of a global list, making it
- safe for use in async web workers where a shared global MEMORY_STORE
- would be visible across all requests and agents.
+ safe for use within a single async context where a shared global
+ MEMORY_STORE would be visible across all requests and agents.
+
+ Note: This implementation is not thread-safe. For multi-threaded
+ usage, wrap access with appropriate synchronization.
For production use with persistence across restarts or workers,
replace this with a vector database backend (e.g. pgvector, Qdrant).
"""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@bindu/extensions/semantic_memory/memory_store.py` around lines 29 - 38, The
docstring for class SemanticMemoryStore incorrectly claims "Thread-safe" while
the implementation uses a plain list; update the class docstring to remove or
revise the "Thread-safe" claim and clearly state that the current in-memory
store is safe for single-threaded/async usage but is not safe for concurrent
access across threads, and mention how to make it thread-safe (e.g., wrap
accesses with threading.Lock) or recommend using a vector DB for production
persistence; reference the class name SemanticMemoryStore and any mentions of
MEMORY_STORE in the docstring to locate and change the wording.
Contributor
|
We dont need this - lets discuss before making any changes. |
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
2 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
bindu/extensions/semantic_memory/with a class-based store, vector embeddings via OpenRouter, and cosine similarity retrieval.Addresses all review feedback from @chandan-1427 on #350.
Change Type
Scope
Linked Issue/PR
Changes from original #350 implementation
MEMORY_STORE = []unsafe across async workersSemanticMemoryStoreclass — each instance has isolated storage[0.0] * 1536dummy fallback causes silent failuresImportError,ValueError, orRuntimeErrorwith clear messagesopenaias core dependencypip install bindu[semantic-memory]API
Installation
Security Impact
NoNo— API key via env var onlyYes— outbound only to OpenRouter embeddings APINo— in-memory onlyNoChecklist
Summary by CodeRabbit
pip install bindu[semantic-memory]