Repository navigation
Conversation
|
✅ DCO Check Passed Thanks @CyberRik, all your commits are properly signed off. 🎉 |
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/
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
@CyberRik the fix is right at 820ee59, but the branch now conflicts with #690, which ceberam merged on 10-19. Your head drops the empty chunk from #806 on my reproduction. Main b359e1c still has it, so the fix is still needed. You build units in one pass and serialize each item once, so #808's linear time holds. Its byte-identical promise goes, on purpose.
blocking: only hybrid_chunker.py conflicts. But #690 now writes the caption into the table header itself, as | Table 1: Revenue by region | in the first table chunk on main. After the rebase, could you check that the caption text appears once and not also as its own unit?
non-blocking: when the table splits, every part lists the caption in doc_items, though only part 1 carries its text. The table is on all parts already, so I'd keep that as it is.
$ python probe.py <tree> # table of 21 rows split at max_tokens=64, paragraph before and after, merge_peers=False
# base f61fbf5: 6 chunks. chunk 1 has doc_items [caption] and text ''. The caption text starts chunk 2.
# head 820ee59: 5 chunks, none empty. Parts 1, 2 and 3 list [caption, table]. Same with the caption after the table.
# main b359e1c: 7 chunks. chunk 1 has doc_items [caption] and text ''. chunk 2 opens '| - |\n| Table 1: Revenue by region |', chunk 5 is '| - |\n |\n'.
$ git merge-tree --write-tree origin/main 820ee594
CONFLICT (content): Merge conflict in docling_core/transforms/chunker/hybrid_chunker.py
$ python -m pytest tests/test_hybrid_chunker.py tests/test_hierarchical_chunker.py -q # head 820ee59, CPython 3.12
39 passed
# same command, hybrid_chunker.py from base f61fbf5, tests and 0d_out_chunks.json kept
5 failed, 34 passed # the 4 new test_table_caption_stays_with_its_table cases and test_chunk_with_repeat_table_header
$ ruff format --check && ruff check # ruff 0.16.5
2 files already formatted
All checks passed!
# not run: mypy, tests/test_code_chunker.py (needs a git clone of external fixtures)
@ceberam, mergify wants two approvals here because 0d_out_chunks.json changes. The squash title becomes the release note. Should it say that caption membership in doc_items moved, for people who index by them?
820ee59 to
a98c2d7
Compare
|
Thanks for the thorough review! Rebased onto main. The conflict was only in I ran your setup again after the rebase (21 rows, max_tokens=64, a paragraph on each side, merge_peers=False) and got 6 chunks, none of them empty. That's true with the caption either before or after the table. The caption text only shows up once, in the first table part, and it's never its own unit. I added that check to Agreed on keeping the caption in One thing I noticed while testing: the table parts from #690's column split still look a bit off. The first starts with |
chrikrah
left a comment
chrikrah
left a comment
There was a problem hiding this comment.
@CyberRik resolved: the caption appears once and the empty chunk is gone. [False-True] passes on main, so three cases fail there, not four, plus the intended repeat-header update. #851 now conflicts in tests/test_hybrid_chunker.py; once rebased, from my perspective this can be merged.
$ python -m pytest tests/test_hybrid_chunker.py tests/test_hierarchical_chunker.py -q # head a98c2d7, CPython 3.12.6, docling_core imported from the exported tree
61 passed, 35 warnings in 10.52s
# hybrid_chunker.py from main b501ec8, tests kept
FAILED tests/test_hybrid_chunker.py::test_chunk_with_repeat_table_header
FAILED tests/test_hybrid_chunker.py::test_table_caption_stays_with_its_table[True-True]
FAILED tests/test_hybrid_chunker.py::test_table_caption_stays_with_its_table[True-False]
FAILED tests/test_hybrid_chunker.py::test_table_caption_stays_with_its_table[False-False]
4 failed, 57 passed, 35 warnings in 10.86s
$ git merge-tree --write-tree origin/main a98c2d7 | grep CONFLICT # origin/main 5582916
CONFLICT (content): Merge conflict in tests/test_hybrid_chunker.py
$ ruff check && ruff format --check # ruff 0.13.1, the two changed .py files
All checks passed!
2 files already formatted
# not run: mypy, tests/test_code_chunker.py
Yes, file the #690 | - | rows separately; main has them too.
|
@CyberRik could you please resolve the existing conflict? |
A caption serializes to no text of its own, as its text is emitted with its table. _split_by_doc_items could still put it in a window by itself, which made an empty chunk (or a stray delimiter once merged), or in the window before the table, which listed it in the wrong chunk. Windows are now built from units that bind an item without text to the next item with text, and the repeat-header table path accepts a table together with its captions. Signed-off-by: Ritankar Mondal <[email protected]>
a98c2d7 to
2b0b902
Compare
|
@ceberam rebased onto main ( @chrikrah thanks for the approval. I'll file the |
github-actions
Bot
added
ai:ci-safe
|
@ceberam this one is approved by @chrikrah and all checks are green, the only thing holding it back is the "two reviewers for test updates" protection (currently at 1 of 2 approvals). The conflict you flagged earlier is resolved. Would you be able to give it a second review when you have a moment? Thanks! |
Fixes #806.
A caption serializes to no text of its own in a chunk, because its text is emitted with its table.
_split_by_doc_itemsstill treated it as a separate item, so:text=''(withmerge_peers=False), or, once merged with the next chunk, a stray leading\n;doc_itemsof the preceding paragraph's chunk instead of the table's, and that chunk got a trailing\n.Change. Windows are now built from units: a run of items without text of their own, plus the next item with text. Items without text at the end of a chunk join the last unit. Headings are bound the same way, since their text goes to the metadata. The window-growing logic itself is unchanged; it just walks units instead of items.
Because a table now arrives together with its caption, the
repeat_table_headerpath checks for a single table plus its own captions (_is_single_table) instead of exactly one item. That path already handles the caption text as the preamble.Ground truth.
tests/data/chunker/0d_out_chunks.jsonchanges in exactly this way on the real test document:#/texts/72, the caption of#/tables/0, moves from thedoc_itemsof chunk 18 (the paragraph before the table) to chunks 19 and 20 (the table). Chunk 18 also loses a trailing\nthat came from joining the caption's empty text; the text of every other chunk is unchanged.Tests.
test_table_caption_stays_with_its_table, parametrized over a preceding paragraph andmerge_peers, checks that no chunk is empty, no chunk starts with a delimiter before the caption, the caption is never listed without its table, and the chunk with the caption text lists the caption. All four cases fail onmainand pass with this change. The chunker test files pass (74 tests); ruff and mypy are clean on the changed files.