fix(ingest): warn when provider=none disables in-cluster; precise model fallback
Address claude-review round 4 on #922: - Important 1: build_ocr_backend now warns when gateway_only + provider=none + DOCUMENT_OCR_INCLUSTER_ENABLED=true — provider=none suppresses the gateway-only in-cluster tier too (it never uses the mistral provider), which surprises an operator who set none just to disable Mistral. Restructured so the gateway_only branch is evaluated before the generic provider=none return. Two tests cover the warn-when-enabled / silent-when-disabled cases. - Nit 3: model fallback uses `model if model is not None else ...` (not `or`), so an empty model string no longer silently falls back to the upstream default and misroutes a per-tier rung. Test added. - Nit 4: the no-surya-literal guard now uses rglob so future document_processors/ subdirs are covered. Deferred (pre-existing / out of scope for this PR): - Important 2 (_GatewayOcrBackend opens a new httpx.AsyncClient per ocr() call): a pre-existing pattern affecting both OCR rungs; a shared pooled client needs careful per-pod lifecycle handling (event-loop binding, aclose) and is better as its own change. Follow-up. - Nit 5 (_MANAGED_QUEUES vs the CLI all-queues list): the two sets differ deliberately (the CLI list includes ingest-maintenance, _MANAGED_QUEUES does not), so a shared constant wouldn't cleanly dedupe them. 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
ebbf905dc5
commit
0614709960
@@ -239,11 +239,22 @@ def build_ocr_backend(
|
|||||||
disabled (warn) rather than misrouted to a direct backend that can't serve it.
|
disabled (warn) rather than misrouted to a direct backend that can't serve it.
|
||||||
"""
|
"""
|
||||||
provider = settings.document_ocr_provider
|
provider = settings.document_ocr_provider
|
||||||
model = model or settings.document_ocr_model
|
# `is not None` (not truthiness): an empty model string must NOT silently fall
|
||||||
if provider == "none":
|
# back to the upstream default and misroute a per-tier rung.
|
||||||
return None
|
model = model if model is not None else settings.document_ocr_model
|
||||||
|
|
||||||
if gateway_only:
|
if gateway_only:
|
||||||
|
# The in-cluster tier is gateway-only and never touches the `mistral`
|
||||||
|
# provider, but provider=none still suppresses it. Warn so an operator who
|
||||||
|
# set provider=none to disable Mistral doesn't silently lose the GPU tier.
|
||||||
|
if provider == "none":
|
||||||
|
if settings.document_ocr_incluster_enabled:
|
||||||
|
logger.warning(
|
||||||
|
"DOCUMENT_OCR_PROVIDER=none disables in-cluster OCR even with "
|
||||||
|
"DOCUMENT_OCR_INCLUSTER_ENABLED=true; set it to 'gateway' or "
|
||||||
|
"'auto' to keep the in-cluster tier"
|
||||||
|
)
|
||||||
|
return None
|
||||||
if settings.embedding_gateway_url:
|
if settings.embedding_gateway_url:
|
||||||
return _GatewayOcrBackend(
|
return _GatewayOcrBackend(
|
||||||
settings.embedding_gateway_url,
|
settings.embedding_gateway_url,
|
||||||
@@ -256,6 +267,9 @@ def build_ocr_backend(
|
|||||||
)
|
)
|
||||||
return None
|
return None
|
||||||
|
|
||||||
|
if provider == "none":
|
||||||
|
return None
|
||||||
|
|
||||||
if provider in ("gateway", "auto") and settings.embedding_gateway_url:
|
if provider in ("gateway", "auto") and settings.embedding_gateway_url:
|
||||||
return _GatewayOcrBackend(
|
return _GatewayOcrBackend(
|
||||||
settings.embedding_gateway_url,
|
settings.embedding_gateway_url,
|
||||||
|
|||||||
@@ -18,6 +18,7 @@ def _settings(**kw) -> Any: # a Settings stand-in (only the read fields matter)
|
|||||||
base = dict(
|
base = dict(
|
||||||
document_ocr_provider="auto",
|
document_ocr_provider="auto",
|
||||||
document_ocr_model="mistral/mistral-ocr-latest",
|
document_ocr_model="mistral/mistral-ocr-latest",
|
||||||
|
document_ocr_incluster_enabled=False,
|
||||||
document_ocr_timeout_seconds=180.0,
|
document_ocr_timeout_seconds=180.0,
|
||||||
document_ocr_mode="sync",
|
document_ocr_mode="sync",
|
||||||
document_ocr_batch_poll_seconds=120,
|
document_ocr_batch_poll_seconds=120,
|
||||||
@@ -124,6 +125,55 @@ def test_build_backend_gateway_only_no_url_disabled():
|
|||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_backend_gateway_only_provider_none_warns_when_incluster_enabled(caplog):
|
||||||
|
"""provider=none also suppresses the gateway-only in-cluster tier (it never uses
|
||||||
|
the mistral provider, but the global `none` gate still applies). When in-cluster
|
||||||
|
is enabled this is almost certainly an operator mistake -> warn so it's visible."""
|
||||||
|
with caplog.at_level(
|
||||||
|
"WARNING", logger="nextcloud_mcp_server.document_processors.ocr"
|
||||||
|
):
|
||||||
|
b = ocr.build_ocr_backend(
|
||||||
|
_settings(
|
||||||
|
document_ocr_provider="none",
|
||||||
|
embedding_gateway_url="http://gw",
|
||||||
|
document_ocr_incluster_enabled=True,
|
||||||
|
),
|
||||||
|
gateway_only=True,
|
||||||
|
)
|
||||||
|
assert b is None
|
||||||
|
assert any(
|
||||||
|
"DOCUMENT_OCR_PROVIDER=none disables in-cluster" in r.message
|
||||||
|
for r in caplog.records
|
||||||
|
)
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_backend_gateway_only_provider_none_silent_when_incluster_disabled(
|
||||||
|
caplog,
|
||||||
|
):
|
||||||
|
"""provider=none with in-cluster OFF is a deliberate disable -> no warning noise."""
|
||||||
|
with caplog.at_level(
|
||||||
|
"WARNING", logger="nextcloud_mcp_server.document_processors.ocr"
|
||||||
|
):
|
||||||
|
b = ocr.build_ocr_backend(
|
||||||
|
_settings(document_ocr_provider="none", embedding_gateway_url="http://gw"),
|
||||||
|
gateway_only=True,
|
||||||
|
)
|
||||||
|
assert b is None
|
||||||
|
assert not any("disables in-cluster" in r.message for r in caplog.records)
|
||||||
|
|
||||||
|
|
||||||
|
def test_build_backend_empty_model_does_not_fall_back(caplog):
|
||||||
|
"""An empty model string must NOT silently fall back to the upstream default
|
||||||
|
(an `or` would); the in-cluster rung keeps the empty id it was handed."""
|
||||||
|
b = ocr.build_ocr_backend(
|
||||||
|
_settings(embedding_gateway_url="http://gw"),
|
||||||
|
model="",
|
||||||
|
gateway_only=True,
|
||||||
|
)
|
||||||
|
assert isinstance(b, ocr._GatewayOcrBackend)
|
||||||
|
assert b._model == ""
|
||||||
|
|
||||||
|
|
||||||
def test_build_backend_model_override_is_not_hardcoded():
|
def test_build_backend_model_override_is_not_hardcoded():
|
||||||
"""The per-tier model is whatever config passes -- surya by default, but fully
|
"""The per-tier model is whatever config passes -- surya by default, but fully
|
||||||
swappable (e.g. lightonocr) with no code change."""
|
swappable (e.g. lightonocr) with no code change."""
|
||||||
@@ -186,7 +236,7 @@ def test_no_surya_string_literal_in_document_processors():
|
|||||||
pkg = pathlib.Path(ocr.__file__).parent
|
pkg = pathlib.Path(ocr.__file__).parent
|
||||||
offenders = [
|
offenders = [
|
||||||
f"{p.name}: {ln.strip()}"
|
f"{p.name}: {ln.strip()}"
|
||||||
for p in pkg.glob("*.py")
|
for p in pkg.rglob("*.py") # recurse: future backends/ subdirs too
|
||||||
for ln in p.read_text().splitlines()
|
for ln in p.read_text().splitlines()
|
||||||
if '"surya' in ln.split("#", 1)[0] or "'surya" in ln.split("#", 1)[0]
|
if '"surya' in ln.split("#", 1)[0] or "'surya" in ln.split("#", 1)[0]
|
||||||
]
|
]
|
||||||
|
|||||||
Reference in New Issue
Block a user