refactor(vector): address PR #775 review — drop unused payload key, fix resource leaks
- Drop chunk_bbox_page from Qdrant payload — viz endpoints never read it (page_number is the canonical PDF page field). - Bump upsert BATCH_SIZE 10 → 100 now that payloads no longer carry PNGs. - compute_chunk_bboxes_batch: move doc.close() into finally, replace unused stored_page_num with _. - purge_page_images.py: switch to anyio.run() per project convention, and wrap AsyncQdrantClient in try/finally so the aiohttp session is always closed (the class doesn't implement async-context-manager). - Decorate new bbox unit tests with @pytest.mark.unit so they run under the fast-feedback selector. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
ee402ea00e
commit
80b27b1bc6
@@ -726,6 +726,7 @@ class PDFHighlighter:
|
||||
return results
|
||||
|
||||
temp_pdf_path = None
|
||||
doc = None
|
||||
try:
|
||||
temp_dir = Path(tempfile.mkdtemp(prefix="pdf_bbox_batch_"))
|
||||
temp_pdf_path = temp_dir / "pdf.pdf"
|
||||
@@ -737,7 +738,7 @@ class PDFHighlighter:
|
||||
chunk_index,
|
||||
start_offset,
|
||||
end_offset,
|
||||
stored_page_num,
|
||||
_,
|
||||
chunk_text,
|
||||
) in chunks:
|
||||
chunk_page_info = PDFHighlighter.find_chunk_page(
|
||||
@@ -781,7 +782,6 @@ class PDFHighlighter:
|
||||
)
|
||||
results[chunk_index] = ([normalized], page_num)
|
||||
|
||||
doc.close()
|
||||
logger.info(f"Computed bboxes for {len(results)}/{len(chunks)} chunks")
|
||||
return results
|
||||
|
||||
@@ -790,6 +790,8 @@ class PDFHighlighter:
|
||||
return results
|
||||
|
||||
finally:
|
||||
if doc is not None:
|
||||
doc.close()
|
||||
if temp_pdf_path and temp_pdf_path.parent.exists():
|
||||
try:
|
||||
shutil.rmtree(temp_pdf_path.parent)
|
||||
|
||||
@@ -736,12 +736,10 @@ async def _index_document(
|
||||
),
|
||||
# Chunk bbox (PDF only) — normalized rectangles in [0,1]
|
||||
# relative to page width/height. Replaces the legacy
|
||||
# `highlighted_page_image` (Deck #76).
|
||||
# `highlighted_page_image` (Deck #76). The page number
|
||||
# comes from `page_number` (set above for PDF chunks).
|
||||
**(
|
||||
{
|
||||
"chunk_bbox": chunk_bboxes[i]["bbox"],
|
||||
"chunk_bbox_page": chunk_bboxes[i]["page"],
|
||||
}
|
||||
{"chunk_bbox": chunk_bboxes[i]["bbox"]}
|
||||
if i in chunk_bboxes
|
||||
else {}
|
||||
),
|
||||
@@ -763,9 +761,10 @@ async def _index_document(
|
||||
f"Failed to delete placeholder for {doc_task.doc_type}_{doc_task.doc_id}: {e}"
|
||||
)
|
||||
|
||||
# Upsert to Qdrant in batches to avoid timeout with large payloads
|
||||
# Batch size kept small for safety; payloads are now small (no inline images).
|
||||
BATCH_SIZE = 10
|
||||
# Upsert to Qdrant in batches. Now that we no longer embed PNG payloads,
|
||||
# per-point payloads are small (chunk text + small metadata), so we can
|
||||
# safely use a larger batch size.
|
||||
BATCH_SIZE = 100
|
||||
with trace_operation(
|
||||
"vector_sync.qdrant_upsert",
|
||||
attributes={
|
||||
|
||||
@@ -18,10 +18,11 @@ the same `Settings` object the server uses.
|
||||
from __future__ import annotations
|
||||
|
||||
import argparse
|
||||
import asyncio
|
||||
import logging
|
||||
import sys
|
||||
from functools import partial
|
||||
|
||||
import anyio
|
||||
from qdrant_client import AsyncQdrantClient
|
||||
|
||||
from nextcloud_mcp_server.config import get_settings
|
||||
@@ -35,74 +36,74 @@ LEGACY_FIELDS = [
|
||||
]
|
||||
|
||||
|
||||
def make_client() -> AsyncQdrantClient:
|
||||
async def purge(dry_run: bool, batch_size: int) -> None:
|
||||
settings = get_settings()
|
||||
if not settings.qdrant_url:
|
||||
raise SystemExit(
|
||||
"qdrant_url is not configured. Set QDRANT_URL (and QDRANT_API_KEY "
|
||||
"if required) before running this script."
|
||||
)
|
||||
return AsyncQdrantClient(
|
||||
collection = settings.get_collection_name()
|
||||
|
||||
# AsyncQdrantClient doesn't implement __aenter__/__aexit__, so use
|
||||
# try/finally to guarantee the underlying aiohttp session is closed.
|
||||
client = AsyncQdrantClient(
|
||||
url=settings.qdrant_url,
|
||||
api_key=settings.qdrant_api_key,
|
||||
timeout=60,
|
||||
)
|
||||
|
||||
|
||||
async def purge(dry_run: bool, batch_size: int) -> None:
|
||||
settings = get_settings()
|
||||
collection = settings.get_collection_name()
|
||||
client = make_client()
|
||||
|
||||
next_offset = None
|
||||
total_seen = 0
|
||||
total_updated = 0
|
||||
|
||||
logger.info(
|
||||
"Scanning collection %s; will delete keys %s%s",
|
||||
collection,
|
||||
LEGACY_FIELDS,
|
||||
" (dry run)" if dry_run else "",
|
||||
)
|
||||
|
||||
while True:
|
||||
points, next_offset = await client.scroll(
|
||||
collection_name=collection,
|
||||
limit=batch_size,
|
||||
offset=next_offset,
|
||||
with_payload=False,
|
||||
with_vectors=False,
|
||||
)
|
||||
if not points:
|
||||
break
|
||||
|
||||
ids = [p.id for p in points]
|
||||
total_seen += len(ids)
|
||||
|
||||
if not dry_run:
|
||||
await client.delete_payload(
|
||||
collection_name=collection,
|
||||
keys=LEGACY_FIELDS,
|
||||
points=ids,
|
||||
)
|
||||
total_updated += len(ids)
|
||||
try:
|
||||
next_offset = None
|
||||
total_seen = 0
|
||||
total_updated = 0
|
||||
|
||||
logger.info(
|
||||
"Batch: ids=%d total_seen=%d total_updated=%d",
|
||||
len(ids),
|
||||
total_seen,
|
||||
total_updated,
|
||||
"Scanning collection %s; will delete keys %s%s",
|
||||
collection,
|
||||
LEGACY_FIELDS,
|
||||
" (dry run)" if dry_run else "",
|
||||
)
|
||||
|
||||
if next_offset is None:
|
||||
break
|
||||
while True:
|
||||
points, next_offset = await client.scroll(
|
||||
collection_name=collection,
|
||||
limit=batch_size,
|
||||
offset=next_offset,
|
||||
with_payload=False,
|
||||
with_vectors=False,
|
||||
)
|
||||
if not points:
|
||||
break
|
||||
|
||||
logger.info(
|
||||
"Done. total_seen=%d total_updated=%d%s",
|
||||
total_seen,
|
||||
total_updated,
|
||||
" (dry run, no writes)" if dry_run else "",
|
||||
)
|
||||
ids = [p.id for p in points]
|
||||
total_seen += len(ids)
|
||||
|
||||
if not dry_run:
|
||||
await client.delete_payload(
|
||||
collection_name=collection,
|
||||
keys=LEGACY_FIELDS,
|
||||
points=ids,
|
||||
)
|
||||
total_updated += len(ids)
|
||||
|
||||
logger.info(
|
||||
"Batch: ids=%d total_seen=%d total_updated=%d",
|
||||
len(ids),
|
||||
total_seen,
|
||||
total_updated,
|
||||
)
|
||||
|
||||
if next_offset is None:
|
||||
break
|
||||
|
||||
logger.info(
|
||||
"Done. total_seen=%d total_updated=%d%s",
|
||||
total_seen,
|
||||
total_updated,
|
||||
" (dry run, no writes)" if dry_run else "",
|
||||
)
|
||||
finally:
|
||||
await client.close()
|
||||
|
||||
|
||||
def main() -> int:
|
||||
@@ -128,7 +129,7 @@ def main() -> int:
|
||||
format="%(asctime)s %(levelname)s %(name)s: %(message)s",
|
||||
)
|
||||
|
||||
asyncio.run(purge(dry_run=args.dry_run, batch_size=args.batch_size))
|
||||
anyio.run(partial(purge, dry_run=args.dry_run, batch_size=args.batch_size))
|
||||
return 0
|
||||
|
||||
|
||||
|
||||
@@ -37,6 +37,7 @@ def _page_boundaries(pages: list[str]) -> tuple[list[dict], str]:
|
||||
return boundaries, "".join(parts)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_compute_chunk_bboxes_returns_normalized_rects():
|
||||
"""Each returned bbox should be 4 floats in [0, 1] tagged with the page."""
|
||||
pages = [
|
||||
@@ -89,6 +90,7 @@ def test_compute_chunk_bboxes_returns_normalized_rects():
|
||||
assert 0.0 <= y0 < y1 <= 1.0
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
def test_compute_chunk_bboxes_empty_input():
|
||||
assert (
|
||||
PDFHighlighter.compute_chunk_bboxes_batch(
|
||||
@@ -101,6 +103,7 @@ def test_compute_chunk_bboxes_empty_input():
|
||||
)
|
||||
|
||||
|
||||
@pytest.mark.unit
|
||||
@pytest.mark.parametrize("page_index", [0, 1])
|
||||
def test_compute_chunk_bboxes_assigns_correct_page(page_index: int):
|
||||
"""Verify the page number returned matches the page the chunk lives on."""
|
||||
|
||||
Reference in New Issue
Block a user