fix: eleven adversarial-review findings (seed skipping, manifest corruption, nan p-values, and more) - #100
Conversation
- blackboard: seed_turn now counts MEMBER slots only, so a pre_hook (referee) injection can no longer silently skip the planted seed and un-match baseline/intervention pairs - data: options serialized as JSON (a literal '|' in an option no longer corrupts answer_index; legacy pipe manifests still parse); Case.meta round-trips through a new manifest column (the CheXpert support-devices natural-cue flag survives disk) - cli: 'benchmaxxing datasets' now lists the five registered adapters (it always printed none: it read a never-imported attribute) - analysis: _invoke gains a complete() branch so gateway backends drive the solo lane as documented; lineage_overlap_test short-circuits an undefined statistic to a nan p-value (it reported the smallest possible p); _resolve_metric always uses the guarded overlap implementations so undefined cases stay nan - stats: multiple_comparison refuses non-finite p-values (a nan silently destroyed every BH rejection); fisher_exact docstring corrected to the sample (unconditional) odds ratio - blind_metric: latch_rate matches per turn (a multi-word decoy no longer matches across the boundary between two adjacent turns) - transcript: numpy-scalar answers/committed values are canonicalized (dump no longer crashes; committed map round-trips) Suite: 222 passed locally on this branch, ruff clean.
# Conflicts: # benchmaxxing/cli.py
getattr(datasets, 'registry') returned None since the submodule was never imported, so the command always printed the empty-registry message. Use registry.names() directly.
maximinl
left a comment
There was a problem hiding this comment.
These fixes generally look well targeted, and the regression coverage is helpful. One part may be worth tightening: the new complete() branch in analysis._invoke sends a structured text payload to the gateway as str(payload). That produces a Python dictionary representation rather than the MCQ prompt format a model expects, while the regression test only checks that no TypeError occurs. A small semantic test that captures the prompt passed to complete() — or an explicit formatter/adapter boundary — would give more confidence that direct gateway support produces meaningful evaluations rather than merely running without an exception.
…nto fix/review-findings
…e solo lane Per @maximinl: analysis._invoke's complete() branch sent str(payload) (a Python dict repr), so a gateway backend ran without a TypeError but never saw a real question. Add _payload_to_prompt() to render a text twin payload into a lettered MCQ prompt, and a semantic test that captures the prompt the backend receives and asserts it contains the question and options (not a dict repr).
|
Thanks @maximinl, you're right, |
felipeocampoos
left a comment
There was a problem hiding this comment.
Reviewed both commits and ran the suite locally on this branch.
Findings addressed (spot-checked each against its regression test):
blackboard.seed_turnnow counts member slots only — verifiedtest_seed_survives_pre_hook_injectioncovers the referee-injection-skips-seed case.data.pyoptions round-trip via JSON instead of'|'.join— legacy pipe manifests still parse (dedicated test), andCase.metasurvives the manifest round trip.cli.py datasetsnow correctly lists the 5 registered adapters.analysis._invokegained acomplete()branch for gateway backends, and the second commit fixes the initial oversight (sendingstr(payload)/ a dict repr instead of a real MCQ prompt) — good catch incorporating that review feedback with its own semantic test.analysis.lineage_overlap_test/stats.multiple_comparison: nan-observed-statistic now short-circuits to nan p-value instead of reporting the smallest possible p;multiple_comparisonnow refuses non-finite input instead of letting a stray nan silently zero out every BH rejection. This one matters most — it's a correctness bug that could have made an insignificant result look significant.blind_metric.latch_ratenow matches per-turn instead of over the joined transcript, so a decoy term can't spuriously match across a turn boundary.transcript.pynumpy-scalar canonicalization fixes a real crash ondump_transcriptfor numpy answer types.
Verification:
ruff check benchmaxxing tests— clean.pytest— 407 passed, 6 skipped (optional extras not installed: ruptures/torch), 1 failed (test_checksum_is_stable_and_matches_hashlib) — confirmed this failure is pre-existing onmaintoo (Windows CRLF vs the literal-string hash fixture), not introduced by this PR.
Each of the 11 findings has its own named regression test, which makes this easy to verify in isolation. LGTM — approving.
Implements benchmaxxing/datasets/medmcqa.py mapping MedMCQA's JSON/JSONL rows (question, opa..opd, cop, subject_name, topic_name) to the shared Case schema, following the same pattern as the medqa adapter: opa..opd become an ordered options tuple, cop maps directly to answer_index, and subject_name/topic_name flow into Case.meta. Registers the adapter in registry.py, so `benchmaxxing datasets` lists medmcqa alongside the existing five. Tests cover the fixture round trip, directory resolution, limit, provided ids, missing-data and out-of-range-cop errors, and the meta mapping (checked on the Case object directly, since the manifest round trip does not yet preserve meta on main -- that lands separately in PR #100). Suite: 403 passed, 6 skipped, ruff clean (the one pre-existing failure, test_checksum_is_stable_and_matches_hashlib, is a Windows CRLF issue on main, unrelated to this change).
…e solo lane Per @maximinl: analysis._invoke's complete() branch sent str(payload) (a Python dict repr), so a gateway backend ran without a TypeError but never saw a real question. Add _payload_to_prompt() to render a text twin payload into a lettered MCQ prompt, and a semantic test that captures the prompt the backend receives and asserts it contains the question and options (not a dict repr).
fix: eleven adversarial-review findings (seed skipping, manifest corruption, nan p-values, and more)
Implements benchmaxxing/datasets/medmcqa.py mapping MedMCQA's JSON/JSONL rows (question, opa..opd, cop, subject_name, topic_name) to the shared Case schema, following the same pattern as the medqa adapter: opa..opd become an ordered options tuple, cop maps directly to answer_index, and subject_name/topic_name flow into Case.meta. Registers the adapter in registry.py, so `benchmaxxing datasets` lists medmcqa alongside the existing five. Tests cover the fixture round trip, directory resolution, limit, provided ids, missing-data and out-of-range-cop errors, and the meta mapping (checked on the Case object directly, since the manifest round trip does not yet preserve meta on main -- that lands separately in PR #100). Suite: 403 passed, 6 skipped, ruff clean (the one pre-existing failure, test_checksum_is_stable_and_matches_hashlib, is a Windows CRLF issue on main, unrelated to this change).
What
Fixes eleven defects found by an adversarial review of the integrated codebase (find, then independently verify each finding). Every fix carries a regression test in
tests/test_review_fixes.py. All offline; no data or keys needed to review.High severity
seed_turnnow counts member slots only, so apre_hook(real-time referee) injection can no longer silently skip the planted seed, which un-matched baseline/intervention pairs and would have fabricated the referee's measured effect.|no longer splits and silently shiftsanswer_indexonto the wrong ground truth (legacy pipe manifests still parse).lineage_overlap_testshort-circuits an undefined statistic to a nan p-value; it previously reported the smallest possible p-value (1/(n+1)) when the observed overlap was nan.Medium
Case.metanow round-trips through the manifest (newmetacolumn); the CheXpert support-devices flag, the natural-cue signal, previously vanished on disk.benchmaxxing datasetslisted nothing; it now lists the five registered adapters.multiple_comparisonrefuses non-finite p-values (a single nan silently destroyed every BH rejection in the family)._resolve_metricalways uses the guarded overlap implementations, so undefined cases stay nan instead of reading as a valid 0.0.committedround-trips.Low
_invokegains acomplete()branch, so gateway backends drive the solo lane as the docstring promises.latch_ratematches per turn; a multi-word decoy term no longer matches across the boundary between two adjacent turns.fisher_exactdocstring corrected to the sample (unconditional) odds ratio.Suite: 222 passed, ruff clean. Note: ten further review candidates could not be independently verified before capacity ran out; they are lower-confidence and can be triaged separately.