Repository navigation
fix(reading-order): align provenance spans after dehyphenation - #4222
teachershuang wants to merge 1 commit into
Conversation
Signed-off-by: teachershuang <guhongkaiailaopo@gmail.com>
|
✅ DCO Check Passed Thanks @teachershuang, all your commits are properly signed off. 🎉 |
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/
|
|
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 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! |
|
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; 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 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. |
|
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 Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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
5ea6490: the final regression module has 13 failures and 9 passing controls, including actual three-page ReadingOrderModel integrations._merge_elements.python3hooks were run with the equivalent project-environmentpythoncommand on Windows. Full Ruff format/lint, ty (exit 0, warnings), Tach, dprint anduv lock --lockedpassed. Full max-lines check still flags seven unchanged baseline files; changed files pass.docling-parse; the test continued. Keeping Draft for upstream CI/platform verification.tesserocrcould 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:
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, includingtest_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.