← 输入框|github.com|刷新
Skip to content

perf(pdf): hi_res read every page with pdfminer just to learn whether the PDF has text - #4588

Merged
cragwolfe merged 3 commits into
mainfrom
perf/hi-res-text-presence-check
Oct 24, 2026
Merged

cragwolfe merged 3 commits into
mainfrom
perf/hi-res-text-presence-check

Conversation

@CyMule

@CyMule CyMule commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

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. For hi_res and ocr_only those elements were then discarded, and hi_res parsed 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

layout-parser-paper.pdf (16 pages):        1.21 s
multi-column.pdf (13 pages):               1.76 s
400-page test PDF:                        14.27 s
DA-619p.pdf (619 pages):                  51.4  s

After

layout-parser-paper.pdf:                   0.034 s   (36x)
multi-column.pdf:                          0.042 s   (42x)
400-page test PDF:                         0.028 s   (514x)
DA-619p.pdf:                               0.22  s   (236x)
  • pdf_has_extractable_text() walks the same page generator as the full pass (same repair paths, password handling and PDFMinerConfig), builds each page with the same per-page builder, now factored out as _pdfminer_page_elements(), and returns at the first page with a Text element with non-whitespace text.
  • pdf_strategy_can_resolve_to_fast() in strategies.py decides who still needs the full pass: fast, auto, and hi_res or ocr_only when missing dependencies would make them fall back to fast, since fast returns those elements.
  • hi_res gains 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_res used from that pass, and hi_res has 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_res ran without the text layer; the new check reports "text" before reaching that page, and hi_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 through hi_res is now logged and skipped, which is called out in the changelog.

Relationship to other work

Touches the same hi_res text-layer call sites as #4583 (per-page complexity guard); whichever lands second needs a small manual merge.

Validation

  • New tests: hi_res and ocr_only call 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); fast and auto unchanged; hi_res survives 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 unmodified main in this environment. ruff check and ruff format --check clean.

View guided diff

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

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 cragwolfe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

@cragwolfe
cragwolfe self-requested a review October 24, 2026 01:50

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@cragwolfe
cragwolfe added this pull request to the merge queue Oct 24, 2026
Merged via the queue into main with commit 3357eed Oct 24, 2026
55 checks passed
@cragwolfe
cragwolfe deleted the perf/hi-res-text-presence-check branch October 24, 2026 03:25
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.

2 participants