Skip to content

fix(mets-gbs): page missing image/coordOCR metadata crashes instead of skipping - #3930

Merged
dolfim-ibm merged 1 commit into
docling-project:mainfrom
mittalpk:fix/mets-gbs-missing-page-metadata
Aug 11, 2026
Merged

dolfim-ibm merged 1 commit into
docling-project:mainfrom
mittalpk:fix/mets-gbs-missing-page-metadata

Conversation

@mittalpk

@mittalpk mittalpk commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

What's wrong

A page's fileGrp entries in the METS XML are independently optional in the backend's own data model (_PageFiles.image/ocr/coordOCR all default to None) — a page can legitimately have no image or coordOCR fptr, e.g. a blank/cover page with no OCR layer.

is_valid()/page_count() never check for this, so such a document reports as valid and loadable, then load_page() crashes with a bare AssertionError:

def _parse_page(self, page_no: int) -> Tuple[SegmentedPdfPage, PILImage]:
    # TODO: use better fallbacks...
    image_info = self.page_map[page_no].image
    assert image_info is not None
    ocr_info = self.page_map[page_no].coordOCR
    assert ocr_info is not None

— instead of the graceful per-page invalid state the class already supports (MetsGbsPageBackend.valid = parsed_page is not None). Under python -O, this assertion is stripped entirely and the code falls through to a different, more confusing crash further down instead.

Fix

_parse_page() now detects a missing image/coordOCR entry and returns (None, None) instead of asserting, so load_page() constructs an invalid MetsGbsPageBackend the same way it would for any other unparseable page — consistent with how the sibling DoclingParsePageBackend already handles a page it can't build (self.valid = (self._ppage is not None) and (self._dp_doc is not None), no crash).

MetsGbsPageBackend's accessor methods (get_text_in_rect, get_text_cells, get_bitmap_rects, get_page_image, get_size) now assert non-None internally, mirroring DoclingParsePageBackend's exact existing pattern — callers are expected to check is_valid() before using a page backend, same contract as the sibling class, and the downstream pipeline code (standard_pdf_pipeline.py, base_pipeline.py, etc.) already does this check for every backend.

How was this tested?

  • Reproduced the crash live against unpatched main first, using a new fixture derived from the existing real METS-GBS test archive (tests/data/mets_gbs/sources/32044009881525_select.tar.gz), with one page's coordOCR fileGrp entry and its underlying file removed to reproduce the missing-metadata case with real data rather than a synthetic minimal example.
  • Added test_page_missing_coordocr_is_skipped_not_crashed, confirmed it fails against unpatched mets_gbs_backend.py (git stash) with the exact AssertionError, and passes after the fix — pages with complete metadata still load normally, the page missing coordOCR is reported as is_valid() == False instead of crashing.
  • Full tests/test_backend_mets_gbs.py suite: 10 passed (9 existing + 1 new), no regressions.
  • ruff format --check / ruff check — clean.
  • ty check on the touched file — clean except one pre-existing, unrelated warning (has_textlines argument mismatch), confirmed present identically on unpatched main.

Checklist:

  • Documentation has been updated, if necessary. (No user-facing docs describe this error path; nothing to update.)
  • Examples have been added, if necessary.
  • Tests have been added, if necessary.

…f skipping

A page's fileGrp entries in the METS XML are independently optional
(_PageFiles.image/ocr/coordOCR all default to None) -- a page can
legitimately have no 'image' or 'coordOCR' fptr, e.g. a blank/cover
page with no OCR layer. is_valid()/page_count() never checked for
this, so such a document reported as valid and loadable, then
load_page() crashed with a bare AssertionError (or silently no-op'd
under python -O) instead of the graceful per-page invalid state the
class already supports (MetsGbsPageBackend.valid = parsed_page is not
None).

Fixed by having _parse_page() detect a missing image/coordOCR entry
and return (None, None) instead of asserting, so load_page()
constructs an invalid MetsGbsPageBackend the same way it would for any
other unparseable page -- consistent with how the sibling
DoclingParsePageBackend already handles a page it can't build
(self.valid = (self._ppage is not None) and (self._dp_doc is not
None), no crash). Updated MetsGbsPageBackend's accessor methods to
assert non-None internally (mirroring DoclingParsePageBackend's exact
pattern), since callers are expected to check is_valid() before using
a page backend, same contract as the sibling class.

Added a regression test using a new fixture derived from the existing
real METS-GBS test archive, with one page's coordOCR fileGrp entry and
file removed to reproduce the missing-metadata case with real data
rather than a synthetic minimal example.

Signed-off-by: Praveen Mittal <pkmittal28@gmail.com>
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

DCO Check Passed

Thanks @mittalpk, all your commits are properly signed off. 🎉

@mergify

mergify Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 All 2 merge protections satisfied — ready to merge.

Show 2 satisfied protections

🟢 Enforce conventional commit

Make sure that we follow https://www.conventionalcommits.org/en/v1.0.0/

  • title ~= ^(fix|feat|docs|style|refactor|perf|test|build|ci|chore|revert)(?:\(.+\))?(!)?:

🟢 Require two reviewer for test updates

When test data is updated, we require two reviewers

  • #approved-reviews-by >= 2

@PeterStaar-IBM
PeterStaar-IBM requested review from PeterStaar-IBM, cau-git, ceberam and dolfim-ibm and removed request for cau-git, ceberam and dolfim-ibm August 6, 2026 09:26

@PeterStaar-IBM PeterStaar-IBM left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm!

@codecov

codecov Bot commented Aug 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
docling/backend/mets_gbs_backend.py 90.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@dolfim-ibm
dolfim-ibm merged commit df18ae4 into docling-project:main Aug 11, 2026
26 checks passed
@ceberam

ceberam commented Aug 11, 2026

Copy link
Copy Markdown
Member

Just finished my review by the time it got merged, but still worth noting some observations, for a future improvement:

  • The PR introduces the same class of defect it described as the motivation for the fix. The description correctly identifies that assert is fragile under python -O. Assertions are stripped at the bytecode level so any guard written as an assert simply disappears in optimized mode). The original assert image_info is not None became a no-op and the code crashed later with a confusing error. We had a similar issue in docling-core (fix(markdown): remove assert statements to support Python optimization mode docling-core#548). However, the PR adds five more assert statements as the primary safety mechanism for the accessor methods:

     assert self._dpage is not None, "Page backend is invalid or was unloaded."

    These are in the same category as the ones it just removed. Under python -O they disappear, and the code crashes later with a confusing AttributeError, the same scenario the PR explicitly called out as bad behavior.

  • In my opinion, we should change unload() to set attributes to None instead of delattr, matching the pattern of DoclingParsePageBackend. This is a small pre-existing issue, but the PR explicitly claims to mirror DoclingParsePageBackend's pattern.

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.

4 participants