fix(review): _enum_fields validation, gate classify_from_text flags, OCR warnings
Address PR #858 review round 3: - document_tier1_engine / document_ocr_provider now validate + normalize via Settings.__post_init__ _enum_fields (the repo's canonical opt-in-enum pattern; case-insensitive) instead of dynaconf Validators. A typo now raises ValueError at load and "Gateway" normalizes to "gateway". - classify_from_text gates no_text_layer/bad_text_layer on ocr_frac >= OCR_PAGE_FRACTION, matching classify_pdf -- a "fast"-routed doc with a few junk pages no longer emits a misleading flag (keeps the shadow vs hot-path classification metrics consistent). - build_ocr_backend warns when an EXPLICIT provider is misconfigured (gateway without EMBEDDING_GATEWAY_URL, mistral without MISTRAL_API_KEY) instead of silently returning None. - Pypdfium2FastProcessor.health_check probes the import; documented why OcrProcessor.health_check is unconditionally True (lazy per-tenant backends). - Removed the leftover per-boundary / per-chunk debug logging loops. Tests: enum normalization + rejection for the two new settings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
f1272dfe84
commit
bc957b5bc7
@@ -301,12 +301,9 @@ _dynaconf = Dynaconf(
|
|||||||
Validator("DOCUMENT_CHUNK_OVERLAP", gte=0),
|
Validator("DOCUMENT_CHUNK_OVERLAP", gte=0),
|
||||||
# Non-empty strings
|
# Non-empty strings
|
||||||
Validator("VECTOR_SYNC_PDF_TAG", len_min=1),
|
Validator("VECTOR_SYNC_PDF_TAG", len_min=1),
|
||||||
# Enum constraints
|
# Enum constraints (document_* enums are validated + normalized in
|
||||||
|
# __post_init__ via _enum_fields instead, for case-insensitive input).
|
||||||
Validator("LOG_FORMAT", is_in=["text", "json"]),
|
Validator("LOG_FORMAT", is_in=["text", "json"]),
|
||||||
Validator("DOCUMENT_TIER1_ENGINE", is_in=["pypdfium2", "pymupdf"]),
|
|
||||||
Validator(
|
|
||||||
"DOCUMENT_OCR_PROVIDER", is_in=["auto", "gateway", "mistral", "none"]
|
|
||||||
),
|
|
||||||
Validator(
|
Validator(
|
||||||
"LOG_LEVEL",
|
"LOG_LEVEL",
|
||||||
is_in=["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"],
|
is_in=["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"],
|
||||||
@@ -887,6 +884,8 @@ class Settings:
|
|||||||
"embedding_provider": {"autodetect", "gateway"},
|
"embedding_provider": {"autodetect", "gateway"},
|
||||||
"mcp_role": {"api", "worker", "all"},
|
"mcp_role": {"api", "worker", "all"},
|
||||||
"collection_metadata_source": {"qdrant", "api"},
|
"collection_metadata_source": {"qdrant", "api"},
|
||||||
|
"document_tier1_engine": {"pypdfium2", "pymupdf"},
|
||||||
|
"document_ocr_provider": {"auto", "gateway", "mistral", "none"},
|
||||||
}
|
}
|
||||||
for _field, _allowed in _enum_fields.items():
|
for _field, _allowed in _enum_fields.items():
|
||||||
_val = (getattr(self, _field) or "").strip().lower()
|
_val = (getattr(self, _field) or "").strip().lower()
|
||||||
|
|||||||
@@ -213,8 +213,12 @@ def classify_from_text(
|
|||||||
# recorded classification metric accurate rather than a misleading "ocr").
|
# recorded classification metric accurate rather than a misleading "ocr").
|
||||||
ocr_frac = (sum(p.needs_ocr for p in pages) / sampled) if sampled else 0.0
|
ocr_frac = (sum(p.needs_ocr for p in pages) / sampled) if sampled else 0.0
|
||||||
|
|
||||||
|
# Flags gated on ocr_frac >= OCR_PAGE_FRACTION (matching classify_pdf): a
|
||||||
|
# doc that routes "fast" must not carry a junk-layer flag just because a few
|
||||||
|
# isolated pages are bad -- otherwise the classification metric diverges
|
||||||
|
# between this hot path and the standalone classify_pdf.
|
||||||
flags: set[str] = set()
|
flags: set[str] = set()
|
||||||
if sampled:
|
if sampled and ocr_frac >= OCR_PAGE_FRACTION:
|
||||||
if total_chars == 0:
|
if total_chars == 0:
|
||||||
flags.add("no_text_layer")
|
flags.add("no_text_layer")
|
||||||
elif mean_quality < MIN_TEXT_QUALITY:
|
elif mean_quality < MIN_TEXT_QUALITY:
|
||||||
|
|||||||
@@ -171,6 +171,19 @@ def build_ocr_backend(settings: Settings) -> _OcrBackend | None:
|
|||||||
settings.mistral_base_url,
|
settings.mistral_base_url,
|
||||||
)
|
)
|
||||||
|
|
||||||
|
# An EXPLICIT provider that's missing its config is an operator error -- warn
|
||||||
|
# loudly (once, since the backend is resolved+cached) rather than silently
|
||||||
|
# disabling OCR. "auto"/"none" fall through to None quietly by design.
|
||||||
|
if provider == "gateway":
|
||||||
|
logger.warning(
|
||||||
|
"DOCUMENT_OCR_PROVIDER=gateway but EMBEDDING_GATEWAY_URL is unset; "
|
||||||
|
"OCR is disabled"
|
||||||
|
)
|
||||||
|
elif provider == "mistral":
|
||||||
|
logger.warning(
|
||||||
|
"DOCUMENT_OCR_PROVIDER=mistral but MISTRAL_API_KEY is unset; "
|
||||||
|
"OCR is disabled"
|
||||||
|
)
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
|
||||||
@@ -258,4 +271,7 @@ class OcrProcessor(DocumentProcessor):
|
|||||||
)
|
)
|
||||||
|
|
||||||
async def health_check(self) -> bool:
|
async def health_check(self) -> bool:
|
||||||
|
# Backends are resolved lazily (and configured per tenant), so there is
|
||||||
|
# nothing to probe here without making a billable upstream call -- the
|
||||||
|
# processor reports healthy and surfaces a real failure per-document.
|
||||||
return True
|
return True
|
||||||
|
|||||||
@@ -120,4 +120,9 @@ class Pypdfium2FastProcessor(DocumentProcessor):
|
|||||||
return ProcessingResult(text=full_text, metadata=metadata, processor=self.name)
|
return ProcessingResult(text=full_text, metadata=metadata, processor=self.name)
|
||||||
|
|
||||||
async def health_check(self) -> bool:
|
async def health_check(self) -> bool:
|
||||||
return True
|
try:
|
||||||
|
import pypdfium2 # noqa: F401, PLC0415 -- availability probe
|
||||||
|
|
||||||
|
return True
|
||||||
|
except Exception:
|
||||||
|
return False
|
||||||
|
|||||||
@@ -595,14 +595,6 @@ async def _index_document(
|
|||||||
len(page_boundaries),
|
len(page_boundaries),
|
||||||
len(content),
|
len(content),
|
||||||
)
|
)
|
||||||
# Log first 3 page boundaries for debugging
|
|
||||||
for boundary in page_boundaries[:3]:
|
|
||||||
logger.debug(
|
|
||||||
" Page %s: offsets [%s:%s]",
|
|
||||||
boundary["page"],
|
|
||||||
boundary["start_offset"],
|
|
||||||
boundary["end_offset"],
|
|
||||||
)
|
|
||||||
# Verify last boundary matches text length
|
# Verify last boundary matches text length
|
||||||
if page_boundaries:
|
if page_boundaries:
|
||||||
last_boundary = page_boundaries[-1]
|
last_boundary = page_boundaries[-1]
|
||||||
@@ -659,16 +651,6 @@ async def _index_document(
|
|||||||
file_path,
|
file_path,
|
||||||
)
|
)
|
||||||
|
|
||||||
# Log first 3 chunks to see their page assignments
|
|
||||||
for i, chunk in enumerate(chunks[:3]):
|
|
||||||
logger.debug(
|
|
||||||
" Chunk %s: page=%s, offsets=[%s:%s]",
|
|
||||||
i,
|
|
||||||
chunk.page_number,
|
|
||||||
chunk.start_offset,
|
|
||||||
chunk.end_offset,
|
|
||||||
)
|
|
||||||
|
|
||||||
# Warning if NO page numbers were assigned
|
# Warning if NO page numbers were assigned
|
||||||
if assigned_count == 0:
|
if assigned_count == 0:
|
||||||
logger.warning(
|
logger.warning(
|
||||||
|
|||||||
@@ -29,9 +29,13 @@ class TestDecompositionDefaults:
|
|||||||
s = Settings(
|
s = Settings(
|
||||||
collection_metadata_source=" QDRANT ",
|
collection_metadata_source=" QDRANT ",
|
||||||
mcp_role=" API ",
|
mcp_role=" API ",
|
||||||
|
document_tier1_engine=" PyPDFium2 ",
|
||||||
|
document_ocr_provider=" Gateway ",
|
||||||
)
|
)
|
||||||
assert s.collection_metadata_source == "qdrant"
|
assert s.collection_metadata_source == "qdrant"
|
||||||
assert s.mcp_role == "api"
|
assert s.mcp_role == "api"
|
||||||
|
assert s.document_tier1_engine == "pypdfium2"
|
||||||
|
assert s.document_ocr_provider == "gateway"
|
||||||
|
|
||||||
|
|
||||||
class TestEnumValidation:
|
class TestEnumValidation:
|
||||||
@@ -41,6 +45,8 @@ class TestEnumValidation:
|
|||||||
("embedding_provider", "openai"),
|
("embedding_provider", "openai"),
|
||||||
("mcp_role", "leader"),
|
("mcp_role", "leader"),
|
||||||
("collection_metadata_source", "redis"),
|
("collection_metadata_source", "redis"),
|
||||||
|
("document_tier1_engine", "mupdf"),
|
||||||
|
("document_ocr_provider", "gatway"),
|
||||||
],
|
],
|
||||||
)
|
)
|
||||||
def test_invalid_enum_rejected(self, field, value):
|
def test_invalid_enum_rejected(self, field, value):
|
||||||
|
|||||||
Reference in New Issue
Block a user