Skip to content

fix(hallways): paginate compute_hallways_for_wing to survive large wings - #1620

Open
davidglidden wants to merge 1 commit into
MemPalace:mainfrom
davidglidden:fix/hallways-paginate-large-wings
Open

fix(hallways): paginate compute_hallways_for_wing to survive large wings#1620
davidglidden wants to merge 1 commit into
MemPalace:mainfrom
davidglidden:fix/hallways-paginate-large-wings

Conversation

@davidglidden

Copy link
Copy Markdown
Contributor

What

Paginate compute_hallways_for_wing's drawer fetch so it survives wings larger than SQLite's SQLITE_MAX_VARIABLE_NUMBER (32766).

Fixes #1619.

Why

The within-wing hallways post-mine step (#1558) fetches the whole wing in a single col.get(where={"wing": wing}). ChromaDB binds one SQL variable per matching id, so once a wing exceeds ~32k drawers the call raises InternalError: ... too many SQL variables. The exception is caught (the mine completes) but the wing's hallway graph silently never builds — on exactly the large wings that benefit most; cross-wing tunnel promotion (#1565) is starved for them too. This is the same hazard already fixed by paginating in miner.py (#851) and regenerate_closets (#1073/#1107); the new #1558 path just didn't inherit the pattern.

How

Walk the collection in batch_size=5000 pages via count() + get(limit=, offset=) and filter to the wing client-side — mirroring the existing idiom in miner.py's status counter. Sibling features are unaffected (dynamics.py doesn't fetch drawers; palace_graph.py tunnels already paginate via limit/offset).

Validation

  • Against a real 42,062-drawer wing: previously raised "too many SQL variables"; now completes in ~3.7s computing 5,525 hallways.
  • tests/test_hallways.py: mock updated for the count() + paginated-get path; all 21 tests pass, ruff clean.

Scope

One function (compute_hallways_for_wing) + its test. No behavior change for wings under the limit.

compute_hallways_for_wing fetched the whole wing in a single
col.get(where={"wing": wing}), which binds one SQL variable per matching
id and trips SQLite's SQLITE_MAX_VARIABLE_NUMBER (32766) inside chromadb
once a wing exceeds ~32k drawers. The post-mine hallway step then crashes
on exactly the large wings that most benefit from hallways; the error is
caught so the mine completes, but the wing's hallway graph silently never
builds.

Mirror the paginated fetch already used for the same hazard in miner.py
(MemPalace#851) and closet_llm.py (MemPalace#1073): walk the collection in batches of 5000
via count() + get(limit=, offset=) and filter to the wing client-side.

Validated against a real 42,062-drawer wing: previously raised
"too many SQL variables"; now completes in ~3.7s computing 5,525
hallways. Test mock updated for the count()+paginated-get path.

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces paginated fetching in compute_hallways_for_wing to prevent hitting SQLite's SQLITE_MAX_VARIABLE_NUMBER limit in ChromaDB when querying large wings. However, the current implementation fetches the entire collection and filters by wing client-side, which is highly inefficient. The reviewer suggests querying with the where filter directly in the paginated col.get() calls, which safely bounds the parameters while avoiding loading unrelated wings. Additionally, terminating the loop based on the batch size allows removing the col.count() call entirely, simplifying both the production code and the test mocks.

Comment thread mempalace/hallways.py
Comment on lines +208 to +219
metadatas: list = []
try:
results = col.get(where={"wing": wing}, include=["metadatas"])
total = col.count()
batch_size = 5000
offset = 0
while offset < total:
batch = col.get(limit=batch_size, offset=offset, include=["metadatas"])
batch_metas = (batch or {}).get("metadatas") or []
if not batch_metas:
break
metadatas.extend(m for m in batch_metas if (m or {}).get("wing") == wing)
offset += len(batch_metas)

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

Efficiency & Performance Issue

Currently, the code fetches the entire collection's metadatas in batches of 5000 and filters them by wing client-side in Python. If the database contains multiple wings with a large number of drawers, this approach is highly inefficient as it loads and processes metadatas for all other wings unnecessarily.

Instead, we can pass where={"wing": wing} directly to col.get(...) along with limit and offset. Because the number of items returned per page is bounded by limit=5000, the internal IN clause query in ChromaDB will only bind at most 5000 parameters, which is well below SQLite's SQLITE_MAX_VARIABLE_NUMBER (32766) limit and completely avoids the crash.

Furthermore, we can terminate the loop when len(batch_metas) < batch_size, which eliminates the need to call col.count() entirely and prevents infinite loops in both production and unit tests (where the mock returns the same dataset repeatedly).

Suggested change
metadatas: list = []
try:
results = col.get(where={"wing": wing}, include=["metadatas"])
total = col.count()
batch_size = 5000
offset = 0
while offset < total:
batch = col.get(limit=batch_size, offset=offset, include=["metadatas"])
batch_metas = (batch or {}).get("metadatas") or []
if not batch_metas:
break
metadatas.extend(m for m in batch_metas if (m or {}).get("wing") == wing)
offset += len(batch_metas)
metadatas: list = []
try:
batch_size = 5000
offset = 0
while True:
batch = col.get(
where={"wing": wing},
limit=batch_size,
offset=offset,
include=["metadatas"],
)
batch_metas = (batch or {}).get("metadatas") or []
metadatas.extend(m for m in batch_metas if m)
if len(batch_metas) < batch_size:
break
offset += len(batch_metas)

Comment thread tests/test_hallways.py
Comment on lines +30 to 41
"""Build a MagicMock collection whose paginated .get() returns the drawer set.

compute_hallways_for_wing now paginates via ``count()`` + ``get(limit=,
offset=)`` (to dodge SQLITE_MAX_VARIABLE_NUMBER on large wings), so the
mock exposes ``count()`` and returns the full set on the single batch.
"""
col = MagicMock()
metadatas = [d for d in drawers]
ids = [f"drawer_{i}" for i in range(len(drawers))]
col.count.return_value = len(drawers)
col.get.return_value = {"ids": ids, "metadatas": metadatas}
return col

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

Documentation & Code Cleanup

Since we no longer use col.count() in the paginated fetch, we can simplify the mock collection and update its docstring to accurately reflect the new where + limit + offset pagination approach.

Suggested change
"""Build a MagicMock collection whose paginated .get() returns the drawer set.
compute_hallways_for_wing now paginates via ``count()`` + ``get(limit=,
offset=)`` (to dodge SQLITE_MAX_VARIABLE_NUMBER on large wings), so the
mock exposes ``count()`` and returns the full set on the single batch.
"""
col = MagicMock()
metadatas = [d for d in drawers]
ids = [f"drawer_{i}" for i in range(len(drawers))]
col.count.return_value = len(drawers)
col.get.return_value = {"ids": ids, "metadatas": metadatas}
return col
"""Build a MagicMock collection whose paginated .get() returns the drawer set.
compute_hallways_for_wing now paginates via ``get(where=, limit=, offset=)``
(to dodge SQLITE_MAX_VARIABLE_NUMBER on large wings), so the mock returns
the full set on the single batch.
"""
col = MagicMock()
metadatas = [d for d in drawers]
ids = [f"drawer_{i}" for i in range(len(drawers))]
col.get.return_value = {"ids": ids, "metadatas": metadatas}
return col

@igorls

igorls commented Aug 15, 2026

Copy link
Copy Markdown
Member

Thanks for this contribution, and apologies for the slow turnaround.

develop has moved a fair way since this was opened and the branch no longer merges cleanly. If you're still interested in landing it, could you rebase onto current develop? Once it merges cleanly and CI is green I'll get it reviewed for the 3.8.0 cycle.

If you'd rather not pick it back up, no problem at all — just say so and I'll close it out, and thanks either way for taking the time to send it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants