Skip to content

Carry a mixed document's layout rules on ParseTestCase - #154

Merged
boyang-zhang1 merged 2 commits into
boyang/multi-task-eval-hardeningfrom
boyang/jsonl-mixed-rules
Sep 14, 2026
Merged

boyang-zhang1 merged 2 commits into
boyang/multi-task-eval-hardeningfrom
boyang/jsonl-mixed-rules

Conversation

@boyang-zhang1

@boyang-zhang1 boyang-zhang1 commented Sep 14, 2026

Copy link
Copy Markdown
Member

Builds on #133 — that PR's commit by @AmirF194 is the first commit here, so its diagnosis, fix and regression test are preserved. Stacked on top of #153, which hardens the evaluation path these documents now reach.

#133 correctly identified that _load_jsonl_dataset dropped the layout rules of a document that also carries parse rules, while the run still reported success: true. It routed those documents to LayoutDetectionTestCase. This commit keeps them on ParseTestCase instead, because that class already type-routes layout, extract_field and parse rules on one test_rules list and is the only branch that carries the parse-side state.

Why the container matters

LayoutDetectionTestCase ParseTestCase
expected_markdown no such field carried
allow_splitting_ambiguous_merged_tables, trm_unsupported, max_top_title_rows no such fields carried
test_rules type closed union of the built-in rule models SerializeAsAny[ParseRuleBase]

Three concrete consequences, all reproduced against main + #133:

  1. Markdown and table settings vanish. Same JSONL document (layout rule + table rule with settings + inline expected_markdown): ParseTestCase | md='| a |' trm=True split=True maxtop=3 becomes LayoutDetectionTestCase | md=<no field> trm=<no field> split=<no field> maxtop=<no field>. data/table.jsonl is entirely expected_markdown-driven, so one layout annotation on a table document would have erased its headline metric with no error.
  2. Extension and extract_field rules crash the load. A rule registered through parse_bench.extensions.register_rule_type validates fine on ParseTestCase and raises ValidationError: 63 validation errors on LayoutDetectionTestCase; an extract_field rule raises too. The exception propagates out of _load_jsonl_dataset, aborting the whole run including unrelated documents.
  3. Product-type detection flips for a whole run. _detect_product_type (inference/cli.py) inspects only test_cases[0] of the test-id-sorted list and runs before --group filtering. With a mixed chart/... document sorting first, a llamaparse pipeline's product type is overridden to layout_detection for all five categories.

The class choice buys nothing at evaluation time: _has_mixed_rules accepts both classes and _evaluate_multi_task is typed LayoutDetectionTestCase | ParseTestCase, splitting the rules by rule type, not by test-case class. The layout half is scored either way.

Deliberately not changed

A document whose only parse-side ground truth is an expected_markdown row stays on the layout branch and still loses that markdown — a pre-existing gap. Moving it to ParseTestCase to keep the markdown would be a worse trade: _has_mixed_rules looks for a non-layout rule, finds none, and the document would lose its layout scoring instead. Fixing it properly means teaching _has_mixed_rules about markdown GT, which belongs in its own change.

Two known gaps this does not close, both pre-existing and both only reachable with mixed documents: analysis/comparison.py::_get_gt_annotations accepts only LayoutDetectionTestCase, so a mixed document loses its GT overlay in comparison output; and an extract_field rule on a ParseTestCase is carried but not scored, because ParseEvaluator filters it out.

Scope

The shipped dataset has no document mixing layout with anything else (data/layout.jsonl is 100% type: "layout", and the group key is (category, pdf)), so data/ and data/test/ load identically before and after — verified. No current leaderboard number moves.

Tests

tests/parse_bench/test_cases/test_loader.py keeps @AmirF194's cases and asserts the invariant that matters — no rule of either kind is dropped — rather than the container class. Added: markdown + table settings survive a mixed document, the expected_markdown-only shape stays layout-shaped, and a mixed document with an extract_field rule loads (with an explicit assertion that the same payload is a ValidationError on the other class — the test is about load-time acceptance, not scoring). ruff check, ruff format --check and the test_cases + evaluation + analysis suites (2593 tests) are clean.

@boyang-zhang1
boyang-zhang1 force-pushed the boyang/jsonl-mixed-rules branch 2 times, most recently from b0bdcf9 to 64f48ce Compare September 14, 2026 17:01
@boyang-zhang1
boyang-zhang1 changed the base branch from main to boyang/multi-task-eval-hardening September 14, 2026 17:01
Builds on the previous commit, which stopped `_load_jsonl_dataset` from
dropping the layout rules of a document that also has parse rules. It routed
those documents to `LayoutDetectionTestCase`, which costs more than it buys:

- `LayoutDetectionTestCase` has no `expected_markdown`,
  `allow_splitting_ambiguous_merged_tables`, `trm_unsupported` or
  `max_top_title_rows` field, so the markdown ground truth that drives text
  similarity / TEDS / GriTS and the table settings were silently dropped
  instead. `data/table.jsonl` is entirely `expected_markdown`-driven, so one
  layout annotation on a table document would have erased its headline metric.
- Its `test_rules` is a closed union of the built-in rule models, while
  `ParseTestCase.test_rules` is `SerializeAsAny[ParseRuleBase]`. A rule
  registered through `parse_bench.extensions` or an `extract_field` rule fails
  validation there, and the exception aborts the whole dataset load.
- `_detect_product_type` reads only `test_cases[0]` of the test-id-sorted list,
  so a mixed document sorting first flipped the auto-detected product type for
  an entire run.

`ParseTestCase` already type-routes layout, extract_field and parse rules on
one `test_rules` list, and the evaluation runner splits them back out by rule
type rather than by test-case class, so the layout half is still scored.

A document whose only parse-side ground truth is an `expected_markdown` row
deliberately stays on the layout branch: `_has_mixed_rules` looks for a
non-layout *rule* and would find none, so moving it would trade a dropped
markdown for dropped layout scoring.

The shipped dataset has no document mixing layout with anything else, so
`data/` and `data/test/` load identically before and after.
Comment thread src/parse_bench/test_cases/loader.py Outdated
Comment on lines +267 to +278
# Anything with parse-side ground truth — parse rules, markdown, or
# both — loads as a ParseTestCase carrying *all* of its rules.
# ``ParseTestCase.test_rules`` type-routes each entry (layout,
# extract_field, parse) through ``_coerce_mixed_rule_list``, and it
# is the only branch that carries ``expected_markdown`` and the
# table settings. Routing a mixed document to
# ``LayoutDetectionTestCase`` instead would keep the layout rules
# but silently drop those, and its rule union is closed, so an
# extension or extract_field rule would fail validation outright.
# The evaluation runner splits the rules by type
# (``_has_mixed_rules`` -> ``_evaluate_multi_task``), so the layout
# half is still scored from here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is pretty hard to read lol

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we shorten to 1-3 lines?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cut to 3 lines each (dc58534), and trimmed the test docstrings the same way. No code change.

Review feedback: the two branch comments in `_load_jsonl_dataset` ran 7 and
13 lines. Cut both to 3, and trim the test docstrings to match. No code change.
@boyang-zhang1
boyang-zhang1 merged commit 8a84d81 into fix/131-layout-order-rules-dropped Sep 14, 2026
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants