Scoring Module Dev: atomic stage-pipeline protocols with official-result validation - #149
Conversation
Made-with: Cursor
- base.py build_prompt: rebuild the `options` dict when a dataset stores choices
as flat per-letter keys (HR-Bench: A/B/C/D) or a single `options_text` string
(MMVP: "(a) Open (b) Closed"), so the template actually shows the choices.
Without this the model never sees the options and scores collapse to ~random.
- qwen3_vl.py: default dtype to bf16 and attn_implementation to flash_attention_2
("auto" mis-resolves to fp32 on merged ckpts -> OOM + breaks flash-attn);
both overridable via --dtype / --attn_implementation / MMEVAL_ATTN.
- run_qwen3vl4b_{blink_mmstar,mcq_suite}.sh: pass --model_series so merged
checkpoints (whose dir name isn't in the registry) can be evaluated; allow
HRBench shards-per-GPU override via env.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VvpLzEozXhCYfabdoL1nve
Irisicy4
left a comment
There was a problem hiding this comment.
Scoring design looks solid overall (fingerprinted resume, llm_error vs wrong separation, refuse-don't-guess). A few things to address — inline comments below, plus one repo-level ask:
Can we trim the ~220k lines of generated result.json/score.json under tests/examples/ (two 95k-line score.json files)? They'll churn on every scoring change; small fixtures (e.g. 20-row slices) or CI-generated outputs would cover the same paths.
| if any(name.strip().lower().startswith("llm") for name in proto.matching_order.split(",") if name.strip()): | ||
| fingerprint["judge_provider"] = args.judge_provider | ||
| fingerprint["judge_model"] = args.judge_model | ||
| fingerprint["judge_temperature"] = args.judge_temperature |
There was a problem hiding this comment.
judge_include_reason changes the judge prompt ("Include a concise reason" vs "Set reason to an empty string") and judge_max_tokens changes truncation/parse-failure behavior — both affect verdicts but are missing from the resume fingerprint, so resuming across a toggle silently mixes verdicts from two judge configs. Please add both to this LLM branch.
There was a problem hiding this comment.
Done, both are now in the LLM branch of the fingerprint.
| if len(up) == 1 and up in candidates: | ||
| letters.append(up) | ||
| elif 2 <= len(up) <= len(candidates) and all(c in candidates for c in up) and len(set(up)) == len(up): | ||
| letters.extend(up) # compact form "AC" |
There was a problem hiding this comment.
Asymmetry: compact "AC" is accepted for predictions here, but parse_gt_letters rejects compact gts — so a dataset storing gt as "AC" scores 0 forever, silently. Suggest making it symmetric by NOT accepting compact form anywhere (it's uncommon and ambiguity-prone) and leaving compact-format datasets to a user patch.
There was a problem hiding this comment.
Rather than dropping compact parsing (it's a real output format), we made it symmetric where it's unambiguous: matchers now accept compact gts once the row is typed mcq, restricted to the sample's actual candidate letters. Type inference alone still refuses compact form since "BED"-style words would misclassify.
| def build_summary(sample_results: List[Dict[str, Any]]) -> Dict[str, Any]: | ||
| total = len(sample_results) | ||
| correct = sum(1 for x in sample_results if x.get("is_correct") == 1) | ||
| accuracy = (correct / total) if total else 0.0 |
There was a problem hiding this comment.
accuracy divides by total including invalid samples, which deflates scores on answer-withheld/malformed splits. Please emit both accuracy and accuracy_valid (correct/valid) with a 1-line comment explaining the difference.
There was a problem hiding this comment.
Done, summary now carries both: accuracy (over all rows, invalid counts as wrong) and accuracy_valid (over gradable rows only).
| if abs_tol > 0.0 and abs(pred - gt) <= abs_tol: | ||
| return True | ||
| rel_tol = float(context.get("numeric_rel_tol") or 0.0) | ||
| if rel_tol > 0.0: |
There was a problem hiding this comment.
When gt == 0 this turns rel_tol into an absolute tolerance (pred 0.04 passes at rel_tol 0.05). ChartQA's official relaxed accuracy falls back to exact equality at target 0 — please exact-match when gt==0 (with abs_tol still honored), otherwise the official_protocol stamp is inaccurate for these rows.
There was a problem hiding this comment.
Fixed, numbers_match no longer applies numeric_rel_tol at gt == 0 (abs_tol still honored), matching the official implementation (lmms-eval chartqa relaxed_correctness: the target_float truthiness check
routes target==0 to exact match).
| cache_by_id[str(row["eval-id"])] = row | ||
| # Prefer tmp (fresher) when available. | ||
| if path.endswith(".tmp"): | ||
| return cache_by_id |
There was a problem hiding this comment.
If tmp passes the fingerprint we return only tmp's rows, dropping a fuller same-fingerprint final score.json. Union (final first, tmp overwrites) recovers more resumable rows — though that may require synced changes elsewhere (flush/finalize logic), so fine as a follow-up.
There was a problem hiding this comment.
Fixed, final score.json loads first, tmp overlays it, both fingerprint-gated. A completed run also always clears its tmp now, so a stale tmp can never overlay a fresher final.
| "qwenvl2d5": ["Qwen2.5-VL-3B-Instruct", "Qwen2.5-VL-7B-Instruct", "Qwen2.5-VL-32B-Instruct", "Qwen2.5-VL-72B-Instruct", | ||
| "qwenvl2d5": ["Qwen2.5-VL-3B-Instruct", "Qwen2.5-VL-3B-Instruct-merged", "Qwen2.5-VL-7B-Instruct", "Qwen2.5-VL-32B-Instruct", "Qwen2.5-VL-72B-Instruct", | ||
| "Qwen2.5-VL-3B-Instruct-AWQ", "Qwen2.5-VL-7B-Instruct-AWQ", "Qwen2.5-VL-32B-Instruct-AWQ", "Qwen2.5-VL-72B-Instruct-AWQ"], | ||
| "qwenvl2d5_my": ["Qwen2.5-VL-3B-Instruct-merged", "Qwen2.5-VL-7B-Instruct-merged"], |
There was a problem hiding this comment.
qwenvl2d5_my (plus *-merged now living in two series) looks like a personal experiment series leaking into the shared registry — and makes get_series order-dependent. Drop or rename before merge.
|
|
||
| Reads the subset manifest from the dataset repo's metadata.json, injects | ||
| the scoring-relevant metadata as the flat top-level `dataset_meta` sample | ||
| field, and ensures grading fields sit at the sample top level regardless |
There was a problem hiding this comment.
Docstring says the loader "ensures grading fields sit at the sample top level", but _process_sample never lifts any fields — the scorer rejects old-layout files instead. Please fix the docstring to match.
There was a problem hiding this comment.
Fixed. Docstring now states the actual contract: rows are expected flat; non-conforming rows pass through unchanged and surface at scoring time via the scorer's explicit layout rejection.
Brings the branch up to date with main (17 commits): native dataset sampling (--sample_num/--sample_order/--sample_seed), the --subset flag, six new model series (kimi_vl, minicpm_v_4d5, glm_4d1v, ovis2, aria, qwen3d5) with registry/env/infer files, boundary-token stripping for aria/glm_4d1v, tsv loader and docs updates, and the skills/ tooling. Conflict resolutions: .gitignore union; main's --subset help text; in mmeval_hf.py main's __init__ wins (the branch's dataset:subset suffix alias is dropped in favor of --subset) while the branch's committed dataset_meta injection and multi-config fallback are kept. Docs updated where main referenced the now-renamed mm-eval/MMBench-en-V11 dataset (consolidated into mm-eval/MMBench-V11 with en/cn subsets). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- resume fingerprint covers judge_include_reason and the resolved
judge_max_tokens; score.json's config embeds the fingerprint verbatim so
resume validates against the final output by construction
- resume cache is the union of final score.json and tmp (tmp overlays);
a completed run always clears its tmp so stale rows never mask a
fresher final
- mcq letter-set grading accepts compact multi-letter gts ("AC") at
match time for rows already typed mcq; type inference stays conservative
- numeric rel_tol no longer applies at gt==0 (official ChartQA relaxed
accuracy semantics)
- summary reports accuracy_valid (correct/valid) alongside accuracy
- registry: drop the unreachable qwenvl2d5_my series and the duplicated
-merged model name
- mmeval_hf docstring matches the actual flat-layout contract
- example fixtures are 20-sample real-pipeline runs selected with the
native sampling flags (--sample_num 20 --sample_order random
--sample_seed 42, mm-eval/MMBench-V11 --subset en): 682,780 -> 2,164
lines across the four MMBench files
- add scripts/migrate_result_layout.py, referenced by the scorer's
old-layout rejection message
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Done. The four files are now 20-sample runs produced end-to-end by using the framework's native sampling flags. |
Irisicy4
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — fingerprint, tmp/final union, gt==0, accuracy_valid, registry and docstring all look good. Two issues on the new revision:
| "accuracy": 0.0, | ||
| "accuracy_valid": 0.0, | ||
| "mean_score": 0.0, | ||
| "invalid": 20, |
There was a problem hiding this comment.
All four regenerated fixtures are 100% invalid: MMBench 20/20, hf_template 20/20, LLaVABench 60/60, modality_test 18/18 — MMBench's answer is '' (answer-withheld split) and the LLaVABench rows lack reference_response. So the fixtures exercise only the invalid-sample path: zero matcher/grader/judge code is covered, and they'd green-light a fully broken scorer. Please regenerate from a split with answers (e.g. MMBench dev) and add an invalid == 0 assertion to the fixture-generation step.
There was a problem hiding this comment.
Fixed as you suggested.
| # Compact multi-letter gts ("AC") are accepted HERE — the row is | ||
| # already typed mcq — while parse_gt_letters stays conservative for | ||
| # type inference (compact form collides with words like "BED"). | ||
| gt_letters = parse_gt_letters(gt_raw) or extract_letter_set(gt_raw, candidates) |
There was a problem hiding this comment.
Note this goes the opposite direction from my suggestion (reject compact everywhere) — accepting compact gts at match time is a defensible alternative, but it has a residual collision: with the A–F fallback candidates (options-in-image datasets), word-like gts parse as letter sets — "BED" → (B,D,E), "FACE" → (A,C,E,F). Please guard it: only accept compact gts when the candidates come from real parsed options, not the A–F fallback (same for template.py / llm.py) — or revert to rejecting compact form.
There was a problem hiding this comment.
Fixed as you suggested. Compact gt parsing now requires the candidates to come from real parsed options; under the A–F fallback a word-like gt ("BED", "FACE") is treated as a whole-string answer instead of a letter set.
…ard)
- example fixtures: regenerate both MMBench fixtures from the dev split
(answers present) with the same native-sampling flags — 20/20 valid,
accuracy 0.65, exercising option discovery/extraction/matching instead of
only the invalid-sample path; example scripts now score their output and
assert summary.invalid == 0 as the fixture gate
- mcq: compact multi-letter gts are only parsed when the candidates come
from a real parsed option source (new extraction.parsed_options + an
options_parsed context flag, honored by exact/template/llm-match) — under
the A-F fallback, word-like gts ("BED") no longer read as letter sets
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…, docs Co-authored-by: Cursor <cursoragent@cursor.com>
| "accuracy": 0.0, | ||
| "accuracy_valid": 0.0, | ||
| "mean_score": 0.0, | ||
| "invalid": 60, |
There was a problem hiding this comment.
This fixture is still 60/60 invalid (and modality_test is 18/18) — the round-2 regeneration covered only the two MMBench fixtures, and the invalid == 0 gate lives only in those two scripts. LLaVABench is the judge-path example, so the llm-judge stage still has zero fixture coverage; everything here exercises only the missing-gt path. Please regenerate it with reference_response present (dev rows) and extend the fixture gate to this script — or drop the degenerate score.json fixtures (LLaVABench, modality_test) rather than committing all-invalid examples.
There was a problem hiding this comment.
Regenerated.
Root cause: LLaVABench keeps its reference answer in gpt4_ans, not the default answer field, so gt resolved to None and every row fell to missing_gt_or_pred; with no dataset_meta the default exact-match,rule-match also ran in place of the declared llm-judge protocol.
Re-scored with --score_gt_field gpt4_ans --score_pipeline llm-judge under an open-weight local judge (Qwen2.5-7B-Instruct, greedy): 60/60 valid, 0 invalid, 0 llm_errors, all 60 rows decided by llm-judge.
| fingerprint must not be reused — resuming a rule-only run into a | ||
| `rule-match,llm-match` rerun would silently keep the old verdicts.""" | ||
| fingerprint = { | ||
| "score_pipeline": list(proto.pipeline), |
There was a problem hiding this comment.
The resume fingerprint covers every config knob but carries no dataset identity — and the cache's only binding to its result.json is "same folder". Same-dataset variants collide silently: rerun the same benchmark into the same out_dir with --split test after a dev run (or --subset cn after en, or after the dataset is re-pushed upstream with corrected rows) and the fingerprint passes, eval-ids 0..N collide, and the old split's verdicts are stamped onto the new rows with no trace.
Please have the loader inject dataset_name / subset / split into dataset_meta (it already knows all three) and include them here — that also makes score.json self-describing about what it scored, which VALIDATION.md's table currently asserts from outside the artifact.
(Doesn't cover same-dataset re-inference with a new checkpoint — a per-row sha256(pred) check would; fine as a follow-up.)
There was a problem hiding this comment.
Done. The loader now injects dataset_name/subset/split into dataset_meta (mmeval_hf.py). A rerun of a different split/subset/dataset into the same out_dir is now a cache miss and is re-scored instead of inheriting stale verdicts.
| raise ValueError( | ||
| f"{result_file}: dataset_meta uses the retired composite `score_type` " | ||
| f"vocabulary — this result.json was produced by an older loader. " | ||
| f"Re-run inference with the current loader (it translates published " |
There was a problem hiding this comment.
Stale remediation: "the current loader translates published metadata to score_pipeline" is no longer true — the translation boundary was deleted this commit, and the loader now hard-fails on score_type with its own retirement error (mmeval_hf.py). A user following this message re-runs inference and dead-ends on the loader's error, each message pointing at the other component. Please change the remediation here (and keep the loader message consistent) to: "re-push the dataset with the score_pipeline schema, then re-run inference."
There was a problem hiding this comment.
Fixed. Neither message references the old format's history anymore; they state that the score_type key is unsupported and point to the score_pipeline schema (docs/en/SCORING.md, 'Dataset metadata contract'), then re-run inference.
…xamples - add a text-LLM `local` judge provider so `llm-judge` / `llm-match` can run without an external API, and document its usage and measured agreement with a GPT judge in docs/en/OPEN_JUDGE.md - carry dataset_name/subset/split into dataset_meta and the resume fingerprint, so re-scoring a different split/subset/dataset into the same out_dir is a cache miss instead of silently inheriting the previous run's verdicts - reject the unsupported `score_type` metadata key with the same actionable message in both the loader and the scorer - regenerate the LLaVABench example as a genuine `llm-judge` run and drop the degenerate all-invalid score.json fixtures Co-authored-by: Cursor <cursoragent@cursor.com>
| """ | ||
| env = Environment() | ||
| env.globals.update({'zip': zip, 'enumerate': enumerate, 'len': len, 'range': range, 'list': list, | ||
| env.globals.update({'zip': zip, 'enumerate': enumerate, 'len': len, 'range': range, 'list': list, |
There was a problem hiding this comment.
This PR's history includes b0b89a8 ("eval: render MCQ options for HRBench/MMVP"), which added ~15 lines right here in build_prompt: rebuild options from flat per-letter keys (HR-Bench A/B/C/D) or a single options_text string (MMVP) so the template's {% if options %} actually fires. Per that commit's own message, without it "the model never sees the options and scores collapse to ~random" — it was validated as part of this branch's parity work.
The sync-merge cd1fb16 resolved the base.py conflict by taking main's side, and the hunk is gone from the head tree (no options_text anywhere in mmeval/data/) — so the PR's commit log advertises a fix its diff no longer contains. Anyone reading merged history will believe it landed.
Was dropping it intentional? If deferred as out-of-scope for the scoring PR, please say so in the PR description and file a follow-up to restore it; if not, cherry-pick the hunk back from b0b89a8. Either is fine — the current silent state is the only bad option.
There was a problem hiding this comment.
It is intentional. Option normalization was moved out of build_prompt and into the dataset loader part: datasets can declare their own prompt template in metadata.json, and the TSV loader rebuilds the options dict itself, which makes a generic rebuild inside build_prompt redundant.
Both datasets are still covered, and I verified the affected paths end-to-end: mm-eval/MMVP renders through its own "{{ options.options_text }}" template; evalkit's MMVP.tsv is index/image/question/A/B/answer, so tsv.py:158 rebuilds the options dict for the default template; HRBench4K is covered the same way on both paths.
Restoring the hunk would only add dead code: its guards fire only when options is empty and the choices sit in top-level keys, and neither holds in the current sample shape.
Irisicy4
left a comment
There was a problem hiding this comment.
Looks good and have reproduction validation now
Summary
Introduce the scoring module for simple-mmeval: score
result.jsonoutputs against ground truth using dataset-defined protocols from the mm-eval new format (flat sample fields +metadata.jsongrading config). Every protocol is an ordered list of atomic stages (no composite scorers), so grading is composable, inspectable, and user-definable from the CLI, and reproduces official published results with each dataset's declared pipeline unchanged.What's included
mmeval/score.pyCLI and scoring pipeline: discoverresult.jsonfiles, run per-sample grading, writescore.jsonwith summary and full config provenance.exact-match,rule-match(rule-based, free),llm-match,llm-judge(LLM), and the fractional gradersvqa-accuracy,anls. Stages run in order (pass→ next,score→ short-circuit); a grader must be last. LLM/API failures never become verdicts — they surface asllm_errorand are re-scored on resume.dataset_meta(task_type,score_pipeline,score_params, protocol notes) injected at load time from each dataset'smetadata.json. Running the declared list stampsofficial_protocol: true;--score_pipelineoverrides it (precedence: CLI flag > dataset metadata > default).answer,question_type, etc.) live as flat top-level sample fields;messagescarries render inputs only.dataset_metafor downstream scoring.--judge_provider—local(open-weight model via the framework's own loader, no external service),openai, orazure_openai.Coverage & validation
score_pipelinenatively.Type of Change
Testing
Test command used:
# Command you used to testChecklist
mmeval/registery.py(if new model)