Carry a mixed document's layout rules on ParseTestCase - #154
Merged
boyang-zhang1 merged 2 commits intoSep 14, 2026
Merged
Conversation
boyang-zhang1
force-pushed
the
boyang/jsonl-mixed-rules
branch
2 times, most recently
from
September 14, 2026 17:01
b0bdcf9 to
64f48ce
Compare
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.
boyang-zhang1
force-pushed
the
boyang/jsonl-mixed-rules
branch
from
September 14, 2026 17:14
64f48ce to
cbe8cbc
Compare
boyang-zhang1
added this pull request to stack #155
September 14, 2026 17:19
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. |
Contributor
There was a problem hiding this comment.
this is pretty hard to read lol
Contributor
There was a problem hiding this comment.
Can we shorten to 1-3 lines?
Member
Author
There was a problem hiding this comment.
Cut to 3 lines each (dc58534), and trimmed the test docstrings the same way. No code change.
logan-markewich
approved these changes
Sep 14, 2026
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
merged commit Sep 14, 2026
8a84d81
into
fix/131-layout-order-rules-dropped
4 checks passed
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.
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_datasetdropped the layout rules of a document that also carries parse rules, while the run still reportedsuccess: true. It routed those documents toLayoutDetectionTestCase. This commit keeps them onParseTestCaseinstead, because that class already type-routes layout,extract_fieldand parse rules on onetest_ruleslist and is the only branch that carries the parse-side state.Why the container matters
LayoutDetectionTestCaseParseTestCaseexpected_markdownallow_splitting_ambiguous_merged_tables,trm_unsupported,max_top_title_rowstest_rulestypeSerializeAsAny[ParseRuleBase]Three concrete consequences, all reproduced against
main+ #133:expected_markdown):ParseTestCase | md='| a |' trm=True split=True maxtop=3becomesLayoutDetectionTestCase | md=<no field> trm=<no field> split=<no field> maxtop=<no field>.data/table.jsonlis entirelyexpected_markdown-driven, so one layout annotation on a table document would have erased its headline metric with no error.extract_fieldrules crash the load. A rule registered throughparse_bench.extensions.register_rule_typevalidates fine onParseTestCaseand raisesValidationError: 63 validation errorsonLayoutDetectionTestCase; anextract_fieldrule raises too. The exception propagates out of_load_jsonl_dataset, aborting the whole run including unrelated documents._detect_product_type(inference/cli.py) inspects onlytest_cases[0]of the test-id-sorted list and runs before--groupfiltering. With a mixedchart/...document sorting first, allamaparsepipeline's product type is overridden tolayout_detectionfor all five categories.The class choice buys nothing at evaluation time:
_has_mixed_rulesaccepts both classes and_evaluate_multi_taskis typedLayoutDetectionTestCase | 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_markdownrow stays on the layout branch and still loses that markdown — a pre-existing gap. Moving it toParseTestCaseto keep the markdown would be a worse trade:_has_mixed_ruleslooks for a non-layout rule, finds none, and the document would lose its layout scoring instead. Fixing it properly means teaching_has_mixed_rulesabout 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_annotationsaccepts onlyLayoutDetectionTestCase, so a mixed document loses its GT overlay in comparison output; and anextract_fieldrule on aParseTestCaseis carried but not scored, becauseParseEvaluatorfilters it out.Scope
The shipped dataset has no document mixing layout with anything else (
data/layout.jsonlis 100%type: "layout", and the group key is(category, pdf)), sodata/anddata/test/load identically before and after — verified. No current leaderboard number moves.Tests
tests/parse_bench/test_cases/test_loader.pykeeps @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, theexpected_markdown-only shape stays layout-shaped, and a mixed document with anextract_fieldrule loads (with an explicit assertion that the same payload is aValidationErroron the other class — the test is about load-time acceptance, not scoring).ruff check,ruff format --checkand the test_cases + evaluation + analysis suites (2593 tests) are clean.