Skip to content

Add text extraction (OCR / document parsing) capability - #269

Open
saarnilauri wants to merge 3 commits into
WordPress:trunkfrom
saarnilauri:feature/text-extraction-poc
Open

Add text extraction (OCR / document parsing) capability#269
saarnilauri wants to merge 3 commits into
WordPress:trunkfrom
saarnilauri:feature/text-extraction-poc

Conversation

@saarnilauri

@saarnilauri saarnilauri commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Description updated after review. Prose whose meaning is now superseded is struck through and followed by its replacement, so the changes are easy to spot; the Scope and model-resolution sections are additions. The point-by-point response to review feedback is in a separate comment below. This will be tidied into final form before merge.

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_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.
    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 to document.
  • TextExtractionBuilder with withDocument($document, $mimeType) plus AiClient::document() / extractTextResult() / extractText() entry points.
  • BeforeExtractTextEvent / AfterExtractTextEvent, dispatched through the shared event dispatcher forwarded by AiClient::document().

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).

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:

Deferred Why
ExtractedBlock / TextExtractionBlockTypeEnum Block-level normalization across Mistral paragraph boxes, Document AI blocks, and Textract Blocks graphs 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 the fromPages() / includingImages() / includingBlocks() fluent methods Page selection and image inclusion travel via customOptions for 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.
AbstractApiBasedTextExtractionModel There is no dominant wire format to share, so the base class would only hold the metadata/config plumbing AbstractApiBasedModel already provides.
Async TextExtractionOperationModelInterface As stated in #268: the operations track has no concrete in-tree implementation, and text extraction should not pioneer it. Async-only providers poll internally behind the sync interface.

Consequence worth calling out: the ->fromPages([1, 2, 3])->includingImages() example in #268 is not available yet. The equivalent today is usingModelConfig() 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() or usingModel().

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).

@github-actions

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: saarnilauri <laurisaarni@git.wordpress.org>
Co-authored-by: dugyen <ugyensupport@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@codecov

codecov Bot commented Jul 31, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.82979% with 29 lines in your changes missing coverage. Please review.
✅ Project coverage is 87.36%. Comparing base (20a1a6d) to head (162b40c).

Files with missing lines Patch % Lines
src/Events/AfterExtractTextEvent.php 0.00% 16 Missing ⚠️
src/Events/BeforeExtractTextEvent.php 0.00% 12 Missing ⚠️
src/Providers/Models/DTO/ModelRequirements.php 96.42% 1 Missing ⚠️
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     
Flag Coverage Δ
unit 87.36% <93.82%> (+0.81%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jeffpaul
jeffpaul requested a review from a team August 26, 2026 13:58
@dugyen

dugyen commented Sep 2, 2026

Copy link
Copy Markdown

Tested this PR locally (checked out feature/text-extraction-poc, composer install, ran the checks CI would run):

Check Result
composer test (full unit suite) ✅ 1201 tests, 4352 assertions — all passing
Filtered to the new text-extraction tests ✅ 29/29 passing
composer phpcs ✅ clean
composer phpstan ✅ no errors
Merge against trunk ⚠️ conflicts in src/AiClient.php and tests/unit/AiClientTest.php

101 tests suite-wide are flagged "risky" on PHP 8.5 due to a deprecated (no-op) ReflectionProperty::setAccessible() call — pre-existing pattern in the test suite, not introduced by this PR, and not a real failure.

Code looks solid overall — the main actionable item is rebasing onto current trunk to resolve the AiClient.php conflict before merge.

@dugyen

dugyen commented Sep 2, 2026

Copy link
Copy Markdown

Follow-up: a closer read of the diff for possible improvements, beyond the test run.

Likely bugs

  1. PageDimensions::fromArray() (src/Results/DTO/PageDimensions.php) and ExtractedPage::fromArray() (src/Results/DTO/ExtractedPage.php) pass array values straight into int $width/int $height/int $pageNumber constructor params without casting. With declare(strict_types=1), a payload where these arrive as floats (e.g. "pageNumber": 1.0 from a JSON-decoding provider) throws an uncaught TypeError instead of the library's normal InvalidArgumentException. The sibling new DTO BoundingBox::fromArray() already guards against this with explicit (float) casts on its numeric fields — worth adding the analogous (int) casts here for consistency.

  2. ModelRequirements::fromExtractionData() (src/Providers/Models/DTO/ModelRequirements.php) hand-rolls the file→modality mapping ($document->isImage() ? image() : document()) instead of reusing the existing inputModalityForPart() helper a few lines below, which already correctly distinguishes image/audio/video/document/text. As written, an audio or video file passed to withDocument() is silently mapped to document modality rather than rejected or matched to an audio/video-capable model — likely to produce a confusing provider-side failure instead of a clear client-side error.

Design/consistency gaps

  1. TextExtractionBuilder has no EventDispatcherInterface support at all, unlike PromptBuilder/EmbeddingBuilder, which both dispatch Before/After lifecycle events. AiClient::document() correspondingly doesn't forward self::$eventDispatcher. Any listener hooked into the shared event bus (logging/metrics/etc.) will be blind to text-extraction calls — worth confirming this is an intentional scope cut for the PoC rather than an oversight.

  2. TextExtractionBuilder::withDocument() duplicates the string-or-File coercion already in PromptBuilder::withFile()/MessageBuilder::withFile(), and the copy has already drifted — it adds an empty-string guard the other two don't have.

Minor

  1. TextExtractionResult::toText() is a one-line alias of toMarkdown() with no behavioral difference (unlike GenerativeAiResult::toText(), which does real filtering) — an extra public method/doc/test to maintain for no gain.

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.

@dugyen

dugyen commented Sep 2, 2026

Copy link
Copy Markdown

Suggested fixes for the points above:

1. Missing (int) casts in fromArray()

// 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 (float) casts already used in BoundingBox::fromArray(), and keeps bad input surfacing as InvalidArgumentException (via the < 1 checks in the constructors) rather than an uncaught TypeError.

2. Reuse inputModalityForPart() in fromExtractionData()
Rather than the inline ternary, either call the existing helper against a synthetic MessagePart, or extract a shared modalityForFile(File $file): ?ModalityEnum that both inputModalityForPart() and fromExtractionData() call. Then explicitly reject audio/video (and anything inputModalityForPart()-equivalent returns null for) with an InvalidArgumentException in withDocument()/fromExtractionData() rather than silently falling through to document modality — text extraction's docblock already states it only supports image/document/text, so an unsupported file type should fail fast with a clear message, not surface as a downstream "no model found" or provider error.

3. Event dispatcher gap
If this is meant to reach parity with PromptBuilder/EmbeddingBuilder rather than stay PoC-scoped, add an EventDispatcherInterface param to TextExtractionBuilder's constructor (mirroring EmbeddingBuilder), wire AiClient::document() to pass self::$eventDispatcher, and dispatch Before/AfterExtractTextEvent around the $model->extractTextResult($this->document) call in extractTextResult(). If it's an intentional PoC-only cut, a one-line note in the PR description would save the next reader from wondering.

4. withDocument() duplication
Consider hoisting the string-or-File coercion (is_string($document) ? new File(...) : $document) into a small static helper on File itself (e.g. File::fromInput($input, ?string $mimeType): File) that all three builders (PromptBuilder::withFile(), MessageBuilder::withFile(), TextExtractionBuilder::withDocument()) call — and decide once whether the empty-string guard belongs there for everyone, rather than only in the newest copy.

5. toText() alias
Either drop toText() and have callers use toMarkdown() directly, or keep it only if a near-term follow-up (e.g. plain-text-without-markdown stripping) is planned — otherwise it's a maintenance no-op copied from GenerativeAiResult's naming without its actual behavior.

saarnilauri and others added 2 commits September 2, 2026 12:08
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>
@saarnilauri
saarnilauri force-pushed the feature/text-extraction-poc branch from 127caab to bd3abcf Compare September 2, 2026 10:13
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>
@saarnilauri

saarnilauri commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

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 trunk, which resolves the AiClient.php and AiClientTest.php conflicts you hit.

  1. Missing (int) casts in fromArray() fixed in PageDimensions and ExtractedPage. Worth recording that the sharper form of this bug is that AbstractDataTransferObject::isArrayShape() catches only InvalidArgumentException, so a TypeError escaping fromArray() breaks that method's boolean contract. Note also that strict_types permits int-to-float widening, so the missing (int) casts mattered more than the (float) casts already present in BoundingBox.
  2. fromExtractionData() bypassing inputModalityForPart() real bug, confirmed: new File('https://example.com/podcast.mp3') produced inputModalities: ["document"]. Fixed by extracting a shared modalityForFile() that both inputModalityForPart() and extraction use, rather than routing a File through a synthetic MessagePart. Unsupported types are now rejected in withDocument() with a message naming the MIME type, so isSupported() keeps its boolean contract and never sees an invalid file.
  3. Event dispatcher gap not an intentional cut; an oversight against this PR's own spec in Add provider-agnostic text extraction (OCR / document parsing) capability #268. BeforeExtractTextEvent / AfterExtractTextEvent added and wired through AiClient::document(). They carry a File rather than list<MessagePart>, so they parallel the embedding events without literally mirroring them.
  4. withDocument() coercion duplication resolved by removing the drifted empty-string guard rather than adding File::fromInput(). new File('') already throws InvalidArgumentException, so the guard only reworded an existing error while creating the divergence. All three call sites now behave identically and File remains the single validator of its own input. The File::fromInput() refactor touches PromptBuilder and MessageBuilder behavior this PR does not otherwise change, so it is better as a follow-up.
  5. toText() alias removed. Beyond being a no-op, GenerativeAiResult::toText() filters, so a same-named method with different semantics is worse than no method.

Two further changes prompted by that review:

  • getPageCount() was count($this->pages), which cannot represent what Add provider-agnostic text extraction (OCR / document parsing) capability #268 promised ("pages processed", billing-relevant). It is now an optional constructor argument defaulting to the number of returned pages, so connectors can pass the provider's own count (Mistral's usage_info.pages_processed). For page-priced providers, where TokenUsage is zeros, this is the only billing signal in the result.
  • ExtractedPage now validates that its images are ExtractedImage instances, matching how TextExtractionResult validates its pages. These DTOs are populated by third-party connectors, which is where runtime validation earns its keep.

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), composer phpcs and composer phpstan clean locally, and CI green across PHP 7.4 / 8.0 / 8.4 / 8.5 including codecov/patch.

Worth noting on the PHP 8.5 risky-test flag you spotted: still the pre-existing ReflectionProperty::setAccessible() pattern, and the new tests follow the same convention as their neighbours rather than diverging. Probably worth its own issue.

These responses were drafted with the assistance of Claude Code (Anthropic) and reviewed by me; the fixes were applied and verified locally before pushing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add provider-agnostic text extraction (OCR / document parsing) capability

2 participants