-
Notifications
You must be signed in to change notification settings - Fork 7.6k
perf: L1 importance pre-filter — skip full scan when enough high-importance drawers exist #660
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -71,12 +71,19 @@ def test_layer0_default_path(): | |||||||
|
|
||||||||
|
|
||||||||
| def _mock_chromadb_for_layer(docs, metas, monkeypatch=None): | ||||||||
| """Return a mock collection whose get() returns docs/metas.""" | ||||||||
| """Return a mock collection whose get() returns docs/metas. | ||||||||
|
|
||||||||
| Layer1._fetch_drawers() has two phases: a fast-path (importance >= 3 | ||||||||
| pre-filter) and a fallback full-scan. For small test datasets (< 500 | ||||||||
| items), each phase makes exactly one col.get() call before breaking | ||||||||
| (len < _BATCH). We provide two identical responses: the first is consumed | ||||||||
| by the fast path, and the second is only consumed when the fast path | ||||||||
| doesn't return enough results (< MAX_DRAWERS) and the fallback executes. | ||||||||
| """ | ||||||||
| mock_col = MagicMock() | ||||||||
| # First batch returns data, second batch returns empty (end of pagination) | ||||||||
| mock_col.get.side_effect = [ | ||||||||
| {"documents": docs, "metadatas": metas}, | ||||||||
| {"documents": [], "metadatas": []}, | ||||||||
| {"documents": docs, "metadatas": metas}, # fast-path batch | ||||||||
| {"documents": docs, "metadatas": metas}, # fallback batch (< MAX_DRAWERS → fallback) | ||||||||
| ] | ||||||||
|
Comment on lines
+74
to
87
|
||||||||
| return mock_col | ||||||||
|
|
||||||||
|
|
@@ -141,9 +148,13 @@ def test_layer1_with_wing_filter(): | |||||||
| result = layer.generate() | ||||||||
|
|
||||||||
| assert "ESSENTIAL STORY" in result | ||||||||
| # Verify wing filter was passed | ||||||||
| # Verify wing filter was passed in the first (fast-path) call. | ||||||||
| # The fast-path combines wing with an importance pre-filter via $and. | ||||||||
| call_kwargs = mock_col.get.call_args_list[0][1] | ||||||||
| assert call_kwargs.get("where") == {"wing": "project_x"} | ||||||||
| where = call_kwargs.get("where", {}) | ||||||||
| assert "$and" in where | ||||||||
| assert {"wing": "project_x"} in where["$and"] | ||||||||
|
||||||||
| assert {"wing": "project_x"} in where["$and"] | |
| assert {"wing": "project_x"} in where["$and"] | |
| assert {"importance": {"$gte": 3}} in where["$and"] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed — added assert {"importance": {"$gte": 3}} in where["$and"] to verify the importance pre-filter is present.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capping the fast-path results (
len(docs) >= MAX_SCAN) and then returning them whenlen(docs) >= MAX_DRAWERSmeansgenerate()is no longer guaranteed to select the true top-15 drawers by importance across the whole palace:col.get()order isn’t tied to importance, and the highest-importance drawers may exist beyond the firstMAX_SCANmatches. This also contradicts the PR description’s claim that output is unchanged when enough high-importance drawers exist. If exactness matters, avoid the cap on the candidate set (or use a deterministic multi-threshold strategy like querying for importance>=5, then >=4, etc. until 15 are collected).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documented — added multi-line comment explaining the MAX_SCAN tradeoff: prevents O(n) scans on 100K+ palaces, accepts approximate top-15. L3 deep search covers full corpus when precision matters.