From 2fdb2bb8b5a4da1cbacbb4699f29a7c05945fce4 Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Sat, 9 May 2026 22:10:24 +0000 Subject: [PATCH] fix(eval): use unique image_rel as sample_id to stop dump_dir overwrites MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- src/bigos/eval/omnidocbench.py | 10 +++++- tests/test_eval_omnidocbench.py | 60 +++++++++++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 1 deletion(-) diff --git a/src/bigos/eval/omnidocbench.py b/src/bigos/eval/omnidocbench.py index c50d39d..1af6b19 100644 --- a/src/bigos/eval/omnidocbench.py +++ b/src/bigos/eval/omnidocbench.py @@ -685,8 +685,16 @@ async def evaluate( if not image_rel: continue + # ``page_no`` is 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 picking ``page_no`` first means every "first page" sample + # collapses to ``sample_id == "1"``. Per-sample dump JSON files written to + # ``dump_dir/{sample_id}.json`` then silently overwrite each other. + # ``image_rel`` (the per-page image filename, e.g. + # ``paper_2401.05459_page_001.png``) is unique per row and carries both + # document and page info, so prefer it. sample_id = str( - page_info.get("page_no") or row.get("page_id") or image_rel or f"row_{idx}", + row.get("page_id") or image_rel or page_info.get("page_no") or f"row_{idx}", ) result = SampleResult(sample_id=sample_id, subset=subset) diff --git a/tests/test_eval_omnidocbench.py b/tests/test_eval_omnidocbench.py index ba0daac..82336b3 100644 --- a/tests/test_eval_omnidocbench.py +++ b/tests/test_eval_omnidocbench.py @@ -196,6 +196,66 @@ def test_truncated_merge_joins_text() -> None: assert "hello" in gt_markdown_json2md(row) +def test_dump_dir_writes_one_file_per_sample( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Regression: rows that share ``page_no`` must not collapse to one dump file. + + OmniDocBench's ``page_info.page_no`` is the page number within a source PDF + (1, 2, 3, ...) and repeats across documents. If ``sample_id`` falls back to + ``page_no`` first, every PDF's "page 1" sample writes to the same + ``dump_dir/1.json`` and silently overwrites all earlier dumps. + """ + png_bytes = ( + b"\x89PNG\r\n\x1a\n\x00\x00\x00\rIHDR\x00\x00\x00\x01\x00\x00\x00\x01" + b"\x08\x02\x00\x00\x00\x90wS\xde\x00\x00\x00\x0cIDATx\x9cc```\x00" + b"\x00\x00\x04\x00\x01\x0c\x8c\x10\x0c\x00\x00\x00\x00IEND\xaeB`\x82" + ) + + manifest = [ + { + "page_info": { + "page_no": 1, + "page_attribute": {"subset": "table_hard"}, + "image_path": f"doc{i}_page1.png", + }, + "layout_dets": [], + } + for i in range(5) + ] + + def fake_hub_download( + repo_id: str, + filename: str, + repo_type: str | None = None, + ) -> str: + if filename == omni._MANIFEST_NAME: + p = tmp_path / omni._MANIFEST_NAME + p.write_text(json.dumps(manifest), encoding="utf-8") + return str(p) + if filename.startswith("images/"): + stem = Path(filename).stem + f = tmp_path / f"{stem}.png" + # Vary file content so each image hashes uniquely (defensive). + f.write_bytes(png_bytes + stem.encode()) + return str(f) + raise AssertionError(f"unexpected filename: {filename}") + + monkeypatch.setattr(omni, "hf_hub_download", fake_hub_download) + + dump_dir = tmp_path / "dump" + report = asyncio.run( + omni.evaluate(_MockBackend(), subset="tables", max_samples=10, dump_dir=dump_dir), + ) + + assert report.n_samples == 5 + sample_ids = [r.sample_id for r in report.results] + assert len(set(sample_ids)) == 5, f"sample_ids collide: {sample_ids}" + dump_files = sorted(p.name for p in dump_dir.iterdir()) + assert len(dump_files) == 5, f"dump files lost to overwrite: {dump_files}" + + def test_eval_report_to_markdown_and_json_dict() -> None: r = SampleResult( sample_id="p1",