Repository navigation
perf(pdf): hi_res read every page with pdfminer just to learn whether the PDF has text - #4588
Conversation
…text Before choosing a strategy, partition_pdf_or_image() ran pdfminer over every page and built every element only to learn whether the PDF has extractable text. hi_res and ocr_only discard those elements (hi_res then runs pdfminer again for its text layer), so for those strategies the check now stops at the first page that yields a Text element with non-whitespace text. The per-page element building is factored out of _process_pdfminer_pages() into _pdfminer_page_elements(), and pdf_has_extractable_text() applies it page by page through the same open_pdfminer_pages_generator(), so repair paths, password handling and PDFMinerConfig are unchanged. Strategies that may resolve to fast (auto, fast, and hi_res/ocr_only when their dependencies are missing) keep the full pass, since fast returns its elements. If pdfminer raises while hi_res extracts its text layer, hi_res logs the failure and continues without the text layer. That keeps the outcome of a PDF whose later page pdfminer cannot read: the full up-front pass used to fail on that page and hi_res ran without the text layer, while the presence check now stops before reaching it.
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Shadow auto-approve: would not auto-approve because issues were found.
View guided diff | Re-trigger cubic
cragwolfe
left a comment
There was a problem hiding this comment.
Reviewed be6b47854a0a3d66ad59f7d4b42604976f16b4a0 with GPT Pro. No actionable findings in this change.
The strategy predicate preserves the full extraction pass whenever FAST is reachable, including dependency fallbacks. Other requested strategies can stop the presence check at the first page with text. Both paths use the same page-to-element conversion, and the direct tests cover first/late text, empty documents, passwords, and fallback behavior.
If a later PDFMiner pass fails, the new boundary discards the partial text layer and links and lets OCR process the pages. Rendering, OCR, and output-conversion errors remain outside that boundary. The broad exception handling in the fixture-comparison helpers is imperfect, but the separate direct assertions cover the material cases; no independent production defect or material coverage gap was established.
When combining the related PDF changes, preserve the shared page conversion and coordinate filtering, and keep page-local skips distinct from a document-wide decision to disable PDFMiner. The separate findings on #4583, #4584, and #4585 remain attached to those changes.
All five required branch checks and the required Socket alert check passed on this exact head. No local tests or reproductions were run; performance claims were not treated as independent measurements.
(authored by codex)
There was a problem hiding this comment.
0 issues found across 1 file (changes from recent commits).
Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.
View guided diff | Re-trigger cubic
What
Before choosing a strategy,
partition_pdf()ran pdfminer over every page and built every element only to learn whether the PDF has any extractable text. Forhi_resandocr_onlythose elements were then discarded, andhi_resparsed the document again for its own text layer. For those strategies the check now stops at the first page that yields text.Process CPU, old full pass vs new check, same process:
Before
After
pdf_has_extractable_text()walks the same page generator as the full pass (same repair paths, password handling andPDFMinerConfig), builds each page with the same per-page builder, now factored out as_pdfminer_page_elements(), and returns at the first page with aTextelement with non-whitespace text.pdf_strategy_can_resolve_to_fast()instrategies.pydecides who still needs the full pass:fast,auto, andhi_resorocr_onlywhen missing dependencies would make them fall back tofast, sincefastreturns those elements.hi_resgains the same fallback the up-front pass always had: if pdfminer fails while extracting the text layer, it logs and continues without one instead of raising.Why
The boolean was all
hi_resused from that pass, andhi_reshas its own pdfminer pass for the merge, so the whole-document pre-pass was pure duplicate work, scaling with page count on exactly the long documents where it hurts most. Stopping at the first text page gives the same boolean for every PDF pdfminer reads without error: a parametrized test confirms this on 35 example PDFs, and the two long ones are covered by the check alone.The one case where the answer can differ is a PDF where a page after the first text page makes pdfminer raise. The old pass then reported "no text" and
hi_resran without the text layer; the new check reports "text" before reaching that page, andhi_res's own pass would hit it. The added fallback reproduces the old outcome there (text layer skipped, partition succeeds). It also means a text-layer failure that the old code would have raised throughhi_resis now logged and skipped, which is called out in the changelog.Relationship to other work
Touches the same
hi_restext-layer call sites as #4583 (per-page complexity guard); whichever lands second needs a small manual merge.Validation
hi_resandocr_onlycall the check once and never the full pass; the check reads exactly 1 page on a 16-page PDF and 4 when 3 blank pages precede the text; 35-PDF equivalence against the old computation including the exception-to-False path (password, repair, no-text PDFs);fastandautounchanged;hi_ressurvives a failing text-layer pass and matches the text-layer-skipped output when a later page fails; dependency-combination tests for the new strategy helper.test_pdf.py(full),test_pdfminer_processing.py,test_pdfminer_utils.py,test_strategies.py: 345 passed, 5 failed, 1 skipped. The 5 failures need Korean Tesseract data (kor.traineddata) and fail identically on unmodifiedmainin this environment.ruff checkandruff format --checkclean.