refactor: centralize magic numbers for page dimensions, limits, and timeout (#168)
- Move DEFAULT_PAGE_WIDTH/HEIGHT to domain/value_objects.py and import in both converters - Add opensearch_default_limit to Settings (configurable via OPENSEARCH_DEFAULT_LIMIT env var) - Pass settings.conversion_timeout to ServeConverter, removing independent _DEFAULT_TIMEOUT - Update OpenSearchStore to accept default_limit from Settings via constructor
This commit is contained in:
parent
e7c27a6706
commit
f2436290c5
7 changed files with 43 additions and 18 deletions
|
|
@ -8,6 +8,10 @@ from __future__ import annotations
|
||||||
|
|
||||||
from dataclasses import dataclass, field
|
from dataclasses import dataclass, field
|
||||||
|
|
||||||
|
# US Letter page dimensions (points) — fallback when page size is unknown
|
||||||
|
DEFAULT_PAGE_WIDTH: float = 612.0
|
||||||
|
DEFAULT_PAGE_HEIGHT: float = 792.0
|
||||||
|
|
||||||
|
|
||||||
@dataclass(frozen=True)
|
@dataclass(frozen=True)
|
||||||
class PageElement:
|
class PageElement:
|
||||||
|
|
|
||||||
|
|
@ -36,6 +36,8 @@ from docling_core.types.doc import (
|
||||||
)
|
)
|
||||||
|
|
||||||
from domain.value_objects import (
|
from domain.value_objects import (
|
||||||
|
DEFAULT_PAGE_HEIGHT,
|
||||||
|
DEFAULT_PAGE_WIDTH,
|
||||||
ConversionOptions,
|
ConversionOptions,
|
||||||
ConversionResult,
|
ConversionResult,
|
||||||
PageDetail,
|
PageDetail,
|
||||||
|
|
@ -50,10 +52,6 @@ logger = logging.getLogger(__name__)
|
||||||
# Uses a timeout to prevent a frozen conversion from blocking all others.
|
# Uses a timeout to prevent a frozen conversion from blocking all others.
|
||||||
_converter_lock = threading.Lock()
|
_converter_lock = threading.Lock()
|
||||||
|
|
||||||
# US Letter page dimensions (points) — fallback when page size is unknown
|
|
||||||
_DEFAULT_PAGE_WIDTH = 612.0
|
|
||||||
_DEFAULT_PAGE_HEIGHT = 792.0
|
|
||||||
|
|
||||||
# Default converter (lazy-init on first request)
|
# Default converter (lazy-init on first request)
|
||||||
_default_converter: DoclingConverter | None = None
|
_default_converter: DoclingConverter | None = None
|
||||||
|
|
||||||
|
|
@ -175,11 +173,11 @@ def _process_content_item(
|
||||||
logger.warning(
|
logger.warning(
|
||||||
"Page %d not found in document metadata — using US Letter fallback (%sx%s pt)",
|
"Page %d not found in document metadata — using US Letter fallback (%sx%s pt)",
|
||||||
page_no,
|
page_no,
|
||||||
_DEFAULT_PAGE_WIDTH,
|
DEFAULT_PAGE_WIDTH,
|
||||||
_DEFAULT_PAGE_HEIGHT,
|
DEFAULT_PAGE_HEIGHT,
|
||||||
)
|
)
|
||||||
pages[page_no] = PageDetail(
|
pages[page_no] = PageDetail(
|
||||||
page_number=page_no, width=_DEFAULT_PAGE_WIDTH, height=_DEFAULT_PAGE_HEIGHT
|
page_number=page_no, width=DEFAULT_PAGE_WIDTH, height=DEFAULT_PAGE_HEIGHT
|
||||||
)
|
)
|
||||||
|
|
||||||
page_height = pages[page_no].height
|
page_height = pages[page_no].height
|
||||||
|
|
@ -248,10 +246,10 @@ def _convert_sync(
|
||||||
pages_detail = [
|
pages_detail = [
|
||||||
PageDetail(
|
PageDetail(
|
||||||
page_number=i + 1,
|
page_number=i + 1,
|
||||||
width=doc.pages[i + 1].size.width if (i + 1) in doc.pages else _DEFAULT_PAGE_WIDTH,
|
width=doc.pages[i + 1].size.width if (i + 1) in doc.pages else DEFAULT_PAGE_WIDTH,
|
||||||
height=doc.pages[i + 1].size.height
|
height=doc.pages[i + 1].size.height
|
||||||
if (i + 1) in doc.pages
|
if (i + 1) in doc.pages
|
||||||
else _DEFAULT_PAGE_HEIGHT,
|
else DEFAULT_PAGE_HEIGHT,
|
||||||
)
|
)
|
||||||
for i in range(page_count)
|
for i in range(page_count)
|
||||||
]
|
]
|
||||||
|
|
|
||||||
|
|
@ -69,13 +69,14 @@ class OpenSearchStore:
|
||||||
verify_certs: Whether to verify TLS certificates.
|
verify_certs: Whether to verify TLS certificates.
|
||||||
"""
|
"""
|
||||||
|
|
||||||
def __init__(self, url: str, *, verify_certs: bool = False) -> None:
|
def __init__(self, url: str, *, verify_certs: bool = False, default_limit: int = 1000) -> None:
|
||||||
self._client = AsyncOpenSearch(
|
self._client = AsyncOpenSearch(
|
||||||
hosts=[url],
|
hosts=[url],
|
||||||
use_ssl=url.startswith("https"),
|
use_ssl=url.startswith("https"),
|
||||||
verify_certs=verify_certs,
|
verify_certs=verify_certs,
|
||||||
ssl_show_warn=False,
|
ssl_show_warn=False,
|
||||||
)
|
)
|
||||||
|
self._default_limit = default_limit
|
||||||
|
|
||||||
# -- lifecycle -------------------------------------------------------------
|
# -- lifecycle -------------------------------------------------------------
|
||||||
|
|
||||||
|
|
@ -147,9 +148,11 @@ class OpenSearchStore:
|
||||||
index_name: str,
|
index_name: str,
|
||||||
doc_id: str,
|
doc_id: str,
|
||||||
*,
|
*,
|
||||||
limit: int = 1000,
|
limit: int | None = None,
|
||||||
) -> list[SearchResult]:
|
) -> list[SearchResult]:
|
||||||
"""Retrieve all indexed chunks for a document, ordered by chunk_index."""
|
"""Retrieve all indexed chunks for a document, ordered by chunk_index."""
|
||||||
|
if limit is None:
|
||||||
|
limit = self._default_limit
|
||||||
resp = await self._client.search(
|
resp = await self._client.search(
|
||||||
index=index_name,
|
index=index_name,
|
||||||
body={
|
body={
|
||||||
|
|
|
||||||
|
|
@ -21,6 +21,8 @@ import httpx
|
||||||
from docling_core.types.doc.base import BoundingBox, CoordOrigin
|
from docling_core.types.doc.base import BoundingBox, CoordOrigin
|
||||||
|
|
||||||
from domain.value_objects import (
|
from domain.value_objects import (
|
||||||
|
DEFAULT_PAGE_HEIGHT,
|
||||||
|
DEFAULT_PAGE_WIDTH,
|
||||||
ConversionOptions,
|
ConversionOptions,
|
||||||
ConversionResult,
|
ConversionResult,
|
||||||
PageDetail,
|
PageDetail,
|
||||||
|
|
@ -31,7 +33,6 @@ from infra.bbox import to_topleft_list
|
||||||
logger = logging.getLogger(__name__)
|
logger = logging.getLogger(__name__)
|
||||||
|
|
||||||
_API_PREFIX = "/v1"
|
_API_PREFIX = "/v1"
|
||||||
_DEFAULT_TIMEOUT = 600.0
|
|
||||||
|
|
||||||
# Docling Serve label → our element type
|
# Docling Serve label → our element type
|
||||||
_LABEL_MAP = {
|
_LABEL_MAP = {
|
||||||
|
|
@ -60,7 +61,7 @@ class ServeConverter:
|
||||||
self,
|
self,
|
||||||
base_url: str,
|
base_url: str,
|
||||||
api_key: str | None = None,
|
api_key: str | None = None,
|
||||||
timeout: float = _DEFAULT_TIMEOUT,
|
timeout: float = 600.0,
|
||||||
):
|
):
|
||||||
self._base_url = base_url.rstrip("/")
|
self._base_url = base_url.rstrip("/")
|
||||||
self._api_key = api_key
|
self._api_key = api_key
|
||||||
|
|
@ -192,8 +193,8 @@ def _extract_pages_from_docling_document(doc: dict) -> list[PageDetail]:
|
||||||
size = page_data.get("size", {})
|
size = page_data.get("size", {})
|
||||||
pages_dict[page_no] = PageDetail(
|
pages_dict[page_no] = PageDetail(
|
||||||
page_number=page_no,
|
page_number=page_no,
|
||||||
width=size.get("width", 612.0),
|
width=size.get("width", DEFAULT_PAGE_WIDTH),
|
||||||
height=size.get("height", 792.0),
|
height=size.get("height", DEFAULT_PAGE_HEIGHT),
|
||||||
)
|
)
|
||||||
|
|
||||||
# Process all element arrays
|
# Process all element arrays
|
||||||
|
|
@ -220,8 +221,8 @@ def _add_element(item: dict, pages: dict[int, PageDetail]) -> None:
|
||||||
if page_no not in pages:
|
if page_no not in pages:
|
||||||
pages[page_no] = PageDetail(
|
pages[page_no] = PageDetail(
|
||||||
page_number=page_no,
|
page_number=page_no,
|
||||||
width=612.0,
|
width=DEFAULT_PAGE_WIDTH,
|
||||||
height=792.0,
|
height=DEFAULT_PAGE_HEIGHT,
|
||||||
)
|
)
|
||||||
|
|
||||||
bbox_data = prov.get("bbox", {})
|
bbox_data = prov.get("bbox", {})
|
||||||
|
|
|
||||||
|
|
@ -25,6 +25,7 @@ class Settings:
|
||||||
batch_page_size: int = 0 # 0 = disabled, > 0 = pages per batch
|
batch_page_size: int = 0 # 0 = disabled, > 0 = pages per batch
|
||||||
opensearch_url: str = "" # empty = disabled
|
opensearch_url: str = "" # empty = disabled
|
||||||
embedding_url: str = "" # empty = disabled (e.g. http://localhost:8001)
|
embedding_url: str = "" # empty = disabled (e.g. http://localhost:8001)
|
||||||
|
opensearch_default_limit: int = 1000 # max chunks returned by get_chunks
|
||||||
embedding_dimension: int = 384 # Granite Embedding 30M / all-MiniLM-L6-v2
|
embedding_dimension: int = 384 # Granite Embedding 30M / all-MiniLM-L6-v2
|
||||||
upload_dir: str = "./uploads"
|
upload_dir: str = "./uploads"
|
||||||
db_path: str = "./data/docling_studio.db"
|
db_path: str = "./data/docling_studio.db"
|
||||||
|
|
@ -54,6 +55,10 @@ class Settings:
|
||||||
errors.append(f"rate_limit_rpm must be >= 0 (got {self.rate_limit_rpm})")
|
errors.append(f"rate_limit_rpm must be >= 0 (got {self.rate_limit_rpm})")
|
||||||
if self.batch_page_size < 0:
|
if self.batch_page_size < 0:
|
||||||
errors.append(f"batch_page_size must be >= 0 (got {self.batch_page_size})")
|
errors.append(f"batch_page_size must be >= 0 (got {self.batch_page_size})")
|
||||||
|
if self.opensearch_default_limit < 1:
|
||||||
|
errors.append(
|
||||||
|
f"opensearch_default_limit must be >= 1 (got {self.opensearch_default_limit})"
|
||||||
|
)
|
||||||
if self.embedding_dimension < 1:
|
if self.embedding_dimension < 1:
|
||||||
errors.append(f"embedding_dimension must be >= 1 (got {self.embedding_dimension})")
|
errors.append(f"embedding_dimension must be >= 1 (got {self.embedding_dimension})")
|
||||||
if self.default_table_mode not in ("accurate", "fast"):
|
if self.default_table_mode not in ("accurate", "fast"):
|
||||||
|
|
@ -97,6 +102,7 @@ class Settings:
|
||||||
batch_page_size=int(os.environ.get("BATCH_PAGE_SIZE", "10")),
|
batch_page_size=int(os.environ.get("BATCH_PAGE_SIZE", "10")),
|
||||||
opensearch_url=os.environ.get("OPENSEARCH_URL", ""),
|
opensearch_url=os.environ.get("OPENSEARCH_URL", ""),
|
||||||
embedding_url=os.environ.get("EMBEDDING_URL", ""),
|
embedding_url=os.environ.get("EMBEDDING_URL", ""),
|
||||||
|
opensearch_default_limit=int(os.environ.get("OPENSEARCH_DEFAULT_LIMIT", "1000")),
|
||||||
embedding_dimension=int(os.environ.get("EMBEDDING_DIMENSION", "384")),
|
embedding_dimension=int(os.environ.get("EMBEDDING_DIMENSION", "384")),
|
||||||
upload_dir=os.environ.get("UPLOAD_DIR", "./uploads"),
|
upload_dir=os.environ.get("UPLOAD_DIR", "./uploads"),
|
||||||
db_path=os.environ.get("DB_PATH", "./data/docling_studio.db"),
|
db_path=os.environ.get("DB_PATH", "./data/docling_studio.db"),
|
||||||
|
|
|
||||||
|
|
@ -47,6 +47,7 @@ def _build_converter():
|
||||||
return ServeConverter(
|
return ServeConverter(
|
||||||
base_url=settings.docling_serve_url,
|
base_url=settings.docling_serve_url,
|
||||||
api_key=settings.docling_serve_api_key,
|
api_key=settings.docling_serve_api_key,
|
||||||
|
timeout=settings.conversion_timeout,
|
||||||
)
|
)
|
||||||
else:
|
else:
|
||||||
from infra.local_converter import LocalConverter
|
from infra.local_converter import LocalConverter
|
||||||
|
|
@ -99,7 +100,10 @@ def _build_ingestion_service() -> IngestionService | None:
|
||||||
from infra.opensearch_store import OpenSearchStore
|
from infra.opensearch_store import OpenSearchStore
|
||||||
|
|
||||||
embedding = EmbeddingClient(settings.embedding_url)
|
embedding = EmbeddingClient(settings.embedding_url)
|
||||||
vector_store = OpenSearchStore(settings.opensearch_url)
|
vector_store = OpenSearchStore(
|
||||||
|
settings.opensearch_url,
|
||||||
|
default_limit=settings.opensearch_default_limit,
|
||||||
|
)
|
||||||
config = IngestionConfig(
|
config = IngestionConfig(
|
||||||
embedding_dimension=settings.embedding_dimension,
|
embedding_dimension=settings.embedding_dimension,
|
||||||
)
|
)
|
||||||
|
|
|
||||||
|
|
@ -19,6 +19,7 @@ class TestSettingsDefaults:
|
||||||
assert s.max_page_count == 0
|
assert s.max_page_count == 0
|
||||||
assert s.max_file_size_mb == 50
|
assert s.max_file_size_mb == 50
|
||||||
assert s.batch_page_size == 0
|
assert s.batch_page_size == 0
|
||||||
|
assert s.opensearch_default_limit == 1000
|
||||||
assert s.upload_dir == "./uploads"
|
assert s.upload_dir == "./uploads"
|
||||||
assert s.db_path == "./data/docling_studio.db"
|
assert s.db_path == "./data/docling_studio.db"
|
||||||
assert "http://localhost:3000" in s.cors_origins
|
assert "http://localhost:3000" in s.cors_origins
|
||||||
|
|
@ -103,6 +104,12 @@ class TestSettingsValidation:
|
||||||
with pytest.raises(ValueError, match="lock_timeout must be > 0"):
|
with pytest.raises(ValueError, match="lock_timeout must be > 0"):
|
||||||
Settings(lock_timeout=0)
|
Settings(lock_timeout=0)
|
||||||
|
|
||||||
|
def test_zero_opensearch_default_limit_rejected(self):
|
||||||
|
import pytest
|
||||||
|
|
||||||
|
with pytest.raises(ValueError, match="opensearch_default_limit must be >= 1"):
|
||||||
|
Settings(opensearch_default_limit=0)
|
||||||
|
|
||||||
def test_invalid_table_mode_rejected(self):
|
def test_invalid_table_mode_rejected(self):
|
||||||
import pytest
|
import pytest
|
||||||
|
|
||||||
|
|
@ -146,6 +153,7 @@ class TestSettingsFromEnv:
|
||||||
monkeypatch.setenv("MAX_PAGE_COUNT", "20")
|
monkeypatch.setenv("MAX_PAGE_COUNT", "20")
|
||||||
monkeypatch.setenv("MAX_FILE_SIZE_MB", "100")
|
monkeypatch.setenv("MAX_FILE_SIZE_MB", "100")
|
||||||
monkeypatch.setenv("BATCH_PAGE_SIZE", "15")
|
monkeypatch.setenv("BATCH_PAGE_SIZE", "15")
|
||||||
|
monkeypatch.setenv("OPENSEARCH_DEFAULT_LIMIT", "500")
|
||||||
monkeypatch.setenv("UPLOAD_DIR", "/data/uploads")
|
monkeypatch.setenv("UPLOAD_DIR", "/data/uploads")
|
||||||
monkeypatch.setenv("DB_PATH", "/data/test.db")
|
monkeypatch.setenv("DB_PATH", "/data/test.db")
|
||||||
monkeypatch.setenv("CORS_ORIGINS", "http://a.com, http://b.com")
|
monkeypatch.setenv("CORS_ORIGINS", "http://a.com, http://b.com")
|
||||||
|
|
@ -163,6 +171,7 @@ class TestSettingsFromEnv:
|
||||||
assert s.max_page_count == 20
|
assert s.max_page_count == 20
|
||||||
assert s.max_file_size_mb == 100
|
assert s.max_file_size_mb == 100
|
||||||
assert s.batch_page_size == 15
|
assert s.batch_page_size == 15
|
||||||
|
assert s.opensearch_default_limit == 500
|
||||||
assert s.upload_dir == "/data/uploads"
|
assert s.upload_dir == "/data/uploads"
|
||||||
assert s.db_path == "/data/test.db"
|
assert s.db_path == "/data/test.db"
|
||||||
assert s.cors_origins == ["http://a.com", "http://b.com"]
|
assert s.cors_origins == ["http://a.com", "http://b.com"]
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue