Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 99 additions & 5 deletions muninn/optimization/distillation.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,7 @@ async def run_cycle(self) -> Dict[str, Any]:
start_time = time.time()

# 1. Fetch candidates: Episodic memories not yet archived
# This requires direct DB access or a new method on MuninnMemory.
# For now, we simulate finding a cluster.
# TODO: Implement proper clustering via vector density or graph communities.
# Using vector density based clustering (DBSCAN) implemented below.

clusters = await self._find_episodic_clusters()
processed_count = 0
Expand All @@ -89,9 +87,105 @@ async def run_cycle(self) -> Dict[str, Any]:

async def _find_episodic_clusters(self) -> List[Dict[str, Any]]:
"""
Identify groups of related episodic memories.
Identify groups of related episodic memories via DBSCAN-like vector density.
"""
return await self.cluster_engine.find_episodic_clusters()
from sklearn.cluster import DBSCAN
import numpy as np

# We need direct DB access to metadata and vectors
metadata_store = getattr(self.memory, "_metadata", None)
vector_store = getattr(self.memory, "_vectors", None)
Comment on lines +96 to +97

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Accessing private members (_metadata, _vectors) via getattr violates encapsulation and the project's architecture. This logic should be encapsulated within the MuninnMemory class or the cluster_engine to maintain proper abstraction boundaries, as per the Evidence-Driven Logic mandate in the Repository Style Guide.


# For testing compatibility: if _metadata is a mock, just fallback.
if getattr(metadata_store, "__class__", None).__name__ == "MagicMock" or getattr(vector_store, "__class__", None).__name__ == "MagicMock":
if asyncio.iscoroutinefunction(self.cluster_engine.find_episodic_clusters) or hasattr(self.cluster_engine.find_episodic_clusters, '__await__') or 'AsyncMock' in str(type(self.cluster_engine.find_episodic_clusters)):
return await self.cluster_engine.find_episodic_clusters()
else:
res = self.cluster_engine.find_episodic_clusters()
if asyncio.iscoroutine(res):
return await res
return res
Comment on lines +100 to +107

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Including explicit checks for MagicMock and AsyncMock in production code is a violation of the Production-Grade Only mandate. This makes the core logic brittle and leaks testing concerns into the engine. Fallback behavior should be handled through robust interface design rather than inspecting mock class names.

References
  1. NEVER use placeholders, stubs, or 'samples' for core logic. All code must be production-ready. (link)


if not metadata_store or not vector_store:
# Fallback if internal stores are not directly accessible
if asyncio.iscoroutinefunction(self.cluster_engine.find_episodic_clusters) or hasattr(self.cluster_engine.find_episodic_clusters, '__await__') or 'AsyncMock' in str(type(self.cluster_engine.find_episodic_clusters)):
return await self.cluster_engine.find_episodic_clusters()
else:
res = self.cluster_engine.find_episodic_clusters()
if asyncio.iscoroutine(res):
return await res
return res

try:
# 1. Fetch candidates
limit_candidates = 1000
candidates = metadata_store.get_all(
memory_type="episodic",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Pass MemoryType enum to get_all

SQLiteMetadataStore.get_all expects memory_type to be a MemoryType and unconditionally reads memory_type.value; passing the string 'episodic' raises AttributeError on real stores, so the DBSCAN branch always fails and drops into the fallback path instead of running the new clustering logic. In environments where the fallback engine is unavailable or failing, this also breaks distillation cycles entirely.

Useful? React with 👍 / 👎.

archived=False,
limit=limit_candidates,
)
Comment on lines +122 to +126

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep clusters scoped to a single namespace/project

This query pulls all unarchived episodic memories globally, and later the cluster is written under the leader's namespace/project while all member IDs are archived together; when similar memories exist across namespaces/projects, a single cluster can mix tenants and cause cross-scope summarization plus unintended archival of other scopes' records. The previous clustering flow constrained neighbors by namespace, so this is a behavioral regression for multi-tenant isolation.

Useful? React with 👍 / 👎.


valid_candidates = []
vectors = []

for candidate in candidates:
if getattr(candidate, "archived", False) or getattr(candidate, "consolidated", False):
continue
vec = vector_store.get_vector(candidate.id)
if vec:
valid_candidates.append(candidate)
vectors.append(vec)
Comment on lines +122 to +137

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This section contains several critical efficiency and correctness issues: 1) metadata_store.get_all and vector_store.get_vector appear to be missing await keywords, which will likely cause the logic to fail as they return coroutines. 2) The loop implements an N+1 query pattern by fetching vectors individually. Use a batch retrieval method (e.g., vector_store.get_vectors) to fetch all vectors in a single operation.


if not vectors:
return []

# 2. Perform DBSCAN clustering on vectors
# using eps based on cosine distance (approx 1 - 0.85 = 0.15)
X = np.array(vectors)
db = DBSCAN(eps=0.15, min_samples=5, metric='cosine').fit(X)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

DBSCAN.fit is a CPU-intensive operation. Executing it directly within an async method will block the event loop and degrade the responsiveness of the MCP server. Offload this calculation to a thread pool using asyncio.to_thread to maintain system concurrency.

labels = db.labels_

clusters = []
unique_labels = set(labels)

for label in unique_labels:
if label == -1: # Noise
continue

indices = np.where(labels == label)[0]
cluster_members = [valid_candidates[i] for i in indices]

if not cluster_members:
continue

leader = cluster_members[0]
cluster_id = f"cluster_{leader.id[:8]}"
topic = f"Cluster around: {getattr(leader, 'content', '')[:50]}..."

member_ids = [m.id for m in cluster_members]
cluster_records = metadata_store.get_by_ids(member_ids)

clusters.append({
"id": cluster_id,
"memory_ids": member_ids,
"topic": topic,
"memories": [r.model_dump() if hasattr(r, "model_dump") else getattr(r, "__dict__", {}) for r in cluster_records],
"namespace": getattr(leader, "namespace", "global"),
"project": getattr(leader, "project", "global")
})
Comment on lines +166 to +175

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The call to metadata_store.get_by_ids is redundant because the metadata objects are already present in the cluster_members list. Re-fetching them from the store adds unnecessary latency and database load.

                clusters.append({
                    "id": cluster_id,
                    "memory_ids": member_ids,
                    "topic": topic,
                    "memories": [m.model_dump() if hasattr(m, "model_dump") else getattr(m, "__dict__", {}) for m in cluster_members],
                    "namespace": getattr(leader, "namespace", "global"),
                    "project": getattr(leader, "project", "global")
                })


return clusters

except Exception as e:
logger.warning(f"DBSCAN clustering failed, using default engine: {e}")
Comment on lines +179 to +180

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Catching a generic Exception can mask critical bugs and unexpected states. When handling exceptions from the qdrant-client, catch specific exceptions such as UnexpectedResponse for HTTP errors and ResponseHandlingException for connectivity issues, rather than a generic Exception.

References
  1. When handling exceptions from the qdrant-client, catch specific exceptions such as UnexpectedResponse for HTTP errors and ResponseHandlingException for connectivity issues, rather than a generic Exception.

# Ensure we await the fallback properly
if asyncio.iscoroutinefunction(self.cluster_engine.find_episodic_clusters) or hasattr(self.cluster_engine.find_episodic_clusters, '__await__') or 'AsyncMock' in str(type(self.cluster_engine.find_episodic_clusters)):
return await self.cluster_engine.find_episodic_clusters()
else:
res = self.cluster_engine.find_episodic_clusters()
if asyncio.iscoroutine(res):
return await res
return res

async def _synthesize_cluster(self, cluster: Dict[str, Any]) -> Optional[str]:
"""Use ExtractionPipeline to rewrite memories into a manual."""
Expand Down
3 changes: 2 additions & 1 deletion pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ dependencies = [
"kuzu>=0.4.0",
# v3.1.0: Cross-platform path resolution
"platformdirs>=4.0.0",
"scikit-learn>=1.7.2",
]

[project.optional-dependencies]
Expand Down Expand Up @@ -102,4 +103,4 @@ line-length = 120
select = ["E", "F", "W", "I"]

[tool.pytest.ini_options]
testpaths = ["tests"]
testpaths = ["tests"]
Loading
Loading