Add text extraction (OCR / document parsing) capability - #269
Add text extraction (OCR / document parsing) capability#269saarnilauri wants to merge 3 commits into
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## trunk #269 +/- ##
============================================
+ Coverage 86.54% 87.36% +0.81%
- Complexity 1381 1493 +112
============================================
Files 69 77 +8
Lines 4438 4900 +462
============================================
+ Hits 3841 4281 +440
- Misses 597 619 +22
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Tested this PR locally (checked out
101 tests suite-wide are flagged "risky" on PHP 8.5 due to a deprecated (no-op) Code looks solid overall — the main actionable item is rebasing onto current |
|
Follow-up: a closer read of the diff for possible improvements, beyond the test run. Likely bugs
Design/consistency gaps
Minor
None of these block the PR — the happy-path wiring is clean and well-tested — but #1 and #2 look like real correctness bugs worth fixing before merge. |
|
Suggested fixes for the points above: 1. Missing // PageDimensions::fromArray()
return new self(
(int) $array[self::KEY_WIDTH],
(int) $array[self::KEY_HEIGHT],
isset($array[self::KEY_DPI]) ? (int) $array[self::KEY_DPI] : null
);
// ExtractedPage::fromArray()
return new self(
(int) $array[self::KEY_PAGE_NUMBER],
$array[self::KEY_MARKDOWN],
$images,
isset($array[self::KEY_DIMENSIONS]) ? PageDimensions::fromArray($array[self::KEY_DIMENSIONS]) : null
);Matches the 2. Reuse 3. Event dispatcher gap 4. 5. |
Adds a provider-agnostic text extraction capability, following the structural pattern established by embedding generation in 1.4.0 (dedicated builder, result type, and requirements factory): - CapabilityEnum::TEXT_EXTRACTION with magic accessors. - TextExtractionModelInterface::extractTextResult(File): TextExtractionResult. - Result DTOs: TextExtractionResult (ResultInterface, non-candidate-based), ExtractedPage (1-based page numbers, markdown content), ExtractedImage, BoundingBox (normalized 0-1 coordinates), PageDimensions. - ModelRequirements::fromExtractionData() mapping the document's MIME type to a document or image input-modality requirement. - TextExtractionBuilder with withDocument($document, $mimeType) plus AiClient::document() / extractTextResult() / extractText() entry points. Validated end to end by two downstream provider PoCs with intentionally different API shapes: ai-provider-for-mistral (synchronous dedicated OCR endpoint) and ai-provider-for-llamaparse (async job-based parsing with internal polling). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
127caab to
bd3abcf
Compare
Correctness: - Reject file types text extraction cannot consume. ModelRequirements mapped any non-image file to the document modality, so an audio or video file was silently required as `document` and surfaced later as a confusing "no model found" error or a provider-side failure. The file-to-modality mapping is now shared with inputModalityForPart() via modalityForFile(), and TextExtractionBuilder::withDocument() rejects unsupported types up front with a message naming the type. - Cast integer fields in PageDimensions::fromArray() and ExtractedPage::fromArray(). Under strict_types a JSON payload carrying 1.0 raised an uncaught TypeError, which also escapes isArrayShape()'s InvalidArgumentException contract. - Validate that ExtractedPage images are ExtractedImage instances, matching how TextExtractionResult validates its pages. API: - Dispatch BeforeExtractTextEvent / AfterExtractTextEvent around extraction, as specified in WordPress#268, and forward the shared dispatcher from AiClient::document(). Listeners were previously blind to extraction calls. - Take pageCount as an optional constructor argument, defaulting to the number of returned pages. It was derived from count($pages), so it could not carry a provider's reported pages_processed, which is the only billing signal in the result for page-priced providers. - Drop TextExtractionResult::toText(). It was an alias of toMarkdown() and invited confusion with GenerativeAiResult::toText(), which filters instead. - Drop the empty-string guard from withDocument() so that File stays the single validator of its own input, consistent with PromptBuilder::withFile() and MessageBuilder::withFile(). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Thanks @dugyen for checking out the branch, running the checks, and then reading the diff. That second pass caught a real bug. All five points are now addressed in 162b40c, two of them differently than suggested. The branch is also rebased onto current
Two further changes prompted by that review:
Two related points that came out of your review are documented in the PR description rather than here, since they are design decisions rather than fixes:
Verification on 162b40c: 1250 tests / 4471 assertions passing (44 extraction-specific, up from 29), Worth noting on the PHP 8.5 risky-test flag you spotted: still the pre-existing These responses were drafted with the assistance of Claude Code (Anthropic) and reviewed by me; the fixes were applied and verified locally before pushing. |
Adds a provider-agnostic text extraction capability, following the structural pattern established by embedding generation in 1.4.0 (dedicated builder, result type, and requirements factory):Adds a provider-agnostic text extraction capability, following the structural pattern established by embedding generation in 1.4.0 (dedicated builder, result type, and requirements factory). Note that #274 has since moved embeddings off resolver-based model selection; this PR deliberately does not follow it there — see Model resolution vs. #274 below.
CapabilityEnum::TEXT_EXTRACTIONwith magic accessors.TextExtractionModelInterface::extractTextResult(File): TextExtractionResult.TextExtractionResult(ResultInterface, non-candidate-based),ExtractedPage(1-based page numbers, markdown content),ExtractedImage,BoundingBox(normalized 0-1 coordinates),PageDimensions.ModelRequirements::fromExtractionData()mapping the document's MIME type to a document or image input-modality requirement.ModelRequirements::fromExtractionData()deriving the input-modality requirement from the document's MIME type, and rejecting types that carry no readable text (audio, video, unrecognized) instead of mapping them todocument.TextExtractionBuilderwithwithDocument($document, $mimeType)plusAiClient::document()/extractTextResult()/extractText()entry points.BeforeExtractTextEvent/AfterExtractTextEvent, dispatched through the shared event dispatcher forwarded byAiClient::document().Validated end to end by two downstream provider PoCs with intentionally different API shapes:
ai-provider-for-mistral(synchronous dedicated OCR endpoint) andai-provider-for-llamaparse(async job-based parsing with internal polling).For more details see the issue that this PR closes #268
Scope
This lands step 1 of the rollout plan in #268, minus the pieces that are better designed against shipped connectors. Explicitly deferred, not overlooked:
ExtractedBlock/TextExtractionBlockTypeEnumblocks, and TextractBlocksgraphs needs two shipped connectors to design against. Bounding boxes and page dimensions are in, so nothing here is blocked.ModelConfig::KEY_EXTRACTION_PAGES/_INCLUDE_IMAGES/_INCLUDE_BLOCKS, and thefromPages()/includingImages()/includingBlocks()fluent methodscustomOptionsfor now. Promoting them to first-class config keys means committing to their semantics across providers whose page-range and image flags differ; worth doing once more than one connector is real.AbstractApiBasedTextExtractionModelAbstractApiBasedModelalready provides.TextExtractionOperationModelInterfaceConsequence worth calling out: the
->fromPages([1, 2, 3])->includingImages()example in #268 is not available yet. The equivalent today isusingModelConfig()with custom options, which is what both PoC connectors use.Model resolution vs. #274
#274 landed after this branch was opened and made an explicit model required for embeddings, dropping resolver-based discovery. This PR deliberately keeps the resolver for extraction, matching text and image generation rather than embeddings.
The reason #274 exists is comparability: embedding vectors from different models occupy different spaces, so a silently chosen model yields results that are wrong in a way the caller cannot detect. Text extraction has no equivalent property — markdown from Mistral OCR and markdown from LlamaParse are interchangeable to the consumer, which is the whole point of normalizing to pages of markdown. Callers who care which provider runs the job still say so with
usingProvider()orusingModel().Happy to align with #274 instead if maintainers prefer uniformity across non-prompt capabilities, but it reads as a rationale specific to embeddings rather than a house rule.
Use of AI Tools
This implementations was drafted with the assistance of Claude Code (Anthropic), used for researching the provider APIs, analyzing the SDK architecture, and writing code.This implementation was drafted with the assistance of Claude Code (Anthropic), used for researching the provider APIs, analyzing the SDK architecture, writing code, and applying the fixes from code review. All work was done with a human in the loop: the design direction, scope decisions, and API trade-offs were made or reviewed by the author, and the proof of concept was verified by the author against the live Mistral and LlamaParse APIs (including real integration test runs and inspection of the extracted output).