feat: retrieval-time TTL filter + three-number bench summary - #155
Merged
Conversation
Feature 1: forget_after / forget_reason in VectorMemory metadata. _load_active_rows gains an optional `now` param and excludes rows whose metadata forget_after has passed. Zero-loss: raw row stays in the DB, only recall filters it. Non-numeric values are ignored with a debug log. HTTP endpoint docstrings updated to document the two new metadata fields. 8 tests in tests/test_ttl_filter.py cover: expired invisible, future visible, non-numeric ignored, zero-loss, batch round-trip, clock override. Feature 2: mean_latency_ms / p95_latency_ms / mean_context_tokens added to _summary, the overall JSON block, and _print_summary in locomo_runner. _process_qa stores context_chars (len of context string) per row so the token estimate (chars / 4) is exact. Accuracy columns untouched. 11 tests in tests/test_locomo_memscore.py cover aggregate math and print. CHANGELOG updated under Unreleased.
📝 WalkthroughWalkthroughThis PR introduces two independent features: a retrieval-time TTL filter for vector-memory rows controlled by ChangesRetrieval-time TTL filter
LoCoMo benchmark metrics enhancement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 2
🧹 Nitpick comments (2)
benchmarks/locomo_runner.py (1)
1108-1124: 💤 Low valueComputation logic is correct, but minor cleanup opportunity.
The latency and context-token calculations are sound. However, line 1118 has a redundant
int()call:math.floor()already returns anintin Python 3.♻️ Optional simplification
- p95_idx = min(int(math.floor(0.95 * n)), n - 1) + p95_idx = min(math.floor(0.95 * n), n - 1)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@benchmarks/locomo_runner.py` around lines 1108 - 1124, The p95 index computation uses an unnecessary int() around math.floor in the expression computing p95_idx; update the p95_idx calculation (the line computing p95_idx using math.floor) to remove the redundant int() so it simply uses math.floor(...) (or better, compute p95_idx = min(math.floor(0.95 * n), n - 1)) while keeping the use of latencies, p95_lat, and mean_lat unchanged.Source: Linters/SAST tools
taosmd/http_server.py (1)
54-57: ⚡ Quick winClarify the request structure for single ingest.
The documentation mentions "user metadata dict" but doesn't show where this goes in the request body. Consider making it explicit like the batch endpoint does, e.g.,
{"text", "agent", "project"?, "metadata"?: {"forget_after"?: float, "forget_reason"?: str}}.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@taosmd/http_server.py` around lines 54 - 57, The documentation for the single-ingest request is ambiguous about where the "user metadata dict" belongs; update the single-ingest endpoint docs in taosmd/http_server.py to explicitly show the request JSON schema (mirroring the batch endpoint) — i.e. the body should be {"text": str, "agent": str, "project"?: str, "metadata"?: {"forget_after"?: float, "forget_reason"?: str}} — and ensure the single-ingest handler (the function that parses the single-item POST) expects and validates a top-level "metadata" object with optional forget_after and forget_reason fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/locomo_runner.py`:
- Around line 1198-1205: The printed label "p50" is incorrect because the code
is using overall.get('mean_latency_ms', ...) which is the arithmetic mean;
update the print f-string in benchmarks/locomo_runner.py (the block that prints
latency using overall.get('mean_latency_ms', ...) and
overall.get('p95_latency_ms', ...)) to relabel "p50=" as "mean=" (or
"mean_latency=") so it accurately reflects the metric, leaving the p95 line
unchanged.
In `@tests/test_locomo_memscore.py`:
- Around line 176-184: The test test_print_summary_includes_latency_line should
also assert that the printed latency line uses the correct label for the mean
latency instead of a p50 label; update the test to call
runner._print_summary(meta, {}, overall) (as it does) and assert that the output
contains the expected label for mean latency (e.g., "mean" or "mean latency")
and that the p95 value is labeled appropriately (e.g., "p95"), referencing the
test name test_print_summary_includes_latency_line and the runner._print_summary
invocation and the overall fields mean_latency_ms and p95_latency_ms so the test
fails if labels are swapped or incorrect.
---
Nitpick comments:
In `@benchmarks/locomo_runner.py`:
- Around line 1108-1124: The p95 index computation uses an unnecessary int()
around math.floor in the expression computing p95_idx; update the p95_idx
calculation (the line computing p95_idx using math.floor) to remove the
redundant int() so it simply uses math.floor(...) (or better, compute p95_idx =
min(math.floor(0.95 * n), n - 1)) while keeping the use of latencies, p95_lat,
and mean_lat unchanged.
In `@taosmd/http_server.py`:
- Around line 54-57: The documentation for the single-ingest request is
ambiguous about where the "user metadata dict" belongs; update the single-ingest
endpoint docs in taosmd/http_server.py to explicitly show the request JSON
schema (mirroring the batch endpoint) — i.e. the body should be {"text": str,
"agent": str, "project"?: str, "metadata"?: {"forget_after"?: float,
"forget_reason"?: str}} — and ensure the single-ingest handler (the function
that parses the single-item POST) expects and validates a top-level "metadata"
object with optional forget_after and forget_reason fields.
🪄 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: 1928af9d-97a0-4fbf-8c6c-769167078a3d
📒 Files selected for processing (6)
CHANGELOG.mdbenchmarks/locomo_runner.pytaosmd/http_server.pytaosmd/vector_memory.pytests/test_locomo_memscore.pytests/test_ttl_filter.py
Comment on lines
+1198
to
+1205
| # Three-number performance summary (accuracy / latency / context size). | ||
| # Never collapsed to a single score \u2014 each dimension is independent signal. | ||
| print( | ||
| f"Latency p50={overall.get('mean_latency_ms', 0.0):.0f} ms " | ||
| f"p95={overall.get('p95_latency_ms', 0.0):.0f} ms | " | ||
| f"Context ~{overall.get('mean_context_tokens', 0.0):.0f} tok/query (mean)" | ||
| ) | ||
| print(sep) |
There was a problem hiding this comment.
Critical: "p50" label is incorrect — you're printing the mean, not the median.
Line 1201 labels mean_latency_ms as "p50", but p50 is the median (50th percentile), not the arithmetic mean. _summary computes mean_lat = sum(latencies) / n, which is the mean. Either compute the actual p50 and print it, or relabel the output as "mean".
🔧 Recommended fix: relabel as "mean"
print(
- f"Latency p50={overall.get('mean_latency_ms', 0.0):.0f} ms "
+ f"Latency mean={overall.get('mean_latency_ms', 0.0):.0f} ms "
f"p95={overall.get('p95_latency_ms', 0.0):.0f} ms | "
f"Context ~{overall.get('mean_context_tokens', 0.0):.0f} tok/query (mean)"
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@benchmarks/locomo_runner.py` around lines 1198 - 1205, The printed label
"p50" is incorrect because the code is using overall.get('mean_latency_ms', ...)
which is the arithmetic mean; update the print f-string in
benchmarks/locomo_runner.py (the block that prints latency using
overall.get('mean_latency_ms', ...) and overall.get('p95_latency_ms', ...)) to
relabel "p50=" as "mean=" (or "mean_latency=") so it accurately reflects the
metric, leaving the p95 line unchanged.
Comment on lines
+176
to
+184
| def test_print_summary_includes_latency_line(): | ||
| runner = _load_runner() | ||
| overall = _make_overall() | ||
| meta = _make_meta() | ||
| out = _capture(lambda: runner._print_summary(meta, {}, overall)) | ||
| assert "Latency" in out or "latency" in out.lower() | ||
| assert "p95" in out or "P95" in out | ||
| assert "1200" in out or "1200.0" in out # mean latency | ||
| assert "2500" in out or "2500.0" in out # p95 |
There was a problem hiding this comment.
Test gap: does not verify label correctness.
test_print_summary_includes_latency_line checks that "1200" appears (the value of mean_latency_ms) but does not verify whether the label says "mean" or "p50". Since _print_summary currently mislabels mean as "p50", this test passes despite the bug. Consider asserting that the label correctly identifies the metric type.
🧪 Suggested assertion to catch label bugs
def test_print_summary_includes_latency_line():
runner = _load_runner()
overall = _make_overall()
meta = _make_meta()
out = _capture(lambda: runner._print_summary(meta, {}, overall))
assert "Latency" in out or "latency" in out.lower()
assert "p95" in out or "P95" in out
assert "1200" in out or "1200.0" in out # mean latency
assert "2500" in out or "2500.0" in out # p95
+ # Verify the label for 1200 is "mean", not "p50" (since overall["mean_latency_ms"]=1200)
+ assert "mean=" in out.lower() or "mean:" in out.lower()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_locomo_memscore.py` around lines 176 - 184, The test
test_print_summary_includes_latency_line should also assert that the printed
latency line uses the correct label for the mean latency instead of a p50 label;
update the test to call runner._print_summary(meta, {}, overall) (as it does)
and assert that the output contains the expected label for mean latency (e.g.,
"mean" or "mean latency") and that the p95 value is labeled appropriately (e.g.,
"p95"), referencing the test name test_print_summary_includes_latency_line and
the runner._print_summary invocation and the overall fields mean_latency_ms and
p95_latency_ms so the test fails if labels are swapped or incorrect.
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
1 participant
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
Feature 1: retrieval-time TTL filter.
VectorMemory.addnow acceptsforget_after(unix float) andforget_reason(str) in user metadata._load_active_rowsexcludes rows whoseforget_afterhas passed, exactly like superseded rows. Zero-loss: the raw row is never deleted. Non-numeric values are silently ignored.POST /ingestandPOST /ingest/batchdocstrings document the new fields. Inspired by supermemory'sforgetAfterconcept.Feature 2: three-number bench summary.
_summary, theoverallJSON block, and_print_summaryinlocomo_runner.pynow emitmean_latency_ms,p95_latency_ms, andmean_context_tokens(context chars / 4 per row)._process_qastorescontext_charsper row. Existing accuracy columns (F1, BLEU-1, Judge, R@K) are untouched. Inspired by supermemory's MemScore philosophy of not collapsing three independent dimensions.Files changed
taosmd/vector_memory.py--_load_active_rowsgains TTL filter + optionalnowparamtaosmd/http_server.py-- endpoint docstring updated withforget_after/forget_reasonnotebenchmarks/locomo_runner.py--_summary,_print_summary,_process_qaupdatedCHANGELOG.md-- two new Unreleased entriestests/test_ttl_filter.py-- 8 tests for the TTL filtertests/test_locomo_memscore.py-- 11 tests for the bench summaryTest plan
python3 -m pytest tests/test_ttl_filter.py tests/test_locomo_memscore.py -v-- 19 passedpython3 -m pytest --timeout=60 -q-- 778 passed, 0 failuresSummary by CodeRabbit
New Features
Documentation
Tests