fix: add failed pages to DoclingDocument for page break consistency#2939
Conversation
|
✅ DCO Check Passed Thanks @jhchoi1182, all your commits are properly signed off. 🎉 |
Merge ProtectionsYour pull request matches the following merge protections and will not be merged until they are valid. 🟢 Enforce conventional commitWonderful, this rule succeeded.Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
🟢 Require two reviewer for test updatesWonderful, this rule succeeded.When test data is updated, we require two reviewers
|
There was a problem hiding this comment.
Pull request overview
Ensures PDFs with skipped/failed pages still preserve correct page numbering in DoclingDocument.pages, so downstream exports can emit consistent page-break markers.
Changes:
- Add
_add_failed_pages_to_document()toStandardPdfPipelineto insert placeholderPageItems for missing pages (with best-effort size info). - Invoke the new helper during document assembly to backfill missing page entries.
- Add pytest coverage validating page presence and size info for skipped-page PDFs.
Reviewed changes
Copilot reviewed 2 out of 5 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
docling/pipeline/standard_pdf_pipeline.py |
Adds logic to backfill missing/failed pages into DoclingDocument.pages and updates some typing annotations. |
tests/test_failed_pages.py |
Adds tests asserting failed/skipped pages are still represented in DoclingDocument.pages with size info. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Hi @cau-git, just checking in on this PR. Is there anything else needed from my end to move this forward? Let me know if you need any changes. |
cau-git
left a comment
There was a problem hiding this comment.
@jhchoi1182 Thanks for this contribution! Your changes pass tests and look legitimate.
59590a2
|
Hi @PeterStaar-IBM @cau-git, apologies for the dismissal. I just pushed a small fix to the test case (checking for PARTIAL status) based on feedback. Could you please take a quick look again? |
|
The |
|
@jhchoi1182 thanks for the updates. Could you please rebase to |
When some PDF pages fail to parse, they were not added to DoclingDocument.pages, causing page break markers to be incorrect during export. This adds failed/skipped pages with their size info (if available) to maintain correct page numbering and structure. - Add _add_failed_pages_to_document() method in StandardPdfPipeline - Add test cases for failed page handling - Add test cases for normal page handling (regression test) - Add test PDF files Signed-off-by: jhchoi1182 <[email protected]>
- Wrap page_backend usage in try-finally to guarantee unload (prevents resource leaks). - Simplify redundant 'float | None | None' type hint. Signed-off-by: jhchoi1182 <[email protected]>
…rom e2e test Signed-off-by: jhchoi1182 <[email protected]>
Signed-off-by: jhchoi1182 <[email protected]>
64fbe4e to
a6b18a2
Compare
|
@cau-git |
…n models (#2999) * fix: add failed pages to DoclingDocument for page break consistency (#2939) * fix: add failed pages to DoclingDocument for page break consistency When some PDF pages fail to parse, they were not added to DoclingDocument.pages, causing page break markers to be incorrect during export. This adds failed/skipped pages with their size info (if available) to maintain correct page numbering and structure. - Add _add_failed_pages_to_document() method in StandardPdfPipeline - Add test cases for failed page handling - Add test cases for normal page handling (regression test) - Add test PDF files Signed-off-by: jhchoi1182 <[email protected]> * fix: ensure resource cleanup and simplify type hints - Wrap page_backend usage in try-finally to guarantee unload (prevents resource leaks). - Simplify redundant 'float | None | None' type hint. Signed-off-by: jhchoi1182 <[email protected]> * fix: add groundtruth for normal_4pages.pdf and exclude failing PDFs from e2e test Signed-off-by: jhchoi1182 <[email protected]> * fix: ensure correct status assertion for failed pages in tests Signed-off-by: jhchoi1182 <[email protected]> --------- Signed-off-by: jhchoi1182 <[email protected]> * fix: Use timezone-aware datetime (#2947) * Use timezone-aware datetime for profiling timestamps Updated timestamp recording to use timezone-aware datetime. Signed-off-by: Nikhil Singh <[email protected]> * run formatter Signed-off-by: Michele Dolfi <[email protected]> --------- Signed-off-by: Nikhil Singh <[email protected]> Signed-off-by: Michele Dolfi <[email protected]> Co-authored-by: Michele Dolfi <[email protected]> * fix(asciidoc): handle commas in image alt text (#2983) * Fix: Handle commas in AsciiDoc image alt text - Modified _parse_picture() to gracefully handle alt text containing commas - Commas in alt text are now preserved instead of causing ValueError - Added test case with realistic auto-generated alt text - split('=', 1) prevents issues when values contain '=' characters * DCO Remediation Commit for n0rdp0l <[email protected]> I, n0rdp0l <[email protected]>, hereby add my Signed-off-by to this commit: ee75249 Signed-off-by: n0rdp0l <[email protected]> * style: fix ruff formatting in test_backend_asciidoc.py Signed-off-by: n0rdp0l <[email protected]> --------- Signed-off-by: n0rdp0l <[email protected]> Co-authored-by: Michele Dolfi <[email protected]> * chore: bump version to 2.73.1 [skip ci] * First attempt at establishing API Kserve2 facet Signed-off-by: Christoph Auer <[email protected]> * refactor: improve KServe v2 engine implementation after code review - Add comprehensive error handling to KserveV2HttpClient - Catch and wrap Timeout, ConnectionError, HTTPError with context - Validate response formats with clear error messages - Refactor URL building to eliminate duplication - Extract _build_model_url() helper method - Single source of truth for infer_url and model_metadata_url - Make URL required parameter (remove default localhost:8000) - Update ApiKserveV2*EngineOptions to require explicit URL - Add preset validation with helpful error messages - Rename constants for clarity: TRITON_* → KSERVE_V2_* - Add comment explaining KServe v2 uses Triton type system - Improve error messages with actual values - Show counts, shapes, and supported types in validation errors - Document official KServe Python SDK alternative - Note async-only requirement and alpha status - Update tests for required URL parameter Signed-off-by: Christoph Auer <[email protected]> * Cleanup in kserve http helper and options Signed-off-by: Christoph Auer <[email protected]> * Further cleanup Signed-off-by: Christoph Auer <[email protected]> * Fix for remote-services on tablemodel Signed-off-by: Christoph Auer <[email protected]> * fix: improved deserialization of engine_options (#3008) * add registry of discriminated subclasses Signed-off-by: Michele Dolfi <[email protected]> * fix detection of engine_type value Signed-off-by: Michele Dolfi <[email protected]> --------- Signed-off-by: Michele Dolfi <[email protected]> * Add options serialization improvements Signed-off-by: Christoph Auer <[email protected]> --------- Signed-off-by: jhchoi1182 <[email protected]> Signed-off-by: Nikhil Singh <[email protected]> Signed-off-by: Michele Dolfi <[email protected]> Signed-off-by: n0rdp0l <[email protected]> Signed-off-by: Christoph Auer <[email protected]> Co-authored-by: jhchoi1182 <[email protected]> Co-authored-by: Nikhil Singh <[email protected]> Co-authored-by: Michele Dolfi <[email protected]> Co-authored-by: Felix Wente <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Michele Dolfi <[email protected]>
…ication) and KServe v2 API support (#2979) * feat: Inference engines abstraction for image classification model family with HF Transformers and ONNX runtime Implements runtime abstraction for image classification models with support for both ONNX Runtime and HuggingFace Transformers engines. Users can switch between engines without model retraining, similar to the object detection abstraction (#2959). Key components: - BaseImageClassificationEngine with factory pattern - OnnxRuntimeImageClassificationEngine and TransformersImageClassificationEngine implementations - Shared HfVisionModelMixin for common HF model utilities - Engine-specific configuration options - Test suite and example demonstrating runtime engine switching Signed-off-by: Christoph Auer <[email protected]> * Add missing files and re-export for backward compat Signed-off-by: Christoph Auer <[email protected]> * Don't run with OCR in the example. Signed-off-by: Christoph Auer <[email protected]> * Remove excess onnxruntime related options for inuts and outputs Signed-off-by: Christoph Auer <[email protected]> * feat: centralize torch compile defaults with DOCLING_INFERENCE_COMPILE_TORCH_MODELS Signed-off-by: Christoph Auer <[email protected]> * feat: Add Kserve2 API engine for image classifier and object detection models (#2999) * fix: add failed pages to DoclingDocument for page break consistency (#2939) * fix: add failed pages to DoclingDocument for page break consistency When some PDF pages fail to parse, they were not added to DoclingDocument.pages, causing page break markers to be incorrect during export. This adds failed/skipped pages with their size info (if available) to maintain correct page numbering and structure. - Add _add_failed_pages_to_document() method in StandardPdfPipeline - Add test cases for failed page handling - Add test cases for normal page handling (regression test) - Add test PDF files Signed-off-by: jhchoi1182 <[email protected]> * fix: ensure resource cleanup and simplify type hints - Wrap page_backend usage in try-finally to guarantee unload (prevents resource leaks). - Simplify redundant 'float | None | None' type hint. Signed-off-by: jhchoi1182 <[email protected]> * fix: add groundtruth for normal_4pages.pdf and exclude failing PDFs from e2e test Signed-off-by: jhchoi1182 <[email protected]> * fix: ensure correct status assertion for failed pages in tests Signed-off-by: jhchoi1182 <[email protected]> --------- Signed-off-by: jhchoi1182 <[email protected]> * fix: Use timezone-aware datetime (#2947) * Use timezone-aware datetime for profiling timestamps Updated timestamp recording to use timezone-aware datetime. Signed-off-by: Nikhil Singh <[email protected]> * run formatter Signed-off-by: Michele Dolfi <[email protected]> --------- Signed-off-by: Nikhil Singh <[email protected]> Signed-off-by: Michele Dolfi <[email protected]> Co-authored-by: Michele Dolfi <[email protected]> * fix(asciidoc): handle commas in image alt text (#2983) * Fix: Handle commas in AsciiDoc image alt text - Modified _parse_picture() to gracefully handle alt text containing commas - Commas in alt text are now preserved instead of causing ValueError - Added test case with realistic auto-generated alt text - split('=', 1) prevents issues when values contain '=' characters * DCO Remediation Commit for n0rdp0l <[email protected]> I, n0rdp0l <[email protected]>, hereby add my Signed-off-by to this commit: ee75249 Signed-off-by: n0rdp0l <[email protected]> * style: fix ruff formatting in test_backend_asciidoc.py Signed-off-by: n0rdp0l <[email protected]> --------- Signed-off-by: n0rdp0l <[email protected]> Co-authored-by: Michele Dolfi <[email protected]> * chore: bump version to 2.73.1 [skip ci] * First attempt at establishing API Kserve2 facet Signed-off-by: Christoph Auer <[email protected]> * refactor: improve KServe v2 engine implementation after code review - Add comprehensive error handling to KserveV2HttpClient - Catch and wrap Timeout, ConnectionError, HTTPError with context - Validate response formats with clear error messages - Refactor URL building to eliminate duplication - Extract _build_model_url() helper method - Single source of truth for infer_url and model_metadata_url - Make URL required parameter (remove default localhost:8000) - Update ApiKserveV2*EngineOptions to require explicit URL - Add preset validation with helpful error messages - Rename constants for clarity: TRITON_* → KSERVE_V2_* - Add comment explaining KServe v2 uses Triton type system - Improve error messages with actual values - Show counts, shapes, and supported types in validation errors - Document official KServe Python SDK alternative - Note async-only requirement and alpha status - Update tests for required URL parameter Signed-off-by: Christoph Auer <[email protected]> * Cleanup in kserve http helper and options Signed-off-by: Christoph Auer <[email protected]> * Further cleanup Signed-off-by: Christoph Auer <[email protected]> * Fix for remote-services on tablemodel Signed-off-by: Christoph Auer <[email protected]> * fix: improved deserialization of engine_options (#3008) * add registry of discriminated subclasses Signed-off-by: Michele Dolfi <[email protected]> * fix detection of engine_type value Signed-off-by: Michele Dolfi <[email protected]> --------- Signed-off-by: Michele Dolfi <[email protected]> * Add options serialization improvements Signed-off-by: Christoph Auer <[email protected]> --------- Signed-off-by: jhchoi1182 <[email protected]> Signed-off-by: Nikhil Singh <[email protected]> Signed-off-by: Michele Dolfi <[email protected]> Signed-off-by: n0rdp0l <[email protected]> Signed-off-by: Christoph Auer <[email protected]> Co-authored-by: jhchoi1182 <[email protected]> Co-authored-by: Nikhil Singh <[email protected]> Co-authored-by: Michele Dolfi <[email protected]> Co-authored-by: Felix Wente <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Michele Dolfi <[email protected]> * Fixes from review Signed-off-by: Christoph Auer <[email protected]> * DCO Remediation Commit for Christoph Auer <[email protected]> I, Christoph Auer <[email protected]>, hereby add my Signed-off-by to this commit: 4cdb01e Signed-off-by: Christoph Auer <[email protected]> * DCO Remediation Commit for Christoph Auer <[email protected]> I, Christoph Auer <[email protected]>, hereby add my Signed-off-by to this commit: e293ba3 Signed-off-by: Christoph Auer <[email protected]> Signed-off-by: Christoph Auer <[email protected]> * Add fallback for API variants Signed-off-by: Christoph Auer <[email protected]> * Recreate uv.lock Signed-off-by: Christoph Auer <[email protected]> --------- Signed-off-by: Christoph Auer <[email protected]> Signed-off-by: jhchoi1182 <[email protected]> Signed-off-by: Nikhil Singh <[email protected]> Signed-off-by: Michele Dolfi <[email protected]> Signed-off-by: n0rdp0l <[email protected]> Signed-off-by: Christoph Auer <[email protected]> Co-authored-by: jhchoi1182 <[email protected]> Co-authored-by: Nikhil Singh <[email protected]> Co-authored-by: Michele Dolfi <[email protected]> Co-authored-by: Felix Wente <[email protected]> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Michele Dolfi <[email protected]>
Description
When some PDF pages fail to parse, they were not added to
DoclingDocument.pages, causing page break markers to be incorrect during export.This PR adds failed/skipped pages with their size info (if available from the backend) to maintain correct page numbering and structure in the final document.
Changes
_add_failed_pages_to_document()method inStandardPdfPipelineDoclingDocument.pageswith:page_no)0.0 x 0.0)PageItem)<page_break>markers even when some pages failRelated Issue
This change is required to properly support the page break marker fix in docling-core:
Without this fix,
DoclingDocument.pageswould be missing failed pages, causing the page break logic in docling-core to produce incorrect results.Checklist: