fix(pdf): keep failed-page sizes on the conversion, not on the pipeline - #4486
Open
RizgarOzan wants to merge 1 commit into
Open
RizgarOzan wants to merge 1 commit into
RizgarOzan wants to merge 1 commit into
Conversation
StandardPdfPipeline stored the page sizes it records for failed pages in an instance dict and reset it at the start and end of every _build_document() call. DocumentConverter reuses one pipeline instance for every conversion, so two concurrent convert() calls on the same converter shared and wiped each other's sizes, and a failed page could end up with another document's size or with Size(0, 0). Keep the dict on the ConversionResult as a private attribute, next to _pdf_outline which already follows that lifecycle. Resolves docling-project#4478 Signed-off-by: Rızgar Ozan <rizgarozan7@gmail.com>
Contributor
|
✅ DCO Check Passed Thanks @RizgarOzan, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🟢 Merge protection satisfied — ready to merge. Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
This branch has not been deployed
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
2 participants
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.
Issue resolved by this Pull Request:
Resolves #4478
StandardPdfPipelinekept_page_sizes_by_noon the pipeline instance and reset it at the start and end of every_build_document().DocumentConverterreuses one pipeline instance per(pipeline_cls, options), so two concurrentconvert()calls on the same converter shared that dict: the second conversion wiped the first one's sizes, and a failed page could get another document's size orSize(0, 0).The dict now lives on the
ConversionResultas a private attribute, next to_pdf_outline, which already follows the same build → assemble lifecycle. The pipeline writes toconv_res._page_sizes_by_nowhile producing pages and reads it back in_add_failed_pages_to_document; the instance attribute and its two resets are gone. No behaviour change for a single conversion.How I tested
tests/test_pdf_streaming_foundation.py::test_failed_page_sizes_are_kept_per_conversionbuilds two conversions on one bare pipeline (the file's existing synthetic backend, no models), then adds the first one's missing pages. Onmainpages 2–4 come back asSize(0, 0); with this change all four keep100×200.pytest tests/test_pdf_streaming_foundation.py tests/test_failed_pages.py: 8 passed, 4 skipped (the skips are the existing fixture-less tests).ruff check/ruff format --checkclean,tach checkOK,ty checkon the two touched modules reports the same 18 pre-existing warnings asmainand no errors.Checklist: