fix(bench): mempalace adapter uses raw chromadb - #36
Conversation
…como_bench pattern) The prior adapter tried two broken paths: 1. mempalace.layers.MemoryStack.add() — method doesn't exist; attempt silently falls through 2. subprocess mempalace mine <palace> — passes palace as mine SOURCE dir instead of target; target goes in the top-level --palace flag Meanwhile MemPalace's own benchmarks/locomo_bench.py bypasses both the MemoryStack and the CLI — it uses chromadb directly, one collection per conversation. That codepath is what produced their published LoCoMo R@10 (60.3% raw, 88.9% hybrid v5). Our adapter now matches that pattern so we reproduce their baseline AND extend with Judge scoring on the same stack. Only the ingest + search halves change. Generator, judge, result shape, _summary, CLI, scorecard output all unchanged.
📝 WalkthroughWalkthroughThe benchmark runner replaces MemPalace-based memory retrieval with direct chromadb querying. Ingest now builds document collections per conversation, creates a persistent chromadb client, manages collections with per-conversation naming, and queries via the chromadb API instead of MemPalace searcher. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| return len(lines), elapsed | ||
| except (ImportError, AttributeError): | ||
| pass # Fall through to CLI path. | ||
| client.delete_collection(name=f"locomo_{conv_id}") |
There was a problem hiding this comment.
WARNING: Bare exception swallowing.
Catching generic Exception here will silently ignore critical errors like disk I/O failures, permission errors, and OOM conditions. This can hide ingestion failures that would otherwise fail silently during benchmark runs. Catch only chromadb's CollectionDoesNotExist exception type.
| """Query the per-conv chromadb collection. Returns top-K turn texts.""" | ||
| import chromadb | ||
| client = chromadb.PersistentClient(path=palace) | ||
| coll = client.get_or_create_collection(name=f"locomo_{conv_id}") |
There was a problem hiding this comment.
WARNING: get_or_create_collection will silently creates empty collection.
If the collection does not exist (because ingestion failed, was skipped, or was deleted, this will create an empty collection instead of failing fast. Benchmark will return 0 results instead of reporting an error.
| ) | ||
| return len(lines), elapsed | ||
| pass | ||
| coll = client.create_collection(name=f"locomo_{conv_id}") |
There was a problem hiding this comment.
SUGGESTION: create_collection can fail on race condition.
Between delete_collection and create_collection there is a race window where another process/thread could create the same collection. Use get_or_create_collection with `get_or_create=True parameter would be safer here for idempotency.
| palace here is the chromadb PersistentClient directory; each conversation | ||
| gets its own collection named `locomo_{conv_id}` so search scopes cleanly. | ||
| """ | ||
| import chromadb |
There was a problem hiding this comment.
SUGGESTION: Missing import error handling.
chromadb import is inside function scope without try/except. If chromadb is not installed this will throw an uncaught ImportError at runtime instead of failing cleanly like the original code did.
Code Review SummaryStatus: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 file)
Fix these issues in Kilo Cloud Reviewed by seed-2-0-pro-260328 · 139,163 tokens |
Captures every model actually used during the benchmark (generator variants, external judge, embedders, cross-encoder, fact extractor) with params, quant, VRAM footprint, and backend. Adds the runtime/host row so anyone reproducing knows the Ollama parallel limit and rescore timeout. Derives hardware-tier recommendations from what we measured: - Orange Pi (RK3588 NPU, 16 GB): qwen3:4b gen on rkllama, external judge, MiniLM ONNX embed, taosmd arch - Fedora 3060 (12 GB VRAM): gemma4:e2b gen, qwen3:4b judge co-resident, prompt-opt on by default - Laptop / Mac Mini: qwen3:4b gen via Ollama, external judge - High-end (≥24 GB): qwen3.5:9b gen viable; e2b still competitive Documents the seven lessons that drive the defaults: bigger-gen-≠-better at small scale, qwen for structured output, NUM_PARALLEL is the real ceiling, nomic context forces batching, architecture dominates generator choice, self-judge inflates, R@K needs dia_id round-trip. Also corrects the Commits row: superseded SHAs (ca0ccb7 → 571d8af for mempalace) and references the right open PRs (#34, #35, #36).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@benchmarks/mempalace_locomo_runner.py`:
- Around line 290-294: Normalize conv_id the same way as in _palace_path()
before using it in collection names: compute a sanitized_id (apply the same
replacements/filters used by _palace_path to remove/replace slashes and enforce
lowercase/allowed characters) and use f"locomo_{sanitized_id}" in calls to
client.delete_collection, client.create_collection and when creating coll.add;
remove the broad except pass fallback so invalid raw conv_id no longer relies on
swallowing errors.
- Around line 289-293: Replace the broad try/except around
client.delete_collection(name=f"locomo_{conv_id}") so you don't swallow real
errors: first check client.list_collections() (or
client.list_collections().names) to see if "locomo_{conv_id}" exists and only
call delete_collection if present, otherwise skip; if you prefer catching
exceptions, catch and suppress only the expected ValueError for missing
collections and re-raise/log any other Exception before calling
client.create_collection(name=f"locomo_{conv_id}").
- Around line 266-267: Add a startup preflight in run() before the conversation
loop to import chromadb and fail fast: wrap a top-level import (try: import
chromadb) in a try/except, log or print the ImportError (`e`) and call
sys.exit(1) if it fails so the process exits with status 1 instead of
proceeding; place this check before the conversation processing loop (the loop
that handles conv / `conversation = conv.get("conversation", conv)`) so missing
chromadb is detected early.
- Around line 302-305: The retrieval currently uses
client.get_or_create_collection which silently creates an empty collection and
masks ingestion failures; replace that call with client.get_collection (or the
client method that fetches an existing collection) so retrieval fails loudly
when the collection is missing, then handle the absent-collection case by
raising/logging an error instead of continuing to coll.query; update the code
paths referencing coll, query, and the docs extraction (out.get("documents") /
docs[0]) to assume a valid collection only after get_collection succeeds.
🪄 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 Plus
Run ID: 41e5202c-74f4-450f-b987-36900c9a33ac
📒 Files selected for processing (1)
benchmarks/mempalace_locomo_runner.py
| import chromadb | ||
| conversation = conv.get("conversation", conv) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the recommended fail-fast pattern for optional Python CLI dependencies (e.g., chromadb) to avoid partial-success runs with exit code 0?
💡 Result:
The recommended fail-fast pattern for optional Python CLI dependencies (e.g., chromadb) is to perform an early import check at module top-level or in a CLI callback/eager option, raising ImportError or sys.exit(1) with a helpful message if missing. This ensures the CLI exits immediately with a non-zero code before any partial execution. For plain scripts or argparse/Typer without Click: - At top-level after standard imports: try: import chromadb except ImportError: print("Error: chromadb is required but not installed. Install with: pip install chromadb") sys.exit(1) This catches missing deps before CLI parsing or main logic runs. For Click/Typer (common for CLIs): - Use an eager option with callback for early validation, similar to --version handling. def check_chromadb(ctx, param, value): try: import chromadb except ImportError: print("Error: chromadb required. pip install chromadb") ctx.exit(1) return value # or None @app.command @click.option("--check-deps", is_flag=True, callback=check_chromadb, expose_value=False, is_eager=True, help="Check deps") def main: # CLI logic using chromadb But simpler: top-level try-import + sys.exit(1), as CLI libs process params after imports. Declare as pyproject.toml optional-dependencies: [project.optional-dependencies] chromadb = ["chromadb"] Users install with pip install .[chromadb], but CLI still needs runtime check for fail-fast. This prevents "partial-success" (exit 0 despite failure later), per Click/Typer docs on eager options (@31,32,30,34) and std optional dep patterns (@3). For chromadb specifically, import issues common (@6-10), so early check critical.
Citations:
- 1: https://stackoverflow.com/questions/27361427/how-to-properly-deal-with-optional-features-in-python
- 2: https://typer.tiangolo.com/tutorial/options/version
- 3: https://click.palletsprojects.com/en/8.0.x/advanced/
- 4: https://click.palletsprojects.com/en/stable/advanced/
- 5: https://stackoverflow.com/questions/77084311/optional-dynamic-dependencies-at-python-runtime
- 6: [QUESTION] eagerness to mimic click.option ? fastapi/typer#41
- 7: http://docs.python.org/3/library/sys.html
🏁 Script executed:
# Check the file structure and locate chromadb imports
fd benchmarks/mempalace_locomo_runner.py --exec wc -l {}Repository: jaylfc/taosmd
Length of output: 39
🏁 Script executed:
# Look at the import statements and the run() function
cd benchmarks && head -50 mempalace_locomo_runner.py | cat -nRepository: jaylfc/taosmd
Length of output: 2399
🏁 Script executed:
# Check for chromadb imports throughout the file
rg "import chromadb" benchmarks/mempalace_locomo_runner.py -n -B 3 -A 3Repository: jaylfc/taosmd
Length of output: 746
🏁 Script executed:
# Find the run() function and see if there's any existing error handling
rg "def run\(\)" benchmarks/mempalace_locomo_runner.py -n -A 10Repository: jaylfc/taosmd
Length of output: 39
🏁 Script executed:
# Check if chromadb is imported at module level or only in functions
rg "^import|^from" benchmarks/mempalace_locomo_runner.py -n | head -30Repository: jaylfc/taosmd
Length of output: 299
🏁 Script executed:
# Find the main entry point or run() function
rg "^def run|^async def run|^def main|^async def main" benchmarks/mempalace_locomo_runner.py -n -A 5Repository: jaylfc/taosmd
Length of output: 385
🏁 Script executed:
# Check the bottom of the file for CLI entry point
tail -100 benchmarks/mempalace_locomo_runner.py | cat -nRepository: jaylfc/taosmd
Length of output: 4587
🏁 Script executed:
# Search for sys.exit calls
rg "sys.exit|return 0|return 1" benchmarks/mempalace_locomo_runner.py -nRepository: jaylfc/taosmd
Length of output: 287
🏁 Script executed:
# Find the _guarded function to confirm it calls the ingest/search functions
rg "_guarded|def _guarded" benchmarks/mempalace_locomo_runner.py -n -A 10Repository: jaylfc/taosmd
Length of output: 1068
🏁 Script executed:
# Check if there's any existing chromadb availability check anywhere in the file
rg "ImportError|chromadb" benchmarks/mempalace_locomo_runner.py -nRepository: jaylfc/taosmd
Length of output: 698
🏁 Script executed:
# Check the end of run() to see what exit code is returned
sed -n '396,450p' benchmarks/mempalace_locomo_runner.py | cat -nRepository: jaylfc/taosmd
Length of output: 2509
🏁 Script executed:
# Verify the full path: run() always returns 0 or non-zero?
grep -n "return [0-9]" benchmarks/mempalace_locomo_runner.py | tail -20Repository: jaylfc/taosmd
Length of output: 319
🏁 Script executed:
# Check the full exception handler for the ingest phase (lines 51-60)
sed -n '50,75p' benchmarks/mempalace_locomo_runner.py | cat -nRepository: jaylfc/taosmd
Length of output: 1088
🏁 Script executed:
# Check what happens after the ingest exception is caught
sed -n '50,100p' benchmarks/mempalace_locomo_runner.py | cat -nRepository: jaylfc/taosmd
Length of output: 2106
🏁 Script executed:
# Get the exact lines around the ingest exception handler (likely lines 451-465)
sed -n '451,475p' benchmarks/mempalace_locomo_runner.py | cat -nRepository: jaylfc/taosmd
Length of output: 1330
🏁 Script executed:
# Double-check the structure by looking at the full run() function from start to end
sed -n '396,515p' benchmarks/mempalace_locomo_runner.py | tail -100 | cat -nRepository: jaylfc/taosmd
Length of output: 4506
Add an early chromadb availability check to prevent silent run completion on missing dependency.
With function-scoped imports, a missing chromadb module causes the first ingest to fail (ImportError), which is caught and logged at line 455, then the conversation is skipped and execution continues. The run() function always returns 0 at line 499, allowing the entire benchmark to complete with exit code 0 and zero results if the dependency is unavailable. Add a startup preflight check before the conversation loop (before line 446) to import chromadb early and exit with status 1 if missing, following the standard fail-fast pattern for optional CLI dependencies.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/mempalace_locomo_runner.py` around lines 266 - 267, Add a startup
preflight in run() before the conversation loop to import chromadb and fail
fast: wrap a top-level import (try: import chromadb) in a try/except, log or
print the ImportError (`e`) and call sys.exit(1) if it fails so the process
exits with status 1 instead of proceeding; place this check before the
conversation processing loop (the loop that handles conv / `conversation =
conv.get("conversation", conv)`) so missing chromadb is detected early.
| try: | ||
| from mempalace.layers import MemoryStack # type: ignore | ||
| stack = MemoryStack(palace_path=palace) | ||
| for line in lines: | ||
| stack.add(line) | ||
| elapsed = time.time() - t0 | ||
| return len(lines), elapsed | ||
| except (ImportError, AttributeError): | ||
| pass # Fall through to CLI path. | ||
| client.delete_collection(name=f"locomo_{conv_id}") | ||
| except Exception: | ||
| pass # Also fall through — CLI is the canonical ingest path. | ||
|
|
||
| # --- Attempt 2: CLI `mempalace mine <path>` -------------------------------- | ||
| # Write turns to a temp .txt file inside the palace directory so the crawler | ||
| # picks them up. | ||
| Path(palace).mkdir(parents=True, exist_ok=True) | ||
| stage_file = os.path.join(palace, "_locomo_stage.txt") | ||
| with open(stage_file, "w", encoding="utf-8") as fh: | ||
| fh.write("\n".join(lines)) | ||
|
|
||
| result = subprocess.run( | ||
| ["mempalace", "mine", palace], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| elapsed = time.time() - t0 | ||
| if result.returncode != 0: | ||
| raise RuntimeError( | ||
| f"mempalace mine failed (rc={result.returncode}): {result.stderr.strip()}" | ||
| ) | ||
| return len(lines), elapsed | ||
| pass | ||
| coll = client.create_collection(name=f"locomo_{conv_id}") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "mempalace_locomo_runner.py" | head -20Repository: jaylfc/taosmd
Length of output: 96
🏁 Script executed:
cat -n ./benchmarks/mempalace_locomo_runner.py | sed -n '280,300p'Repository: jaylfc/taosmd
Length of output: 967
🏁 Script executed:
grep -r "list_collections\|delete_collection" --include="*.py" | head -20Repository: jaylfc/taosmd
Length of output: 153
🏁 Script executed:
grep -r "import chromadb\|from chromadb" --include="*.py"Repository: jaylfc/taosmd
Length of output: 172
🏁 Script executed:
grep -B5 -A5 "PersistentClient\|delete_collection\|create_collection" ./benchmarks/mempalace_locomo_runner.py | head -40Repository: jaylfc/taosmd
Length of output: 1689
🏁 Script executed:
grep -r "\.get_collection\|\.list_collection" --include="*.py"Repository: jaylfc/taosmd
Length of output: 39
🌐 Web query:
chromadb.PersistentClient delete_collection exception not found
💡 Result:
When using chromadb.PersistentClient.delete_collection(name), attempting to delete a non-existent collection raises a ValueError with the message "Collection {name} does not exist.". This is the expected behavior as documented in the source code and confirmed across multiple Stack Overflow answers and GitHub issues. To safely delete a collection without raising an exception if it doesn't exist, use a try-except block: try: client.delete_collection("your_collection_name") except ValueError: pass # Collection does not exist, safe to ignore Alternative approaches to check existence first: 1. Check via list_collections: if any(c.name == "your_collection_name" for c in client.list_collections): client.delete_collection("your_collection_name") 2. Try get_collection first (raises ValueError if missing): try: client.get_collection("your_collection_name") client.delete_collection("your_collection_name") except ValueError: pass Note: There is no built-in "delete if exists" method like in some databases. For server mode (HttpClient), deleting non-existent collections may return HTTP 500 in older versions, but local PersistentClient consistently raises ValueError.
Citations:
- 1: https://stackoverflow.com/questions/78175724/chromadb-how-to-check-if-collection-exists
- 2: https://docs.trychroma.com/docs/collections/manage-collections
- 3: https://stackoverflow.com/questions/78175724/chromadb-how-to-check-if-collection-exists/78176923#78176923
- 4: https://cookbook.chromadb.dev/core/collections
- 5: https://github.com/chroma-core/chroma/blob/main/chromadb/api/models/Collection.py
- 6: [Bug]: Deleting a non-existing collection causes a 500 internal server error chroma-core/chroma#1616
Don't swallow all collection deletion errors.
except Exception: pass hides real storage/permission/corruption issues. chromadb's delete_collection() raises ValueError when the collection doesn't exist—only that case is safe to suppress. The suggested approach using list_collections() to check existence before deletion follows chromadb best practices and surfaces other errors.
Suggested hardening
- try:
- client.delete_collection(name=f"locomo_{conv_id}")
- except Exception:
- pass
+ collection_name = f"locomo_{conv_id}"
+ existing = {c.name for c in client.list_collections()}
+ if collection_name in existing:
+ client.delete_collection(name=collection_name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| from mempalace.layers import MemoryStack # type: ignore | |
| stack = MemoryStack(palace_path=palace) | |
| for line in lines: | |
| stack.add(line) | |
| elapsed = time.time() - t0 | |
| return len(lines), elapsed | |
| except (ImportError, AttributeError): | |
| pass # Fall through to CLI path. | |
| client.delete_collection(name=f"locomo_{conv_id}") | |
| except Exception: | |
| pass # Also fall through — CLI is the canonical ingest path. | |
| # --- Attempt 2: CLI `mempalace mine <path>` -------------------------------- | |
| # Write turns to a temp .txt file inside the palace directory so the crawler | |
| # picks them up. | |
| Path(palace).mkdir(parents=True, exist_ok=True) | |
| stage_file = os.path.join(palace, "_locomo_stage.txt") | |
| with open(stage_file, "w", encoding="utf-8") as fh: | |
| fh.write("\n".join(lines)) | |
| result = subprocess.run( | |
| ["mempalace", "mine", palace], | |
| capture_output=True, | |
| text=True, | |
| ) | |
| elapsed = time.time() - t0 | |
| if result.returncode != 0: | |
| raise RuntimeError( | |
| f"mempalace mine failed (rc={result.returncode}): {result.stderr.strip()}" | |
| ) | |
| return len(lines), elapsed | |
| pass | |
| coll = client.create_collection(name=f"locomo_{conv_id}") | |
| collection_name = f"locomo_{conv_id}" | |
| existing = {c.name for c in client.list_collections()} | |
| if collection_name in existing: | |
| client.delete_collection(name=collection_name) | |
| coll = client.create_collection(name=f"locomo_{conv_id}") |
🧰 Tools
🪛 Ruff (0.15.10)
[error] 291-292: try-except-pass detected, consider logging the exception
(S110)
[warning] 291-291: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/mempalace_locomo_runner.py` around lines 289 - 293, Replace the
broad try/except around client.delete_collection(name=f"locomo_{conv_id}") so
you don't swallow real errors: first check client.list_collections() (or
client.list_collections().names) to see if "locomo_{conv_id}" exists and only
call delete_collection if present, otherwise skip; if you prefer catching
exceptions, catch and suppress only the expected ValueError for missing
collections and re-raise/log any other Exception before calling
client.create_collection(name=f"locomo_{conv_id}").
| client.delete_collection(name=f"locomo_{conv_id}") | ||
| except Exception: | ||
| pass # Also fall through — CLI is the canonical ingest path. | ||
|
|
||
| # --- Attempt 2: CLI `mempalace mine <path>` -------------------------------- | ||
| # Write turns to a temp .txt file inside the palace directory so the crawler | ||
| # picks them up. | ||
| Path(palace).mkdir(parents=True, exist_ok=True) | ||
| stage_file = os.path.join(palace, "_locomo_stage.txt") | ||
| with open(stage_file, "w", encoding="utf-8") as fh: | ||
| fh.write("\n".join(lines)) | ||
|
|
||
| result = subprocess.run( | ||
| ["mempalace", "mine", palace], | ||
| capture_output=True, | ||
| text=True, | ||
| ) | ||
| elapsed = time.time() - t0 | ||
| if result.returncode != 0: | ||
| raise RuntimeError( | ||
| f"mempalace mine failed (rc={result.returncode}): {result.stderr.strip()}" | ||
| ) | ||
| return len(lines), elapsed | ||
| pass | ||
| coll = client.create_collection(name=f"locomo_{conv_id}") | ||
| coll.add(documents=docs, ids=ids, metadatas=metas) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the exact ChromaDB collection naming constraints (allowed characters and max length) for current Python client versions?
💡 Result:
The ChromaDB collection naming constraints for current Python client versions (v1.5.x as of April 2026) are: - Length: 3 to 512 characters - Must start and end with a lowercase letter or digit (a-z, 0-9) - Can contain lowercase letters (a-z), digits (0-9), dots (.), dashes (-), and underscores (_) in between - Must not contain two consecutive dots (..) - Must not be a valid IPv4 address These rules are enforced in the Python client and server, as documented in the official Chroma documentation and reflected in the source code's check_index_name function (with relaxed rules from earlier stricter versions).
Citations:
- 1: https://docs.trychroma.com/docs/collections/manage-collections
- 2: https://cookbook.chromadb.dev/core/collections
- 3: https://github.com/chroma-core/chroma/blob/main/chromadb/api/segment.py
🏁 Script executed:
wc -l benchmarks/mempalace_locomo_runner.pyRepository: jaylfc/taosmd
Length of output: 98
🏁 Script executed:
sed -n '280,310p' benchmarks/mempalace_locomo_runner.pyRepository: jaylfc/taosmd
Length of output: 1256
🏁 Script executed:
rg "conv_id" benchmarks/mempalace_locomo_runner.py -B 2 -A 2Repository: jaylfc/taosmd
Length of output: 4003
Normalize conv_id before using it as a Chroma collection name.
The collection name pattern locomo_{conv_id} at lines 290, 292, and 302 uses raw conv_id which may contain characters or formats that violate ChromaDB naming constraints (must be 3–512 chars, start/end with lowercase a-z or 0-9, contain only lowercase a-z, digits, dots, dashes, underscores in between, no consecutive dots, not a valid IPv4 address). The code already sanitizes conv_id in _palace_path() (e.g., replacing "/"); apply the same sanitization to the collection name to prevent silent failures or reliance on the try/except catch.
🧰 Tools
🪛 Ruff (0.15.10)
[error] 291-292: try-except-pass detected, consider logging the exception
(S110)
[warning] 291-291: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/mempalace_locomo_runner.py` around lines 290 - 294, Normalize
conv_id the same way as in _palace_path() before using it in collection names:
compute a sanitized_id (apply the same replacements/filters used by _palace_path
to remove/replace slashes and enforce lowercase/allowed characters) and use
f"locomo_{sanitized_id}" in calls to client.delete_collection,
client.create_collection and when creating coll.add; remove the broad except
pass fallback so invalid raw conv_id no longer relies on swallowing errors.
| coll = client.get_or_create_collection(name=f"locomo_{conv_id}") | ||
| out = coll.query(query_texts=[question], n_results=top_k) | ||
| docs = out.get("documents") or [[]] | ||
| return docs[0] if docs else [] |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full context of this file
wc -l benchmarks/mempalace_locomo_runner.pyRepository: jaylfc/taosmd
Length of output: 98
🏁 Script executed:
# Read the file around lines 302-305 with more context
sed -n '290,320p' benchmarks/mempalace_locomo_runner.pyRepository: jaylfc/taosmd
Length of output: 1124
🏁 Script executed:
# Check the imports at the top of the file to identify the client library
head -50 benchmarks/mempalace_locomo_runner.pyRepository: jaylfc/taosmd
Length of output: 2049
🏁 Script executed:
# Look for where collections are created (not just retrieved) in this file
rg "get_or_create_collection|get_collection|create_collection" benchmarks/mempalace_locomo_runner.py -nRepository: jaylfc/taosmd
Length of output: 195
🏁 Script executed:
# Check if there's any error handling around this function call
sed -n '300,320p' benchmarks/mempalace_locomo_runner.pyRepository: jaylfc/taosmd
Length of output: 706
🏁 Script executed:
# Check how _mempalace_search is called to see if errors are expected
rg "_mempalace_search" benchmarks/mempalace_locomo_runner.py -A 2 -B 2Repository: jaylfc/taosmd
Length of output: 418
Use get_collection in retrieval to avoid silently masking data ingestion failures.
get_or_create_collection silently creates an empty collection when the expected collection doesn't exist (e.g., due to ingest failure or ID mismatch). This causes the query to return empty results without alerting to the underlying problem, which skews benchmark metrics. Retrieval should fail loudly here.
Suggested fix
- coll = client.get_or_create_collection(name=f"locomo_{conv_id}")
+ coll = client.get_collection(name=f"locomo_{conv_id}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| coll = client.get_or_create_collection(name=f"locomo_{conv_id}") | |
| out = coll.query(query_texts=[question], n_results=top_k) | |
| docs = out.get("documents") or [[]] | |
| return docs[0] if docs else [] | |
| coll = client.get_collection(name=f"locomo_{conv_id}") | |
| out = coll.query(query_texts=[question], n_results=top_k) | |
| docs = out.get("documents") or [[]] | |
| return docs[0] if docs else [] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@benchmarks/mempalace_locomo_runner.py` around lines 302 - 305, The retrieval
currently uses client.get_or_create_collection which silently creates an empty
collection and masks ingestion failures; replace that call with
client.get_collection (or the client method that fetches an existing collection)
so retrieval fails loudly when the collection is missing, then handle the
absent-collection case by raising/logging an error instead of continuing to
coll.query; update the code paths referencing coll, query, and the docs
extraction (out.get("documents") / docs[0]) to assume a valid collection only
after get_collection succeeds.
) * docs(specs): LoCoMo scorecard log — taosmd × 3 variants + mem0, rescored Single source of truth for every LoCoMo number we've produced so they don't live only in chat transcripts. Captures: - Self-judge scorecards for taosmd-e2b, taosmd-e4b, taosmd-e2b+prompt-opt, mem0-e2b (all runs 2026-04-17 to 2026-04-19) - External qwen3:4b rescore numbers for the three taosmd variants (100% coverage, 0 errors). mem0 rescore queued. - Per-category tables, not just headlines — Temporal 0.29 vs 0.02 (14.5x) is the most dramatic architecture signal - Known artefacts: mem0 R@K=0.0 is an adapter limitation (no dia_id pass-through), patched in PR #33 - Methodology disclosures: same generator (gemma4:e2b), same prompt, same dataset, same top-K=10, same judge (qwen3:4b), commit SHAs for every input - Follow-up: mem0 external rescore in flight, MemPalace adapter queued — will add scorecards to this doc as they complete * docs(specs): correct stale commit SHAs in scorecard methodology CodeRabbit CRITICAL on #34 caught that 40403cc / 86c4c19 / 3c5c6c2 are no longer reachable — rewritten out of history by PR #30's rebase to a single commit. Replaced with the reachable SHAs and noted that the old ones were intentionally rewritten so anyone reading git log won't be confused. * docs(specs): correct external-judge scorecards + record mem0 rescore Two corrections in one: 1. The external qwen3:4b scorecards table had wrong numbers (0.27 / 0.22 / 0.34 for taosmd variants). Those were the earlier qwen3.5:9b biased- sample numbers that got superseded but I left in the table by mistake. Now corrected to the actual qwen3:4b 100%-coverage numbers (0.40 / 0.38 / 0.41) directly from the streaming rescore log. Per-category rows also restated from source. 2. mem0 rescore completed in 116.9 min, 100% coverage, 0 errors: - Single-hop 0.04 / Temporal 0.02 / Multi-hop 0.10 / Open-dom 0.07 - Overall Judge 0.06 Added to the same table. Biggest architecture gap is Temporal (taosmd-e2b+prompt-opt 0.41 vs mem0 0.02 = 20.5x). Overall gap ~7x under identical external judge, same generator. Also refreshed the "In flight / queued" section: mem0 rescore done, MemPalace adapter already built as `ca0ccb7` (landed in PR #30, ready to run — just needs `pip install mempalace` on the Fedora host). The earlier stale numbers are kept in the caveat block so anyone comparing against chat history or the push notifications knows why they shifted. * docs(specs): add Configuration log + hardware tier recommendations Captures every model actually used during the benchmark (generator variants, external judge, embedders, cross-encoder, fact extractor) with params, quant, VRAM footprint, and backend. Adds the runtime/host row so anyone reproducing knows the Ollama parallel limit and rescore timeout. Derives hardware-tier recommendations from what we measured: - Orange Pi (RK3588 NPU, 16 GB): qwen3:4b gen on rkllama, external judge, MiniLM ONNX embed, taosmd arch - Fedora 3060 (12 GB VRAM): gemma4:e2b gen, qwen3:4b judge co-resident, prompt-opt on by default - Laptop / Mac Mini: qwen3:4b gen via Ollama, external judge - High-end (≥24 GB): qwen3.5:9b gen viable; e2b still competitive Documents the seven lessons that drive the defaults: bigger-gen-≠-better at small scale, qwen for structured output, NUM_PARALLEL is the real ceiling, nomic context forces batching, architecture dominates generator choice, self-judge inflates, R@K needs dia_id round-trip. Also corrects the Commits row: superseded SHAs (ca0ccb7 → 571d8af for mempalace) and references the right open PRs (#34, #35, #36). * docs(specs): MemPalace self-judge landed — surprise on the per-category split MemPalace-e2b full run completed. Self-judge Overall 0.42 — much closer to taosmd (0.48) than to mem0 (0.09). Per-category: - MemPalace beats baseline taosmd on Temporal (0.33 vs 0.29) + Multi-hop (0.24 vs 0.22) - taosmd pulls ahead on Open-dom (0.64 vs 0.51) + Single-hop (0.34 vs 0.29) - prompt-opt variant still the Overall leader at 0.51 - mem0 a distant fourth on every category Story shifts from "taosmd wins by 7x over competitors" to "taosmd and MemPalace are in the same tier, mem0 is much further behind — and raw verbatim-store + a sensible default embedder is a strong baseline on its own." Also added ingest-timing comparison: MemPalace fastest at ~100s for all 10 convs (simpler architecture = less processing per turn). External rescore for MemPalace is running now on Fedora, ETA ~01:55 BST. * docs(specs): MemPalace external rescore complete — final 5-row scorecard MemPalace-e2b external qwen3:4b rescore: Overall Judge 0.34 (180.5 min, 100% coverage, 0 errors). All three architectures now have the same treatment: same generator, same prompt, same external judge, same 1540 QAs. Only the memory layer varies. Final headline numbers (external Judge, gemma4:e2b generator): - taosmd-e2b+prompt-opt 0.41 - taosmd-e2b 0.40 - taosmd-e4b 0.38 - MemPalace-e2b 0.34 - mem0-e2b (infer=False) 0.06 Per-category reveals a more nuanced story than the Overall numbers: - Single-hop: three-way tie at ~0.16-0.17 — solved at this tier by any competent semantic-retrieval system - Temporal: taosmd (0.36) and MemPalace (0.35) nearly tied; only prompt-opt breaks away at 0.41 - Multi-hop: taosmd-opt leads at 0.24; KG + query expansion help on synthesis questions - Open-dom: taosmd's clearest architectural win (0.51 vs MemPalace 0.41, +24% relative) - mem0 distant fourth everywhere Reframes the positioning: taosmd's architecture edge concentrates on harder question types that benefit from rerank + synthesis (Open-dom, Multi-hop); on simpler retrieval (Single-hop, Temporal) MemPalace's verbatim-store + default embedder is nearly as good. Cleaner story than "we dominate" and more useful for positioning against the target audiences documented in project_taosmd_positioning.md. Next: README rewrite aligned with that positioning memory and these nuanced numbers — lead with target audiences (SBC, taOS clusters, offline/compliance, long-horizon agents), frame benchmark numbers as "at the compute tier we target," highlight architectural edge on the categories where it actually shows. * docs(specs): matrix C1-C6 complete — log results, lessons, c_stack in flight - Add Parametric retrieval matrix (C1-C6) scorecard: C3 adjacent_turns is the biggest single-lever win at 0.465; C6 multihop_decompose regresses to 0.317; C5 bge_reranker deferred pending refactor. - Add lessons #8 (multihop decomposition regresses at small-LLM scale) and #9 (context stitching beats retrieval width). - Reorganise 'In flight / queued' section into Complete / In flight / Queued sub-headings. Log the c_stack run currently mid-bench and the three queued follow-ups (qwen9b dense, Qwen3.6 HLWQ via vLLM, Qwen3.6 MoE via Ollama). * docs(specs): adj=2 is new leader at 0.499; stacking is additive (retract yesterday's claim) Five new results logged (2026-04-21 evening + 2026-04-22): - c_stack final 0.482 — stacking IS additive (+0.017 over adj=1). Yesterday's 'stacking didn't stack' read was from a 62% partial rescore. - adj_sweep_adj2 0.499 — new leader, +0.089 vs baseline-opt. - adj_sweep_adj3 0.487 — regresses from adj=2, sweet spot is 2. - adj1_k20 0.479 — k=20 adds +0.014 on adj=1. - adj1_llm partial 0.464 — llm-exp flat on adj=1. Clean stack decomposition: adj=1 alone = 0.465 adj=1 + k=20 = 0.479 (+0.014 from k=20) adj=1 + llm-exp = 0.464 (+0.00 from llm-exp) adj=1 + k=20 + llm = 0.482 (+0.003 from llm-exp on top of k=20) Next queued: adj2_k20 (predicted ~0.513), then qwen3.5:9b block, then Qwen3.6 MoE (HLWQ via vLLM + GGUF via Ollama). * docs(specs): 9B generator block — c_stack_plus_qwen9b new leader at 0.509 - Add qwen3.5:9b generator block section with three results and the stacking-at-9B insight: full stack gains +0.028 at 9B vs +0.017 at 5B. Bigger model can use the wider retrieval surface the 5B couldn't. - Tier crossover flagged: 0.509 matches the Letta/LangMem/OpenAI-memory band (0.50–0.52) on a local 12 GB GPU. Mem0 paper (0.66) and audited Zep (0.584) remain ahead cross-tier. - Retract the adj=2 + k=20 = 0.513 prediction. Actual measurement was 0.477. Context token budget saturates at adj=2 on 5B; adding k=20 floods it. - Update complete/in-flight/queued with today's timeline, adj1_llm final (0.458, not 0.464 partial), qwen9b numbers, qwen9b_k20_thinking_on queued as the post-POSTMATRIX control run. - Note PR #42 (think=false on generator, 20x speedup), PR #43 (revert think=false on judge after the 1452 silent-zero bug), PR #44 (--thinking-mode opt-in flag). * docs(specs): adj2_full_stack_qwen9b 0.545 — new leader, parity with audited Zep Today's key landings: - adj2_full_stack_qwen9b: 0.545 — overall leader, +0.029 over adj=2 alone at 9B, +0.046 over the previous adj=1+stack 9B leader. - adj2_qwen9b: 0.516 — adj=2 alone at 9B (logged earlier today). - c6_multihop_qwen9b: 0.306 — multihop regression worsened at 9B (was 0.317 at 5B). Footgun confirmed across all model sizes. - qwen35_9b_full_context: 0.090 — retrieval ablation. Full conversation in context collapses to slightly above mem0 floor. Empirical proof that retrieval is essential, not just a context-window workaround. Headline revision (3rd this week): stacking is adj-dependent AND model-size-dependent. 5B + adj=2 + stack regresses (-0.022); 9B + adj=2 + stack compounds (+0.029). Smaller model attention saturates earlier; bigger model can absorb wider retrieval surface even at adj=2. Tier crossover: 0.545 is within 0.04 of audited Zep (0.584) on gpt-4o-mini. Functional parity on a local 12 GB GPU + 9B quant. Mem0 paper (0.66) and Mem0^g (0.68) remain ahead — both reported by mem0's own harness, not independently audited.
1 participant
Why
The prior adapter had two broken ingest paths:
mempalace.layers.MemoryStack.add()— the method doesn't exist onMemoryStack; the class only hasrecall,search,status,wake_up. Attempt silently falls through.subprocess mempalace mine <palace>— passes the palace directory as the mine source rather than as--palacetarget. Wrong semantics; mine expects the text source as its positional arg.Both paths fail without any obvious error, producing empty retrievals and suppressed scores.
What changed
MemPalace's own
benchmarks/locomo_bench.pybypasses both theMemoryStackclass and themempalaceCLI — it usesimport chromadbdirectly, onePersistentClientcollection per conversation. That is the codepath that produced their published LoCoMo R@10 numbers (60.3% raw).This PR rewrites only
_ingest_conversation_mempalaceand_mempalace_searchto match that pattern:chromadb.PersistentClient(path=palace)per conversationlocomo_{conv_id}— fresh per run, idempotent on restartcoll.add(documents=..., ids=..., metadatas=...)for ingestcoll.query(query_texts=[question], n_results=top_k)for search_mempalace_searchgains aconv_idparameter; its single call site updatedGenerator, judge, result-dict shape,
_summary, CLI flags, and scorecard output are all unchanged.Test plan
python3 -c "import ast; ast.parse(open('benchmarks/mempalace_locomo_runner.py').read())"— syntax cleanpython3 benchmarks/mempalace_locomo_runner.py --help— CLI assembles, all flags presentSummary by CodeRabbit
Chores
Refactor