Address claude-review round 3 on #922:
- Important: _process_batch emitted "no gateway backend (provider=mistral or
EMBEDDING_GATEWAY_URL unset)" for the gateway_only in-cluster rung, where the
gateway IS configured — sending operators chasing a non-existent config
problem. The real reason is "in-cluster GPU is synchronous-only; batch is the
upstream path". Guard the warning with `if not self._gateway_only`. Extended
test_gateway_only_processor_never_uses_batch_mode to drive _process_batch and
assert _batch_fallback_warned stays False.
- Nits: refresh stale ladder in escalation.py module docstring
(fast->structured->ocr-incluster->ocr-upstream); fix "minimum='ocr'" ->
"ocr-incluster" in a test docstring; rename stale tier="ocr" ->
"ocr-upstream" in test_process_tier_oversize_fails_fast.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address claude-review on #922:
Blocking — test coverage for the new tier2 rung:
- test_registry_tiering.py: inline empty-text routes to ocr-incluster before
ocr-upstream; only-incluster-enabled routes to incluster; disabled-incluster
skips to upstream; evaluate_escalation empty_text hops to ocr-incluster (and
falls through to upstream when incluster off); next_available_tier walks the
full fast→structured→ocr-incluster→ocr-upstream ladder + ignore_ocr_enabled
ideal-target.
- test_escalation_signature.py: enabling document_ocr_incluster_enabled changes
the dead-letter signature (independent of the upstream rung).
- test_tiered_escalation_strategy.py: structured→ocr-incluster hops to
INGEST_QUEUE_OCR_INCLUSTER; tier_for_queue covers the in-cluster queue.
Important — real fixes:
- registry.py: run scan detection (image_coverage_per_page) when EITHER OCR rung
is enabled, not just the upstream one — a tenant with only in-cluster OCR on
was missing image-coverage scan signals.
- ocr.py: the gateway_only (in-cluster) processor never enters batch mode — the
GPU is synchronous/low-latency; batch OCR is the upstream Mistral async path.
_get_batch_client short-circuits to None. Covered by a new test.
Nit:
- cli.py: worker --tier help lists ocr-incluster/ocr-upstream as separate fleets.
Left as-is: the lazy anyio.Lock init in OcrProcessor — instances ARE created at
module import (document_processors/__init__.py), so deferring lock creation off
import time is still required; moving it into __init__ would reintroduce the
import-time-primitive issue the comment guards against.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Insert a configurable in-cluster OCR rung into the escalation ladder (Deck #353):
a tier2-eligible doc is OCR'd on the on-demand burst GPU before falling through to
paid upstream OCR. The in-cluster backend is reached ONLY via the embedding gateway
(model prefix routes to the GPU over the tailnet) and is a config value (default
surya/surya-ocr-2, swappable to e.g. lightonocr) — never hard-coded.
Ladder: fast -> structured -> ocr-incluster -> ocr-upstream
(queues ingest-ocr-incluster / ingest-ocr-upstream).
- escalation.py: 4-tier ladder; in-cluster flag folded into the dead-letter signature.
- ocr.py: OcrProcessor(name, tier, model_setting, gateway_only); build_ocr_backend(
..., model=, gateway_only=) — gateway_only forces the gateway backend (never the
direct Mistral fallback), disabling the tier with a warning if no gateway URL.
- registry.py: per-rung enable map; scanned docs target minimum="ocr-incluster";
inline path runs the cheapest available OCR rung.
- procrastinate.py: two OCR queues; legacy ingest-ocr kept as a drain target.
- config.py: DOCUMENT_OCR_INCLUSTER_ENABLED (off) + DOCUMENT_OCR_INCLUSTER_MODEL.
- __init__.py: register the two OCR instances; vector/processor.py: pages_ocr
metered for the upstream (paid) rung only; cli.py: new --tier choices + legacy drain.
- metrics.py: zero the legacy ingest-ocr queue gauge during rollout.
- tests: migrated to the split ladder + new tests (gateway-only forcing, per-tier
model incl. lightonocr override, no-hard-coded-surya guard). 1792 pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 6 review (PR #910):
- Add the missing propagation test: a transport error (httpx.ConnectError) from
batch submit() propagates out of process() rather than being caught by the sync
OCR try/except or falling back to a sync transcription — guards the intentional
"opted into batch → procrastinate retry, not sync fallback" asymmetry.
- Move the batch-test module imports (BatchPollResult, batch_ocr_store) to the
top of test_ocr_processor.py, dropping the mid-file `# noqa: E402`.
Deferred (reviewer: not actionable for this PR): extracting a lazy-init helper
for the parallel _backend / _batch_client resolution quadruplets.
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 5 review (PR #910), all nits, no blockers:
- _result_from_success: a succeeded job with `pages=[]` (empty list, not just a
missing key) is now a per-document failure ("no pages returned") instead of a
silent 0-chunk success. Test added.
- Comment the deadline-expiry path: the gateway-side job isn't cancelled (no
cancel endpoint at this layer) — it's reaped by the gateway file purge; we just
stop polling it.
- Drop the vestigial status="pending" from the BatchOcrJob test fakes (the column
was removed in round 4; BatchPollResult.status fakes are untouched).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 3 review (PR #910):
- Guard an unexpected terminal batch status in _process_batch: anything that
isn't succeeded/failed (gateway version skew, a new lifecycle state) now marks
the document parse-failed instead of falling through to _pages_to_text([]) — a
0-chunk "success" that silently indexed empty text and re-submitted forever.
Test added.
- gateway_batch_client.submit: raise an actionable ValueError on a 2xx response
with no job_id (was a bare KeyError deep in the caller).
- Document that a _process_batch transport error intentionally propagates to
procrastinate for retry rather than falling back to sync (opt-in batch wants
the retry).
- Annotate _batch_client as GatewayBatchOcrClient | None (TYPE_CHECKING import
already present); clarify the delete_stale_for_doc first-submit no-op comment.
- Add a parametrized build_gateway_batch_client test (the gateway-only invariant:
mistral/none/no-URL -> None; gateway|auto + URL -> client).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 2 review (PR #910):
- BLOCKING: BatchOcrJobStore._shared_lock is now lazy-init (anyio.Lock | None,
created on first shared() call) instead of at class-definition time — matches
the CLAUDE.md "no anyio primitives at import time" rule and OcrProcessor's
pattern. The None-check->assign has no await between, so it's race-free.
- document_ocr_mode now normalizes via _enum_fields (case-insensitive, like
document_ocr_provider) instead of a strict dynaconf is_in Validator, so
DOCUMENT_OCR_MODE=Batch normalizes to "batch" rather than erroring. Tests for
case-normalization + invalid-value rejection.
- TYPE_CHECKING-gated GatewayBatchOcrClient import so build_gateway_batch_client
/ _get_batch_client are typed `GatewayBatchOcrClient | None` instead of Any
(runtime import stays lazy to avoid the import cycle).
- Rename ocr_options -> doc_identity_options (it's threaded to all tiers; only
OCR reads it) + clarify the comment.
- Drop the redundant forward-ref quotes on _shared_instance.
- Add direct _batch_identity unit tests (partial/empty options branches).
Left as follow-up: reusing one httpx.AsyncClient across submit/poll (same
per-call pattern as the existing sync _GatewayOcrBackend; no clean aclose hook
on the cached client today).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round 1 review (PR #910):
- BLOCKING: add document_ocr_mode / _batch_poll_seconds / _batch_max_wait_seconds
to config._field_map — without it dynaconf silently ignored the env vars and
DOCUMENT_OCR_MODE=batch could never be enabled in production. Add a regression
test asserting the three round-trip from env.
- migration 008: give batch_ocr_jobs a composite PRIMARY KEY on
(user_id, doc_id, doc_type, etag) instead of a bare UniqueConstraint (N1).
- OcrProcessor: use a dedicated _batch_client_lock instead of sharing the sync
backend lock (N3).
- tests: use https:// gateway URLs in the new fixtures to clear SonarCloud's
"insecure http" security hotspots (all 14 were test-only http://gw literals).
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add DOCUMENT_OCR_MODE=sync|batch (default sync). In batch mode the tier-3 OCR
processor submits documents to the embedding gateway's async Batch OCR routes
(POST /v1/ocr/batch + GET /v1/ocr/batch/{job_id}, astrolabe-cloud-website#372)
for ~50% cheaper large-corpus backfill. The direct Mistral OCR path is left
untouched. Tracked on Deck #332.
Batch jobs run minutes-hours, so the OCR tier cannot block (the procrastinate
worker reclaims jobs in `doing` after INGEST_STALLED_JOB_SECONDS). Instead it
submits, records the gateway job id in a new per-tenant `batch_ocr_jobs` table
(procrastinate args are immutable across retries), and raises a BatchPending
signal that TieredEscalationStrategy turns into a same-queue deferred re-poll —
releasing the worker slot between polls. On completion the per-page markdown is
indexed like the sync path; a failure or a job past
DOCUMENT_OCR_BATCH_MAX_WAIT_SECONDS marks the document parse-failed.
Batch is opt-in and gateway-only: with the direct mistral backend, no gateway
URL, or the inline/memory pipeline (which can't defer), it falls back to sync.
One batch job per document (coalescing N docs/job is a follow-up).
- embedding/gateway_batch_client.py: submit/poll client (reuses GatewayTokenProvider).
- vector/batch_ocr_store.py + migration 008: job tracking (portable SQLite+PG).
- document_processors/escalation.py: BatchPending control-flow signal.
- document_processors/ocr.py: batch state machine + sync fallback.
- vector/processor.py: thread doc identity to the OCR tier; raise BatchPending
from the pending sentinel; propagate it as control flow (not a failure).
- vector/queue/procrastinate.py: BatchPending -> same-queue retry_in, exempt
from the transient cap (bounded by the processor's deadline).
- config + docs; tests across client/store/processor/strategy/parse-tier.
1653 unit tests pass; ruff + ty green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-3 review on PR #892 found a real bug: the gateway backend's httpx.Timeout
raises httpx.ReadTimeout (a httpx.TimeoutException, NOT a builtin TimeoutError),
so the `except TimeoutError` added in r2 only covered the Mistral
(anyio.fail_after) path — gateway timeouts still fell through to
reason="error". Catch both (TimeoutError, httpx.TimeoutException) so either
backend's timeout lands in the dedicated parse_failed_reason="timeout" bucket.
Add an end-to-end test driving a gateway httpx.ReadTimeout through the
processor.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-2 review on PR #892:
- OcrProcessor.process now catches TimeoutError separately and returns
parse_failed_reason="timeout" with a populated message ("OCR timed out after
Ns"), instead of conflating timeouts with API errors under "error" and logging
an empty suffix. Lets dashboards tell a too-low timeout from a failing
provider. Test added.
- Add validator-rejection tests for DOCUMENT_OCR_TIMEOUT_SECONDS=0 (gte=1) and
DOCUMENT_MAX_PDF_SIZE_MB=-1 (gte=0), matching the existing validator-test
pattern.
- Comment the _Settings test fixture's max_pdf_size_mb=0.0 default.
SonarCloud: quality gate was failing on new_security_hotspots_reviewed (S5332
"use https") from an http:// URL in the new gateway-timeout test — switched to
https:// (mirrors commit 98c9d58e).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Round-1 review on PR #892:
- Wire DOCUMENT_OCR_TIMEOUT_SECONDS into _MistralOcrBackend too (was
gateway-only): wrap process_async in anyio.fail_after so the SDK-managed
client honours the setting; on expiry it fails fast as a clean parse error.
Test added.
- Tighten the misleading "honoured without a restart" comment — per-call
get_settings() is for test monkeypatching; a live change still needs a
restart since the backend is cached for the pod lifetime.
- Comment the size guard's two intentional gaps: an explicit processor_name
override bypasses it, and the early return skips the parse-duration histogram.
SonarCloud (new-code smells in the added tests):
- S1244 float-equality asserts → pytest.approx (test_config.py, test_ocr_processor.py).
- S1186/S7503: rewrite the gateway-timeout test with mocker AsyncMock/MagicMock
instead of a hand-rolled fake client (no empty method, no async-without-await).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Two ingest-robustness fixes from card 309 (OHR-Bench smoke-test triage).
The OCR backend timeout was a hardcoded 180s module constant, so a tenant
whose gateway has its own shorter ceiling couldn't tune it. Promote it to
DOCUMENT_OCR_TIMEOUT_SECONDS (default 180), resolved per call via get_settings
so an override applies without a restart.
Large, awkward PDFs (e.g. a 42 MB scanned DUDE) were handed straight to the
fast/OCR tiers, where they burned the full OCR timeout for zero recovered
text. Add a pre-parse size guard in the tiered PDF pipeline: a PDF over
DOCUMENT_MAX_PDF_SIZE_MB (default 50, 0 disables) fails fast with
parse_failed_reason="oversize" before any tier runs, so the existing
permanent-failure path marks the placeholder failed and records
astrolabe_document_parse_failed_total{reason="oversize"} instead of retrying.
Both knobs go through Settings + dynaconf validators (env-var keys verified by
regression tests) and are documented under Background Indexing Configuration.
Refs: Deck board 12 card 309 (AC #3 OCR timeout + size guard).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Follow-up to the tiered document processor (#858), landing the round-4 review
nits the reviewer approved without:
- pypdfium2_fast: free the page handle in an outer finally so a corrupt page
that makes get_textpage() raise can't orphan it.
- test: classify_from_text junk-text-layer path (non-zero chars, low quality,
high ocr_frac) flags bad_text_layer -- the hot-path coverage gap.
- test: build_ocr_backend raises ValueError when the gateway M2M client_id is
set without its token_url/secret.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds the OCR escalation target the tiered registry already routes to. Scanned /
no-text-layer PDFs (the tier-0 "ocr" verdict) escalate here when
document_ocr_enabled (default off).
Two interchangeable backends, selected by document_ocr_provider
(auto | gateway | mistral | none):
- gateway: POST to the Astrolabe Cloud model gateway's /v1/ocr -- the same
M2M-authenticated gateway as embeddings, so NO provider keys live in the pod
(the platform default; reuses EMBEDDING_GATEWAY_URL + the M2M creds).
- mistral: call the Mistral OCR API directly from the pod (MISTRAL_API_KEY), for
self-hosters / deployments without the gateway.
"auto" prefers the gateway, then direct Mistral.
Both return per-page markdown joined into text + exact page_boundaries (the
pdf_highlighter contract; bbox re-derived from the PDF bytes as for other tiers).
Validated end-to-end via direct Mistral on the scanned Student 147.pdf:
success, 15 pages, 22k chars, offsets exact, ~4s.
Settings: document_ocr_provider (enum-validated), document_ocr_model
("mistral/mistral-ocr-latest" -- gateway routes on the prefix, the direct mistral
backend strips it). OcrProcessor registered at lowest priority so it is never the
non-tiered default.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
