Address claude-review round 2 on #922:
- Legacy `ingest-ocr` tier resolution (important 1): document why it deliberately
resolves to `fast` rather than mapping to `ocr-upstream` — stranded pre-split
jobs re-extract empty and re-escalate via the ladder to the cheap
`ocr-incluster` rung, keeping them OFF the paid upstream rung. Added a
tier_for_queue(LEGACY_INGEST_QUEUE_OCR) == "fast" assertion.
- Double get_settings() in `_get_batch_client` (important 2): bind once to a local.
- Stale docstrings (important 3): OcrProcessor (serves both rungs now),
_tier_available (both OCR rungs gated), evaluate_escalation (targets
ocr-incluster, falls through to ocr-upstream).
- Misconfigured model_setting (nit 5): OcrProcessor.__init__ raises ValueError on
an unknown settings attr (fail-fast at startup vs AttributeError mid-OCR); also
removes the dynamic-getattr static-analysis smell SonarCloud flagged.
- Redundant guard (nit 4): kept `and ocr_tier is not None` — it's required for ty
to narrow ocr_tier to str for record_document_escalation; added a comment.
- Test gap (nit 6): added a test pinning the CURRENT incluster-failure ->
tier-1 fallback (does NOT cascade to upstream) so the future 503-escalation
change is an explicit diff.
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>
Address round-3 review on PR #914:
- When a glyph-corrupt doc's structured rung is NOT registered, the inline path
now falls through to OCR (with reason corrupt_glyphs), mirroring the external
next_available_tier instead of silently keeping the fast result. A structured
parse FAILURE remains terminal (tracked via structured_failed), matching the
external path which does not escalate a failure. Added a debug log for the
unregistered case and "(OCR not attempted)" to the failure warning.
- Tests: inline + external glyph-corrupt fallthrough to OCR when structured is
unregistered; glyph-corrupt + junk-quality both-flags precedence (structured
wins over the bad_text_layer/ocr route).
- Note the total_chars>0 mutual-exclusion with the scanned branch in
_route_from_signals.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address round-1 review on PR #914:
- Attribute the OCR hop in a fast->structured->ocr inline cascade to
from_tier="structured" (not a second "fast" escalation), so
astrolabe_document_escalation_total per-tier counts stay accurate.
- Add test_inline_fast_structured_ocr_cascade pinning that two-hop path and the
metric attribution.
- Note in classify_from_text that its doc-level control ratio is over full_text
(all pages), not the sampled subset classify_pdf uses.
- Clarify that corrupt_glyphs never lands in the suppressed-escalation counter.
- Dedupe the glyph-corrupt test string into tests/fixtures/glyph_corruption.py.
- Use pytest.approx for the control-char-ratio zero checks (SonarCloud S1244).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The fast (pypdfium2) extractor can leak raw glyph codes on subset fonts with a
broken /ToUnicode CMap. The result scores high on the existing text-quality
heuristic -- a uniform glyph/Caesar offset preserves whitespace and token
lengths -- yet is unsearchable. The structured (pymupdf) tier extracts the same
pages correctly.
Add a language-agnostic C0-control-character-ratio signal to the tier-0
classifier that detects this corruption and routes the document to a new
`structured` recommended_tier. Wire the fast->structured hop on the inline path
and generalise it so a low-quality-but-non-empty layer also tries structured
before OCR -- the inline and external ingest modes now follow the full
fast->structured->ocr ladder identically. A scanned / no-text-layer document
(total_chars == 0) still shortcuts straight to OCR, since a text extractor
cannot recover a pure raster.
New per-tenant tunable DOCUMENT_GLYPH_CORRUPTION_RATIO (default 0.02); escalation
metrics gain a `corrupt_glyphs` reason label.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review round 4 (both nits):
- Rename the flag to ignore_ocr_enabled so its OCR-specific scope is explicit at
the call sites (the gate only bypasses the OCR-enabled check).
- Add test_evaluate_escalation_empty_suppressed_even_when_structured_registered:
empty_text (minimum='ocr') skips a registered structured tier and suppresses to
ocr when OCR is off, never hopping to structured.
Deck #324.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The inline _process_pdf path does not emit document_escalation_suppressed_total;
the "what-if OCR" counter is instrumented only on the per-tier external path
(evaluate_escalation / _parse_pdf_tier). Comment the inline OCR gate so a reader
doesn't mistake the omission for a bug. Deferred the assert_never nit (typing
.assert_never is 3.11+; Literal+frozen already guard construction) and the
pre-existing minimum-ValueError pass (only "ocr"/None are ever passed).
Deck #324.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- escalation: EscalationDecision.kind is now Literal["hop","suppressed"] so ty
catches a bad kind statically (and the processor branch is exhaustive).
- processor: simplify the suppressed-escalation log line (no longer repeats
to_tier / tier).
- registry: clarify _tier_available's ignore_enabled drops the OCR-enabled gate
specifically (a future per-tier gate would extend the condition).
Deck #324.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OCR is the paid, opt-in tier (DOCUMENT_OCR_ENABLED, default off). The per-tier
escalation gate already declines to hop to OCR when it's disabled (the pre-OCR
tier is terminal — no surprise cost), but that left operators blind to how much
OCR demand exists.
evaluate_escalation now returns a structured EscalationDecision:
- "hop" — a higher tier can run; the caller raises EscalateError (queue-hop).
- "suppressed" — the ideal next tier (e.g. ocr) exists but is DISABLED; the caller
indexes the current tier's output as terminal and records the
would-be hop on the new astrolabe_document_escalation_suppressed_total
{from_tier,to_tier,reason} counter instead of hopping.
- None — index as-is (good text, or no such tier at all).
So with OCR off, escalation_suppressed_total{to_tier="ocr"} is the latent OCR
demand an operator weighs before enabling OCR; enabling it converts these into
real document_escalation_total{to_tier="ocr"} hops. next_available_tier gains an
ignore_enabled flag to compute the *ideal* (enabled-gate-ignored) target.
Tests: registry suppressed vs hop vs terminal (incl. structured-hop-not-suppressed
when OCR off but structured available); _parse_pdf_tier records suppressed +
indexes without raising.
Deck #324 (parent #323).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Split external (procrastinate) document processing into per-tier queues so a
document is attempted at most once per tier and requeued to the next tier's
queue on a low-quality parse, using procrastinate's native retry.
- escalation.py: TIER_LADDER (fast->structured->ocr) + EscalateError signal
- registry: process_tier (one tier) + evaluate_escalation post-parse gate
(reuses classify_from_text) + next_available_tier; shared _classify_result
and _oversize_result with the inline pipeline
- processor: process_document(tier=...) runs one tier and raises EscalateError
before embed (junk text never indexed); inline memory path unchanged
- queue/procrastinate: ingest-fast|structured|ocr queues; TieredEscalationStrategy
(queue-hop on EscalateError, bounded same-tier transient retry); queue-aware
task; producer defers to ingest-fast; per-queue counts + all-queue reclaim
- cli: worker --tier {fast,structured,ocr}
- billing: pages_ocr usage event + pipeline_tier metadata (paid OCR billed apart)
- observability: astrolabe_ingest_queue_depth{queue,status} gauge + per-queue
counts in nc_get_vector_sync_status / management status endpoint
- config: INGEST_ESCALATION_ENABLED (default true), INGEST_TRANSIENT_MAX_ATTEMPTS
INGEST_ESCALATION_ENABLED=false and INGEST_QUEUE=memory preserve prior behaviour.
Deck #323.
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>
Address PR #863 review:
- MIN_TEXT_QUALITY 0.45 -> 0.5 so the module/diagnostic default matches the
DOCUMENT_OCR_MIN_TEXT_QUALITY setting (registry always passes the setting; this
keeps classify_pdf and the test/default path on the production threshold).
- image_coverage_per_page is bounded to MAX_SAMPLED_PAGES (the image pass is the
costly part, so a 200-page scan isn't fully rasterised on the hot path); pages
beyond the cap fall back to the text-quality signal, and page_fraction still
gates over every page.
- Extracted _page_image_coverage(page) helper, shared by classify_pdf and
image_coverage_per_page (DRY + keeps the tiling-double-count note in one place).
- Scan-detection failure logs at WARNING (not DEBUG) so a systematic failure on
an OCR-enabled tenant is visible at LOG_LEVEL=INFO.
- Add the missing DOCUMENT_OCR_MIN_PAGE_CHARS range-validator test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The hot-path classifier escalated to OCR purely on character count, so a
scanned/handwritten PDF with a low-quality embedded text layer (>16 chars/page
but garbled) routed `fast` and indexed the junk -- e.g. Student 147.pdf's
"Little Acoms Primary"/"0110912020", which pollutes the vector and demotes the
doc in search (Deck #207).
- classifier: recalibrate `_text_quality` with a long-token-fraction term that
detects word-merging (dropped inter-word spaces) -- the dominant junk-layer
failure the old whitespace/overlong(>20) terms missed. Measured: the Student
147 scan ~0.42 (60% pages junk) vs >=0.94 for clean digital docs.
- classify_from_text now routes on quality + scan: a page is OCR-worthy if
near-empty OR low text-quality OR (when OCR + scan detection are enabled) it's
mostly a raster image. New `image_coverage_per_page` re-opens the PDF for the
scan signal, so that cost is paid only by OCR-opted-in tenants. Thresholds are
passed in from per-tenant settings (keyword-only).
- config: 4 per-tenant settings -- DOCUMENT_OCR_MIN_TEXT_QUALITY (0.5),
DOCUMENT_OCR_PAGE_FRACTION (0.5), DOCUMENT_OCR_MIN_PAGE_CHARS (16),
DOCUMENT_OCR_DETECT_SCANNED (true) -- with range validators.
- metrics: new astrolabe_document_ocr_page_fraction histogram (the value the
page-fraction threshold acts on) alongside document_text_quality, so operators
can tune the OCR escalation per tenant (quality vs cost).
Escalation gate, OCR backends, and off-by-default behavior unchanged (#858).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review round 2:
- OcrProcessor backend resolution is now guarded by an anyio.Lock (lazy-init,
double-checked) so a burst of concurrent first-OCR calls resolves the backend
once instead of each fetching its own gateway M2M token.
- The document_tier1_engine=pymupdf rollback now logs a warning when it falls
back to the fast processor (no 'structured' registered) instead of silently
using the very engine the operator opted out of.
- classify_from_text defaults ocr_frac to 0.0 (not 1.0) for a zero-page PDF, so
the recorded classification metric is "fast" (no OCR evidence) rather than a
misleading "ocr"; the no_text_layer/bad_text_layer flags are gated on having
sampled at least one page.
New tests: zero-page classify routes fast, rollback-fallback warning.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address PR #858 review:
- 🔴 OcrProcessor now resolves its backend once and reuses it. Rebuilding per
call created a fresh GatewayTokenProvider each time -- discarding its M2M-token
cache, so every OCR'd document fetched a new token -- and a new Mistral client.
- 🔴 build_ocr_backend uses explicit ValueError (not assert, which is stripped
under `python -O`) for the gateway M2M triple.
- PIPELINE_TIER in the Qdrant payload now reflects the tier that actually
produced the doc: the registry stamps result.metadata["pipeline_tier"] and the
processor reads it (was hardcoded "fast", wrong for OCR/structured).
- Escalation now requires classification.page_count > 0, so a zero-page
(empty/corrupt) PDF isn't pointlessly sent to OCR; documented that a fast
FAILURE (encrypted/unopenable) is a hard failure and is not OCR-escalated.
- Documented the OCR page_boundaries separator-attribution choice.
- Downgraded the per-document page-boundary / page-assignment INFO logs to debug.
New tests: zero-page no-escalation, pipeline_tier stamping.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OCR is an enhancement, not a gate. Previously, escalating a scanned doc to the
OCR tier returned the OCR result unconditionally -- so with DOCUMENT_OCR_ENABLED
=true but no backend configured (no gateway URL / no MISTRAL_API_KEY) the OCR
processor returned success=False and the whole document was marked failed and
skipped: strictly worse than leaving OCR off (where it would at least index the
tier-1 text).
Now the registry keeps the tier-1 fast result when the OCR escalation doesn't
succeed (no backend, API down, empty output), logging a warning. A
misconfiguration degrades gracefully instead of dropping scanned docs.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replaces single-engine pymupdf4llm extraction with a tiered pipeline (Deck #205,
follows the tier-0 classifier #855). pypdfium2 becomes the default and only
hot-path PDF extractor; pymupdf4llm is deprecated to a rollback toggle.
Why: pymupdf4llm's O(n^2) find_tables drove the OOM (#852) and the form-PDF
parse timeouts (#856), carries AGPL/commercial licensing liability, and -- per
the benchmarks -- recovers near-zero usable tables on the real corpus. pypdfium2
(Apache/BSD) extracts the same text far faster (Student 1a.pdf: 120s timeout ->
0.2s) with no table-detection bomb.
- document_processors/pypdfium2_fast.py: tier-1 "fast" processor emitting text +
exact page_boundaries (the pdf_highlighter contract). pymupdf processor is now
tier "structured" (the rollback engine), registered but not default.
- registry: tiered routing in ProcessorRegistry. tier-1 fast extracts, then
classification is DERIVED from that text (classifier.classify_from_text -- no
PDF re-open), records the classification metrics, and escalates scanned /
no-text-layer docs to the "ocr" tier when document_ocr_enabled (default off;
no provider yet, so fast is terminal). Wires record_document_escalation + the
real "escalated" span attribute (was hardcoded False).
- Removes the separate _shadow_classify pass from vector/processor.py -- it
re-opened every PDF and re-extracted text (~0.5-1.3s/doc of pure duplicated
CPU that lowered throughput); classification now rides the tier-1 extraction.
- Settings: document_tier1_engine ("pypdfium2" default | "pymupdf" rollback,
enum-validated), document_ocr_enabled (default false).
Tests: pypdfium2 extractor, registry tiering (fast routing, rollback, classify
recording, OCR escalation on/off), classify_from_text. Full unit suite green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Failed deletes no longer bump astrolabe_documents_indexed_total: the outer
except in process_document now gates doc_type on operation != "delete", so a
delete error is counted as processed-error but not as an indexing event.
Added test_failed_delete_is_processed_but_not_indexed.
- registry parse span: pass record_exception=True explicitly (matches
instrument_tool) and add a structured logger.warning on the parse-error path
(processor/tier/byte_size/duration_ms) for a Loki-aggregatable failed-parse
signal.
- test_error_does_not_increment_throughput: snapshot-before/delta pattern
instead of absolute 0.0 (counters are global singletons).
- config: document the deliberate gateway asymmetry between
get_embedding_model_name() (no gateway branch) and
get_embedding_provider_family() (short-circuits on gateway).
- Cleanup in touched scope: narrow `except (HTTPStatusError, Exception)` to
`except Exception` (drop now-unused import); convert registry signatures from
Optional[...] to `... | None`.
Refs Deck #175, PR #831.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make per-tier bottlenecks in the document-processing pipeline
(scan -> fetch -> parse -> chunk -> embed -> Qdrant upsert) visible via
metrics, traces, and structured logs. Today the document_processors layer
emits only a logger.info line: no metric, no span, and page counts live only
inside a log string. The single processing-duration histogram is unlabeled and
whole-document, so it cannot isolate parse vs embed vs upsert.
New astrolabe_* metric family (distinct from the mcp_* protocol metrics):
- astrolabe_document_parse_{duration_seconds,total} + pages/chars/bytes counters
recorded at the ProcessorRegistry.process() boundary (covers all current and
future processors uniformly)
- astrolabe_document_escalation_total (dormant; tiered-pipeline readiness)
- astrolabe_embedding_{duration_seconds,requests_total,chunks_total,chars_total}
- astrolabe_document_chunks_total, astrolabe_documents_indexed_total{source,status}
Tracing: new document_processor.parse child span + enriched embed/chunk span
attributes (provider/model/batch_size/chunk_count). Structured logs gain a
consistent field vocabulary (doc_id, doc_type, processor, tier, pages, chars,
byte_size, chunks, duration_ms, status) so Loki can aggregate without regex.
Tier-readiness: processor/tier are labels from day one and a tier property is
added to DocumentProcessor, so adding docling/OCR/LLM tiers later is additive
(new label values, never new metrics). Tenant comes from the kube namespace
label; mime_type/model are span attributes only (cardinality). Existing
mcp_vector_sync_*/mcp_qdrant_* are left untouched.
Refs Deck #175 (superset of #173 Phase 2). Dashboard/recording-rules follow-up
tracked on #175 for homelab-argocd.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
