fix(eval): use unique image_rel as sample_id to stop silent dump_dir overwrites - #6
Merged
Merged
Conversation
OmniDocBench's page_info.page_no is the page number within a source PDF
(1, 2, 3, ...), not a unique identifier across the manifest. Picking it
first for sample_id meant every PDF's first page collapsed to
sample_id == '1', and per-sample diagnostic dumps written to
dump_dir/{sample_id}.json silently overwrote one another. With the
'tables' subset (~hundreds of rows), users got only one or two JSON
dumps instead of one per processed sample.
Reorder the priority to prefer image_rel (e.g. paper_xxx_page_001.png),
which is unique per row and carries both document and page info.
Add a regression test that processes 5 manifest rows sharing
page_no == 1 and asserts the dump_dir contains 5 distinct files.
Co-authored-by: Bartłomiej Rosa <bartrosa@users.noreply.github.com>
bartrosa
marked this pull request as ready for review
May 23, 2026 12:45
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.
Bug and impact
Running
bigos eval --benchmark=omnidocbench --dump-dir=...silently dropped most per-sample diagnostic JSON dumps. With thetablessubset (~hundreds of pages spread across many PDFs), users would see only one or two*.jsonfiles indump_dir/instead of one per processed sample. The aggregate report numbers were correct, but the per-sample diagnostic data — used to inspect failing pages, comparepred_previewvsgt_preview, etc. — was lost.Root cause
In
bigos.eval.omnidocbench.evaluate,sample_idwas built from:page_info.page_nois the page number within a source PDF (1, 2, 3, …) and is not unique across manifest rows: every multi-page document starts at page 1, so pickingpage_nofirst meant every "first page" sample collapsed tosample_id == "1". The dump path is computed asdump_dir / f"{_safe_dump_stem(sample_id)}.json", so all those samples wrote todump_dir/1.jsonand silently overwrote each other.I reproduced this with 5 manifest rows sharing
page_no=1: the eval reported 5 samples, but only 1 dump file was written (the last one).Fix and validation
sample_idpriority soimage_rel(e.g.paper_2401.05459_page_001.png) — which is unique per row and carries both document and page info — is preferred overpage_no.page_id(which would also be unique if present) keeps top priority.test_dump_dir_writes_one_file_per_sample) that processes 5 rows sharingpage_no=1and assertsdump_dircontains 5 distinct files and 5 distinct sample_ids.Validation:
ruff checkclean.doc0_page1.png.json, …).No public API change; affects only the value used for
result.sample_idand the per-sample dump filename.