fix(serializer): resolve page-break markers when serializing a single node - #715
Open
Anai-Guo wants to merge 1 commit into
Open
fix(serializer): resolve page-break markers when serializing a single node#715Anai-Guo wants to merge 1 commit into
Anai-Guo wants to merge 1 commit into
Conversation
… node The internal `#_#_DOCLING_DOC_PAGE_BREAK_<prev>_<next>_#_#` sentinel is created in `DocSerializer.serialize()` but was only ever replaced in `serialize_doc()`, which the single-node path never reaches. A page break nested inside a group therefore leaked the raw sentinel out of `serializer.serialize(item=...)` and into `HierarchicalChunker` chunk text. Resolve the markers in `DocSerializer.serialize()` too, via a new `_replace_page_breaks()` hook that the Markdown, DocTags and LaTeX serializers override with their existing context-free replacement. The hook is a no-op in the base class, so HTML `SPLIT_PAGE` -- which needs the page numbers and the marker offsets, and can only resolve them once the whole document is available -- keeps the sentinel as before. Markers that sit between two top-level items are still resolved by `serialize_doc()`: `serialize()` skips the substitution when the node being serialized is itself the page break, so an empty `page_break_placeholder` keeps producing the same separators it does today. Fixes docling-project#714 Signed-off-by: Tai An <antai12232931@outlook.com>
Contributor
|
✅ DCO Check Passed Thanks @Anai-Guo, all your commits are properly signed off. 🎉 |
Contributor
Merge Protections🔴 1 of 2 protections blocking · waiting on 👀 reviews
🔴 Require two reviewer for test updatesWaiting for
This rule is failing.When test data is updated, we require two reviewers
Show 1 satisfied protection🟢 Enforce conventional commitMake sure that we follow https://www.conventionalcommits.org/en/v1.0.0/
|
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
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.
Fixes #714.
The bug
DocSerializer.serialize()emits an internal sentinel for each page break(
#_#_DOCLING_DOC_PAGE_BREAK_<prev>_<next>_#_#,common.py:481), and every serializer replaces it inserialize_doc(). Butserialize_doc()is only reached from the whole-document path, so a page breaknested inside a group leaked the raw sentinel out of the single-node entry point — and into
HierarchicalChunker, which buildschunk.textfromserialize(item=...):The fix
A new
DocSerializer._replace_page_breaks()hook, called fromserialize()as well. The baseimplementation returns the text unchanged; Markdown, DocTags and LaTeX override it with the
context-free replacement they already performed in
serialize_doc(), which now just calls the hook.Two details keep the existing behaviour intact:
HTMLOutputStyle.SPLIT_PAGEneedsprev_page/next_pageand each marker's offset to slice the body into pages, so it can only resolve them oncethe whole document is available. Its sentinel survives to
serialize_doc()exactly as before.serialize()skips the substitution when the node being serialized is itself the page break.Markers between two top-level items therefore still reach
serialize_doc()as their own part. Thatmatters for
page_break_placeholder="": resolving those markers early would make them empty parts,which the
if p.textjoin filters drop, silently removing the blank separators thattest_md_cross_page_list_page_break_emptypins down.LaTeX's
serialize_doc()resolves markers fromself.params.merge_with_patch(patch=kwargs)whilerequires_page_break()readsself.params; the override followsrequires_page_break()and usesself.params. A per-callpage_break_commandoverride is already only half-honoured today (whetherbreaks are emitted at all is decided from
self.params), so I left that as-is rather than change it here.Verification
test/test_serialization.py(Markdown, DocTags, LaTeX) serialize#/groups/2ofactivities.json— a list spanning pages 1–2 — on its own. All three fail onmainwith the raw sentinel and pass here.
test/data/doc/as Markdown(with
page_break_placeholderset and empty), DocTags, LaTeX and HTMLSPLIT_PAGE, before andafter. All 190 outputs are byte-identical.
pytest test/— same 9 pre-existing failures asmainin my environment (test_doctags_load,test_docling_doc; unrelated to serialization), 372 → 375 passed, the 3 being the new tests.ruff checkandruff format --checkclean with the repo config.🤖 Generated with Claude Code