perf: batch writes, bulk mtime dedup, concurrent mining, entity STOPWORDS - #628
perf: batch writes, bulk mtime dedup, concurrent mining, entity STOPWORDS#628jphein wants to merge 1 commit into
Conversation
…ORDS Split from MemPalace#562 per maintainer request. - palace.py: epsilon mtime comparison for float dedup, cosine distance space, bulk_check_mined() pre-fetch to eliminate N+1 queries - miner.py: batch ChromaDB upserts (one per file instead of per chunk), ThreadPoolExecutor concurrent mining with --workers flag, _prepare_file() helper for thread-safe read/chunk/route, junk file filter (SKIP_PATTERNS + JUNK_FILE_SIZE), word-boundary keyword matching in detect_room(), configurable chunk params, paginated status() past 10K drawers - convo_miner.py: batch upserts in groups of 100 instead of per-chunk writes - entity_detector.py: 73 technical STOPWORDS (Handler, Node, Service, etc.) to reduce false-positive entity detection - cli.py: --workers argument for parallel mining
There was a problem hiding this comment.
Pull request overview
This PR focuses on improving mining throughput and reducing redundant ChromaDB work by batching reads/writes, adding a bulk “already mined” prefetch, and enabling concurrent file preparation during project mining.
Changes:
- Updated Chroma collection creation and file mtime dedup logic, including a new
bulk_check_mined()to avoid per-file lookups. - Reworked project mining to support batched upserts and optional multi-worker concurrent preparation (
--workers). - Batched conversation mining upserts and expanded entity detector STOPWORDS to reduce false positives.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| mempalace/palace.py | Adds cosine-space default on collection creation; introduces bulk prefetch of mined mtimes. |
| mempalace/miner.py | Adds skip filters, word-boundary routing, configurable chunking, concurrent mining, and paginated status. |
| mempalace/convo_miner.py | Switches from per-chunk upserts to batched upserts per file. |
| mempalace/entity_detector.py | Extends STOPWORDS list with common technical/documentation terms. |
| mempalace/cli.py | Adds --workers flag to the mine subcommand and passes it through. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for meta in batch["metadatas"]: | ||
| src = meta.get("source_file") | ||
| mtime = meta.get("source_mtime") | ||
| if src and mtime is not None: | ||
| mined[src] = float(mtime) | ||
| if not batch["ids"]: | ||
| break | ||
| offset += len(batch["ids"]) |
There was a problem hiding this comment.
bulk_check_mined() assumes every entry in batch["metadatas"] is a dict, but Chroma can return None/empty metadata entries. A single None will raise AttributeError (meta.get) and abort the bulk fetch early, defeating the N+1 elimination. Consider skipping non-dict metadatas (e.g., if not isinstance(meta, dict): continue) and continuing the pagination loop rather than bailing out of the whole function.
| for meta in batch["metadatas"]: | |
| src = meta.get("source_file") | |
| mtime = meta.get("source_mtime") | |
| if src and mtime is not None: | |
| mined[src] = float(mtime) | |
| if not batch["ids"]: | |
| break | |
| offset += len(batch["ids"]) | |
| for meta in batch.get("metadatas") or []: | |
| if not isinstance(meta, dict): | |
| continue | |
| src = meta.get("source_file") | |
| mtime = meta.get("source_mtime") | |
| if src and mtime is not None: | |
| mined[src] = float(mtime) | |
| batch_ids = batch.get("ids") or [] | |
| if not batch_ids: | |
| break | |
| offset += len(batch_ids) |
| cfg_chunk_size = palace_config.chunk_size | ||
| cfg_chunk_overlap = palace_config.chunk_overlap | ||
| cfg_min_chunk_size = palace_config.min_chunk_size |
There was a problem hiding this comment.
mine() reads chunk sizing from MempalaceConfig (palace_config.chunk_size/chunk_overlap/min_chunk_size), but MempalaceConfig currently doesn’t define these properties. This will raise AttributeError at runtime the first time mine() is called. Either add these config properties (with defaults) to mempalace/config.py or fall back to the module constants (CHUNK_SIZE/CHUNK_OVERLAP/MIN_CHUNK_SIZE) when the config doesn’t provide overrides.
| cfg_chunk_size = palace_config.chunk_size | |
| cfg_chunk_overlap = palace_config.chunk_overlap | |
| cfg_min_chunk_size = palace_config.min_chunk_size | |
| cfg_chunk_size = getattr(palace_config, "chunk_size", CHUNK_SIZE) | |
| cfg_chunk_overlap = getattr(palace_config, "chunk_overlap", CHUNK_OVERLAP) | |
| cfg_min_chunk_size = getattr(palace_config, "min_chunk_size", MIN_CHUNK_SIZE) |
| total_drawers += len(batch_docs) | ||
| room_counts[room or "general"] += 1 | ||
| pending_docs.extend(batch_docs) | ||
| pending_ids.extend(batch_ids) | ||
| pending_metas.extend(batch_metas) | ||
|
|
||
| # Flush when batch is large enough | ||
| if len(pending_docs) >= _UPSERT_BATCH_SIZE: | ||
| collection.upsert( | ||
| documents=pending_docs, | ||
| ids=pending_ids, | ||
| metadatas=pending_metas, | ||
| ) |
There was a problem hiding this comment.
Concurrent mining path upserts prepared chunks without first deleting existing drawers for that source_file. This bypasses the delete+insert safety in process_file() (added to avoid hnswlib updatePoint segfaults and to prevent stale drawers when a file’s chunk count shrinks). Consider issuing collection.delete(where={"source_file": str(filepath)}) for each processed file before its docs are added/flushed, or restructuring writes to preserve the same per-file delete+insert behavior in workers>1 mode.
| @@ -546,11 +698,26 @@ def mine( | |||
| dry_run: bool = False, | |||
| respect_gitignore: bool = True, | |||
| include_ignored: list = None, | |||
| workers: int = 0, | |||
| ): | |||
| """Mine a project directory into the palace.""" | |||
| """Mine a project directory into the palace. | |||
|
|
|||
| When workers > 1, files are read/chunked/routed in parallel threads | |||
| and then written to ChromaDB sequentially (the Python client is not | |||
| thread-safe for concurrent writes to the same collection). | |||
| """ | |||
| import concurrent.futures | |||
There was a problem hiding this comment.
New workers>1 concurrent mining behavior (bulk_check_mined filtering + ThreadPoolExecutor prepare + batched upserts) isn’t covered by tests. Since tests/test_miner.py already exists for mine()/scan_project(), it would be valuable to add at least one integration test that runs mine(..., workers=2) against a temp project and asserts drawers are written and modified-file re-mines don’t leave stale drawers.
| drawers_added = 0 | ||
| _ADD_BATCH_SIZE = 100 | ||
| if batch_docs: | ||
| for batch_start in range(0, len(batch_docs), _ADD_BATCH_SIZE): | ||
| batch_end = batch_start + _ADD_BATCH_SIZE |
There was a problem hiding this comment.
_ADD_BATCH_SIZE is redefined on every file iteration. Since it’s a constant, consider moving it to module scope (or at least defining it once outside the per-file loop) to keep the hot path simpler and avoid repeated rebinding.
Split from #562 per maintainer request.
Changes
palace.pyabs(stored - current) < 0.01instead of==. JSON round-trips lose float precision; strict equality caused unnecessary re-mining.create_collection(..., metadata={"hnsw:space": "cosine"})so new palaces get cosine similarity by default.bulk_check_mined()— Pre-fetch all source_file/source_mtime pairs in paginated batches. Eliminates the N+1 query pattern where each file triggered a separate ChromaDB lookup.miner.pycollection.upsert()per file instead of per chunk. Reduces write overhead ~10x on typical files.ThreadPoolExecutorconcurrent mining —--workersflag (default:min(8, cpu_count)) parallelizes file read/chunk/route while keeping DB writes sequential (ChromaDB client is not thread-safe)._prepare_file()helper — Pure-computation half ofprocess_file(), safe for concurrent use.bulk_check_mined()integration — Single bulk fetch replaces per-file dedup queries in the concurrent path.SKIP_PATTERNS(minified JS, lockfiles, source maps) andJUNK_FILE_SIZE(500 KB) skip files that are technically text but useless for semantic search.detect_room()usesre.findall(r'\b...\b')instead of substringcount(), preventing false room routing (e.g., "test" matching "contest").chunk_text()accepts optionalchunk_size,chunk_overlap,min_chunk_sizeoverrides.status()— Reads metadata in batches instead of a singlelimit=10000call, fixing the 10K-drawer ceiling.convo_miner.pycollection.upsert()per chunk.entity_detector.pycli.py--workersargument onminesubcommand, passed through tomine().Test plan
python -m pytest tests/ -x -q— all existing tests passmempalace mine <dir>— verify batch writes produce identical palace contentmempalace mine <dir> --workers 1— sequential path still worksmempalace mine <dir> --workers 4— concurrent path works, no segfaultsmempalace status— verify pagination works past 10K drawersmempalace mine <dir> --dry-run— dry run still reports correctly