Repository navigation
Skip the redundant pdfminer re-parse in PdfConverter - #2827
Open
Mohamed Emad (TheGeekyM) wants to merge 1 commit into
Open
Mohamed Emad (TheGeekyM) wants to merge 1 commit into
Mohamed Emad (TheGeekyM) wants to merge 1 commit into
Conversation
`PdfConverter.convert` could run `pdfminer.high_level.extract_text` twice on the same bytes in one conversion. This happened whenever the first pdfminer pass returned empty text. This PR records whether pdfminer has already run, and only uses the final "still empty" fallback when it has not. That is the case where pdfplumber found form-style pages but got no text from them.
Mohamed Emad (TheGeekyM)
marked this pull request as ready for review
November 4, 2026 07:31
|
Mohamed Emad (@TheGeekyM) please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
Contributor License AgreementContribution License AgreementThis Contribution License Agreement (“Agreement”) is agreed to by the party signing below (“You”),
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PdfConverter.convertcould runpdfminer.high_level.extract_texttwice on the same bytes in one conversion. This happened whenever the first pdfminer pass returned empty text. This PR records whether pdfminer has already run, and only uses the final "still empty" fallback when it has not. That is the case where pdfplumber found form-style pages but got no text from them.Motivation
convert(packages/markitdown/src/markitdown/converters/_pdf_converter.py) has three places that call pdfminer:form_page_count == 0, it runs pdfminer on the whole document, which gives better spacing for prose.exceptbranch runs pdfminer.if not markdown:runs pdfminer again.The last check did not know whether pdfminer had already run. If step 1 or step 2 returned an empty string, step 3 parsed the whole document again with the same library and the same input. pdfminer is deterministic, so the second pass always returned the same empty result. The only effect was a second full parse.
The most common case is a scanned or image-only PDF. pdfplumber finds no form pages, so step 1 runs pdfminer, gets no text, and step 3 runs it again. The repo's own fixture
MEDRPT-2024-PAT-3847_medical_report_scan.pdffollows this path. Anyone converting scanned documents paid for two full parses per file, and on large scans that cost is the whole pdfminer runtime a second time. Batch jobs and server use, where many scans go through one process, pay it on every file.What changed
packages/markitdown/src/markitdown/converters/_pdf_converter.pyused_pdfminer = False, set right after the input is buffered.Truejust before the pdfminer call in theform_page_count == 0branch and in theexceptbranch.if not markdown:toif not markdown and not used_pdfminer:and updated the comment. The fallback now only runs when pdfplumber found form-style pages and every chunk came back empty. That is the one case where pdfminer has not been tried yet and could still return text.packages/markitdown/tests/test_pdf.pyTestPdfminerFallbackNotRepeated, which covers each path into pdfminer and checks it runs exactly once (details under Tests).How it works
pdfminer can be reached in two ways: as the main extractor (no form pages, or pdfplumber failed) or as a last resort (form pages found but empty). The flag tells the final check which way the code went. If pdfminer was the main extractor, its result is final, even an empty one. If it was not, the last-resort call works as before.
The flag is set before the call, not after it. If pdfminer raises inside the
tryblock, theexceptbranch still runs and sets the flag itself, so the final check always sees the right state.Alternatives I considered:
_merge_partial_numbering_lines, or require calling it in several places. A single flag keeps one exit path and one post-processing step.page.close()memory behaviour covered byTestPdfMemoryOptimization. I kept this change small so it is easy to review.if not markdown and form_page_count > 0.form_page_countis not defined ifpdfplumber.openitself raises, so this would need extra guarding. The explicit flag is clearer.Tests
All new cases are in
packages/markitdown/tests/test_pdf.py, in a new classTestPdfminerFallbackNotRepeated. It sits next toTestPdfMemoryOptimizationand reuses itspdf_activityfixture and_convert_cleanup_fixturehelper.test_scanned_pdf_runs_pdfminer_once: converts the real scanned medical-report fixture. It checks that the output is empty, that no page was classified as a form, and thatpdf_activity.count(("pdfminer", None)) == 1. This is the real-world case that motivated the change.test_plain_pdf_with_empty_pdfminer_result_not_retried: patchesextract_textto return""onpdf_cleanup_plain.pdfand checks it was called exactly once and the result is"". This covers theform_page_count == 0branch.test_pdfplumber_failure_with_empty_pdfminer_result_not_retried: makespdfplumber.openraise andextract_textreturn""onpdf_cleanup_form.pdf. It checks there is one call and the result is empty. This covers theexceptbranch.test_pdfplumber_failure_uses_pdfminer_text: makespdfplumber.openraise, with the real pdfminer. It checks that the real text comes through and pdfminer ran once, so the exception fallback still produces output.test_empty_form_content_still_falls_back_to_pdfminer: patches_extract_form_content_from_wordsto return"", so every page counts as a form page with no content. It checks that the final fallback still runs pdfminer once and recovers all three copies of the expected text. This protects the one path where the last-resort call is still needed.To run them:
The tests use pytest fixtures (
monkeypatch,pdf_activity), sohatch test tests/test_pdf.pyfrompackages/markitdownis the intended runner.Compatibility & risk
convert.test_empty_form_content_still_falls_back_to_pdfminer). One related case is out of scope and unchanged: if pdfminer itself raises inside theform_page_count == 0branch, theexceptbranch still calls it a second time. That is existing error-handling behaviour, not the empty-result retry this PR fixes.The tests are in a new
TestPdfminerFallbackNotRepeatedclass, not insideTestPdfMemoryOptimizationas the original idea said, so the description lists them that way. I haven't run the tests or pre-commit, so the description doesn't claim they pass.