Skip to content

fix(reading-order): align provenance spans after dehyphenation - #4222

Draft
teachershuang wants to merge 1 commit into
docling-project:mainfrom
teachershuang:hongkaigu/fix-reading-order-charspans
Draft

teachershuang wants to merge 1 commit into
docling-project:mainfrom
teachershuang:hongkaigu/fix-reading-order-charspans

Conversation

@teachershuang

@teachershuang teachershuang commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Compute the appended provenance span after merging text, and trim the preceding span when its trailing hyphen is removed. This fixes out-of-range/overlapping spans without changing dehyphenation decisions, text/orig output, whitespace, page/bbox attribution or hyperlink handling.

Resolves #4217

This deliberately leaves the merge-policy changes in #4180, #4211 and #4155 out of scope.

Validation

  • Baseline 5ea6490: the final regression module has 13 failures and 9 passing controls, including actual three-page ReadingOrderModel integrations.
  • Fixed: 96 passed, 1 skipped across reading-order, page-assembly and list-marker tests. Covers ordinary/dehyphenated merges, consecutive merges, empty text/provenance, hyperlinks and JSON serialization.
  • All 4 added executable statements covered; no uncovered branch in _merge_elements.
  • Changed-file validation hooks passed; the two python3 hooks were run with the equivalent project-environment python command on Windows. Full Ruff format/lint, ty (exit 0, warnings), Tach, dprint and uv lock --locked passed. Full max-lines check still flags seven unchanged baseline files; changed files pass.
  • PDF e2e: 13/15 fixtures passed. The same five reference checks on two PDFs fail on both untouched baseline runtime and patched runtime. No reference data changed. One patched run additionally logged a native Windows access violation in docling-parse; the test continued. Keeping Draft for upstream CI/platform verification.
  • Full repository test suite and all optional backends were not verified. Optional tesserocr could not build without native Tesseract libraries; this uses the locked standard + dev environment, Python 3.12.13.

Implementation/tests were AI-assisted. Independent Codex diff review found no actionable issues; that review's sandbox could not run tests, so the tests above were run separately.

Checklist:

  • Documentation reviewed; no API/configuration changes requiring updates.
  • Examples reviewed; existing usage unchanged.
  • Regression and integration tests added.

Upstream CI update (2026-09-14)

At unchanged head 2ebce75128e103cb6a3769d50b35c288f997f60a, Run CI and Docs CI completed successfully after workflow approval. Core: 1065 passed / 10 skipped, including the reading-order regression integrations. PDF-model: 21 passed, including test_e2e_pdfs_conversions. OCR, VLM, ASR, light examples, lint, package and docs jobs also succeeded. Core and PDF-model coverage uploads completed; the final Codecov patch check passed after the core upload, with all modified coverable lines covered.

Windows/macOS and the two optional installation jobs were skipped by this workflow. These Linux CI results do not resolve the previously disclosed local Windows reference differences/native warning or establish all-platform coverage. Keeping Draft pending the outstanding maintainer convention decision on zero-length provenance; related #4180/#4211/#4155 remain unmerged. No code changes in this update.

Signed-off-by: teachershuang <guhongkaiailaopo@gmail.com>
@github-actions

Copy link
Copy Markdown
Contributor

✅ DCO Check Passed

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

@mergify

mergify Bot commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor

Merge Protections

🟢 Merge protection satisfied — ready to merge.

Show 1 satisfied protection

🟢 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)(?:\(.+\))?(!)?:

Copy link
Copy Markdown

Thanks for taking this on — the focused scope looks right to me. Keeping provenance alignment separate from the merge-policy changes in #4180, #4211, and #4155 makes sense.

Computing the appended span from the final merged text and adjusting the preceding span when its trailing hyphen is removed addresses the reported issue without changing dehyphenation decisions.

One convention worth confirming with the maintainers: the new empty-input tests allow zero-length spans, whereas my issue used start < end. If zero-length provenance is intentional, that wording should be clarified rather than treating those cases as invalid.

Please also coordinate with #4180 and #4211, since they touch the same merge logic, and retain provenance assertions for all resulting merge paths—including preserved hyphens without an inserted space. #4155 extends the affected element types, so coverage for section headers and list items would also be useful.

I’m happy to leave the merge order to the maintainers. Thanks again for the fix and regression coverage!

@teachershuang

Copy link
Copy Markdown
Contributor Author

Thanks @dexus-ai. I rechecked this against current main and reran your original script on 2ebce75: all three cases are within bounds. The six related test modules give 96 passed / 1 skipped; _merge_elements has no uncovered executable lines or branches in that run.

The empty-input cases deliberately preserve page attribution without inventing characters: a span may be empty if the contribution is empty or consists only of a removed soft hyphen. This is a boundary-case choice, not a claim that maintainers have approved a general convention. Could maintainers confirm whether retaining those empty spans is preferred over dropping their provenance? Nonempty contributions in the reported examples still satisfy start < end.

I also reread #4180, #4211 and #4155. The no-space semantic-hyphen path belongs to unmerged #4211; current main's predictor only selects TEXT continuations, while #4155 expands that eligibility. The direct merge tests already use real list items, but that is not end-to-end coverage of the proposed expanded eligibility. I have not pulled those policy changes into this PR. Whichever merge order maintainers choose, the resulting paths will need provenance assertions, especially the no-space branch.

No code change in this follow-up. Full CI/docs still have zero jobs and await workflow approval; the previously disclosed PDF baseline failures and Windows native warning remain unverified upstream. Keeping this Draft.

Copy link
Copy Markdown

Thanks @teachershuang for rerunning the reproducer and clarifying the edge cases. Preserving page attribution for empty contributions makes sense as a rationale; I’m happy to leave the intended zero-length span convention to the maintainers.

Thanks also for checking the related PRs and clearly separating the verified results from the remaining upstream checks. That addresses my questions for now. We haven’t yet validated the patch against our own PDFs, so this is not an end-to-end confirmation from our side.

@codecov

codecov Bot commented Sep 14, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

This branch has not been deployed

No deployments
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.

ReadingOrderModel produces invalid ProvenanceItem charspans when dehyphenating merged text elements

2 participants