Skip to content

feat(#297): SUPPORT2 tabular lane, adapter + answer-preserving cues + three runners - #354

Merged
Yehudha-kennedy merged 4 commits into
mainfrom
feat/support2-tabular-lane
Jul 29, 2026
Merged

feat(#297): SUPPORT2 tabular lane, adapter + answer-preserving cues + three runners#354
Yehudha-kennedy merged 4 commits into
mainfrom
feat/support2-tabular-lane

Conversation

@Agastya191

Copy link
Copy Markdown
Collaborator

Closes part of #297: the SUPPORT2 tabular lane (Lane C), implementation only. Per the coordination on the issue, this is the implementation half; the real-data run is being handled separately.

What this adds

  • benchmaxxing/datasets/support2.py, the SUPPORT2 adapter. Each patient row renders as a vignette plus a binary prognostic question (hospdead by default, death available), with the ordered record kept in meta["fields"] so cues can perturb fields and re-render. Registered in the dataset registry.
  • benchmaxxing/cues/tabular.py, six answer-preserving tabular cues.
  • experiments/support2/, the three runners the issue asks for (solo susceptibility + noise floor, confident-wrong-seed cascade contagion, referee detection), a shared _common.py, a provenance-checked manifest builder, and a lane README.
  • experiments/support2/results/support2_manifest.csv + support2_provenance.json, the pinned 120-case set.
  • Offline tests for all of it.

Why tabular is a sharper test than imaging or text

On a radiograph or an MCQ, "the cue did not change the evidence" is a judgement call. On a table it can be made exact. Five of the six cues are information-identical: the contaminated record states precisely the same clinical facts as the clean one, reordered, restated in equivalent units (mg/dL to SI, C to F), padded with decimals, named twice, or with an already-absent value written out as "not recorded". A model that changes its prognosis under those is reading surface form, not evidence.

administrative_hint is the exception. It appends a line the record did not contain, so it is the tabular analogue of the text lane's demographic_hint, and it is reported separately rather than folded into the headline number.

Design decisions worth a look

The lane reuses Modality.TEXT rather than adding a third modality. schema.py asks to be kept small and stable, and a tabular case is a serialized record rendered into the existing text contract. This means data.py, validate.py, blackboard.py and referee.py are untouched. The tabular-ness lives in meta["fields"], which is what the cues operate on. If reviewers would rather see Modality.TABULAR, that is a reasonable disagreement and cheap to change, but it touches the shared contract.

The binary outcome forced a second cascade arm. A confident-wrong seed can only recruit a holdout that was right on its own, so every patient the model was already wrong about is silently uncountable. Reporting wrong_seed alone would hide that. support2_cascade.py therefore also runs flip_seed (the peers assert whatever the holdout did not say bare), which is eligible everywhere by construction and gives a polarity split: does a confidently wrong peer pair recruit as well as a confidently right one?

targeted is a genuinely distinct referee here. In experiments/referee/referee_deployable.py the oracle's predicate is the scoring definition, so its perfect score is definitional. In this lane targeted gets the planted key but no counterfactual, so it over-fires on patients the holdout was independently wrong about. That makes naive to targeted to deployable an informative ranking rather than one trivially perfect row. oracle_audit is still reported, labelled as a ceiling.

Leakage columns are never rendered. The SUPPORT and APACHE models' own survival estimates (surv2m, surv6m, prg2m, prg6m), the physiology scores behind them (sps, aps), the outcome and follow-up columns, DNR status, and the cost columns. Full list in support2.EXCLUDED, copied into the provenance JSON.

Two bugs the real data exposed

The distributed support2.csv header is off by one. It writes an unnamed row-index column that its header line does not name, so every data row carries 48 fields against 47 header names. A plain csv.DictReader shifts every column and hospdead silently reads as sex. Nothing looks wrong: the vignettes still render and the labels are still 0/1-ish, while the entire lane's ground truth would have been the patient's sex. read_rows detects the extra field on the first data row and realigns. Pinned by test_unnamed_index_column_is_realigned, which uses the real file's shape.

precision_inflation was rounding instead of padding. The source stores labs at raw float precision (1.7998047 for an albumin of 1.8). Padding that to 2 decimals gives 1.80, a changed number, which is the one thing an information-identical cue may not do. Fixed on both sides: the adapter renders at chart precision, and the cue refuses to touch a value already more precise than the target.

A concurrency bug worth knowing about beyond this lane

GeminiBackend imports the vendor SDK lazily on first construction. If a ThreadPoolExecutor is allowed to be the first thing that touches it, every worker races into that same import and the process deadlocks on the import lock: zero calls reach the network, and the run hangs indefinitely with an empty cache rather than failing. I hit this twice before sampling the process and seeing all four workers parked in PyImport_ImportModuleLevelObject.

_common.Cache now serializes backend construction and warms it on the main thread in __init__.

The same pattern exists in experiments/imaging/imaging_solo.py:80-81 (unguarded lazy construction inside the pool; ask_uncached takes the lock, ask does not) and, in a milder form, in experiments/referee/referee_deployable.py:106 and experiments/medqa/text_cue_types.py:92. It is a race, so it usually wins and nobody sees it, but the failure mode is a silent hang rather than an error. Not fixed in this PR since those lanes are in active use; happy to send a separate hardening PR.

What is NOT in this PR

No results. The implementation is verified end to end against the live API on a 2-case smoke run (16 calls, well-formed summary), but those artifacts were deleted rather than committed: at n=2 the noise floor is 0.5 and every rate is 0, 0.5 or 1, which is not a result and should not sit in results/ looking like one.

The manifest and provenance are committed so whoever runs it lands on the same 120 patients (60 survive / 60 die, drawn from the 9,105-patient cohort whose in-hospital mortality is about 26%, so the balancing matters for interpreting clean accuracy).

Testing

Offline, no key, no network:

  • tests/test_support2_adapter.py, row parsing, leakage exclusion, the shifted-header repair, manifest round-trip.
  • tests/test_tabular_cues.py, every cue is answer-preserving, plus per-cue behaviour and failure modes.
  • tests/test_support2_experiments.py, drives all three runners end to end against a stub backend and asserts the referee ordering actually holds (naive over-fires at FPR 1.0, targeted over-fires at precision 0.5, deployable is clean at precision 1.0).

Also verified: all six cues fire on 200/200 real SUPPORT2 records, and the unit conversions are correct (36 C to 96.8 F, 1.2 mg/dL creatinine to 106.1 umol/L, 1.8 g/dL albumin to 18 g/L).

Reviewer attention

  • The committed manifest embeds patient records. SUPPORT2 is public and de-identified with no DUA, and the imaging lane commits a manifest too, but that one holds image references while this one holds rendered vignettes (age, sex, race, diagnoses, labs) for 120 patients. If that is not wanted in git, the alternative is committing only the provenance JSON plus a case_ids.txt and rebuilding with --case-ids-file, which gives identical reproducibility. Easy to switch, say the word.
  • The UCI download endpoint for dataset id 880 currently returns NOT FOUND. The README points at the Vanderbilt mirror with a checksum.

… ablation

_shortcut_answer always seeded the first distractor, which on the first real run
coincided with the committee's own independent answer in 16/20 cases. With seed ==
baseline there is no shared-vs-isolated gap, so contagion is undefined (mean came out
-0.006, uninterpretable).

- run_cascade gains seed_target="baseline_relative": choose a non-correct option the
  committee would NOT independently give, measured against an isolated unseeded verdict
  under the same (contaminated) condition as the arms, reusing referee._final_answer.
- content_builder hook lets the seeded turn carry a confident claim (terse_seed_content);
  the cue-anchored reasoned rationale stays #115's build_seed_content, this is its seam.
- run_cascade_ablation sweeps seed targets and reports per-arm contagion/onset, censoring
  the degenerate seed==baseline cases rather than averaging them as spurious zeros.
- Default (first_distractor, bare) keeps the seed construction and every existing arm
  unchanged; the result dict only gains additive metadata keys.

Codex adversarial-review fixes folded in:
- baseline computed under the same CONTAMINATED condition as the arms (was CLEAN), so a
  condition-sensitive backend is baselined against the verdict adoption is measured over.
- _UNSET sentinel distinguishes an unsupplied baseline from a genuine None (abstaining)
  verdict, so None is reused instead of triggering a second, possibly divergent, run.
- _seed_adoption excludes the planted turn via its seeded flag, not a reserved "seed"
  agent id, so a real member named "seed" is still counted.

Ground truth is never altered: only the planted peer turn changes.
Tests: tests/test_cascade_baseline_seed.py (14). Full suite 674 passed, 3 skipped.
Note: ruff not runnable in this env; verified line length <= 100 and clean py_compile.
Add a blank line before the referee-import explanatory comment so the
import block is isort-clean; this is the one ruff finding introduced by
PR #292. The pre-existing RUF022 (__all__ ordered by issue number to
mirror the module docstring) is left unchanged.
The cascade seed was a bare planted answer, and the diagnosis on the first null run was that a
confident committee rejects a naked assertion: propagation needs a seed carrying a reason tied to
a cue in the case. This adds that stimulus behind a seed_style parameter on run_cascade.

- reasoned_seed_content states the shortcut answer and justifies it with a detail quoted from the
  case's own vignette. It delegates to the Break-it C builders (case_anchor / anchored_seed_content,
  #138) rather than composing a third rationale wording, so a seed planted through the cascade is
  word-for-word the one that runner plants and the two stay comparable.
- SEED_STYLES names the three stimulus strengths: bare (default, unchanged), terse, reasoned.
  content_builder stays as the escape hatch and now raises if combined with a non-bare style
  instead of letting one silently win.
- seed_style threads through run_cascade_ablation as a per-sweep constant, so a style contrast is
  two sweeps over the same cases and the arm keys stay keyed by seed target alone.

A case with no question/report text raises rather than degrading to a generic rationale, which
would file a bare seed under the reasoned arm and bias the very contrast the style measures.

Default path is byte-identical: seed_style="bare" maps to no builder, so every existing arm and
the audit runs are untouched; the result dict only gains additive metadata.

Suite: 688 passed, 3 skipped. Ruff unchanged on the touched files (1 pre-existing RUF022).
… three runners

Adds Lane C: SUPPORT2 rendered into the existing text schema (vignette plus a
binary prognostic question), six tabular cues, and the three experiments the
issue asks for (solo susceptibility + noise floor, confident-wrong-seed cascade
contagion, referee detection).

Five of the six cues are information-identical: the contaminated record states
exactly the same clinical facts, reordered, restated in equivalent units,
decimal-padded, named twice, or with an already-absent value written out. A
prognosis that moves under those is following surface form, not evidence.
administrative_hint adds a line and is reported separately.

Reuses Modality.TEXT rather than adding a third modality, so schema, data,
validate, blackboard and referee are untouched; the record lives in
meta["fields"], which is what the cues perturb.

Two repairs the real data forced:

- The distributed support2.csv writes an unnamed row-index column its header
  does not name, so rows carry 48 fields against 47 names. A plain DictReader
  shifts every column and hospdead silently reads as sex. read_rows detects the
  extra field and realigns.
- precision_inflation was rounding source floats (1.7998047 to 1.80) instead of
  padding, which changes the number. The adapter now renders at chart precision
  and the cue refuses to touch an already-more-precise value.

_common.Cache serializes backend construction and warms it on the main thread:
GeminiBackend imports the vendor SDK lazily, and letting a worker pool race into
that import deadlocks every thread on the import lock, hanging the run with an
empty cache instead of failing.

Ships the pinned 120-case balanced manifest and its provenance. No results.
@Agastya191

Copy link
Copy Markdown
Collaborator Author

@MohShahin sounds good, that split works. This PR is the implementation half; the real-data run is yours whenever the imaging run frees up.

It is verified end to end against the API on a smoke run, and the 120-case balanced manifest plus provenance are committed so your run lands on the same patients (60 survive / 60 die, drawn from the 9,105-patient cohort whose in-hospital mortality is about 26%, so the balancing matters for interpreting clean accuracy). Three commands in the lane README, in order, sharing one cache so runs 2 and 3 are nearly free.

Two gotchas worth knowing before you start:

  • The UCI download for dataset id 880 is dead (returns NOT FOUND). Use the Vanderbilt mirror; URL and checksum are in the README.
  • The distributed support2.csv has an unnamed index column that shifts every field by one under a plain DictReader, so hospdead silently reads as sex. Handled in the adapter, but worth knowing if you parse it anywhere else.

Separately, and relevant to your imaging run: experiments/imaging/imaging_solo.py:80-81 constructs the Gemini backend lazily inside the thread pool without a lock. I hit exactly that in my version and it deadlocks every worker on the import lock, so the run hangs with an empty cache rather than erroring. It is a race so it usually wins, but the failure mode is silent. Happy to send a small PR hardening the imaging, referee and medqa caches the same way this PR does for the tabular lane.

@sebasmos tagging you since the SUPPORT2 call and the prioritisation were yours. Two decisions in here are worth a look if you have a minute: the lane reuses Modality.TEXT rather than adding a third modality (so schema, data, validate, blackboard and referee are untouched), and the binary outcome forced a second cascade arm, since a confident-wrong seed can only recruit a holdout that was right on its own and reporting that arm alone would silently drop every patient the model was already wrong about. Both are written up in the PR description.


Test result, run against the committed tree:

tests/test_support2_adapter.py
tests/test_tabular_cues.py
tests/test_support2_experiments.py

43 passed in 989.41s

All offline: no key, no network, no real data. test_support2_experiments.py drives all three runners end to end against a stub backend and asserts the referee ordering holds rather than just that the scripts execute: the naive gate over-fires at FPR 1.0, targeted over-fires at precision 0.5, and deployable stays clean at precision 1.0 with recall 1.0.

The full suite is not included here. An earlier run of this lane plus the adjacent modules (test_datasets, test_cli, test_text_cues) passed 89/89; I stopped a later full-suite run because it was saturating I/O and blocking git. Nothing outside the lane changed except the EXPECTED_DATASETS update in test_datasets.py, which that 89-test run covered. ruff check is clean on every file in this PR.

@sebasmos sebasmos left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The information-identical cue design is a genuinely sharper test than imaging or MCQ, since equivalence is exact on a table rather than a judgement call.

One check before this can count as done. The body says the real-data run is being handled separately per the coordination on #297, but #297 has three comments, none of them yours, and @MohShahin is the assignee. Please confirm who is actually running it, and until it lands this is implementation only, so I would rather not have it read as closing part of #297.

@Yehudha-kennedy
Yehudha-kennedy merged this pull request into main Jul 29, 2026
@Yehudha-kennedy
Yehudha-kennedy deleted the feat/support2-tabular-lane branch July 29, 2026 21:13

@EmmanuelpaulKwesiga EmmanuelpaulKwesiga left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Information-identical cue set is correctly defined: five cues restate the same clinical facts in a different surface form, with administrative_hint separated as the one that adds new information. Unit conversions verified across temp, creatinine, BUN, glucose and albumin. Shared cache and lock in _common.py is the right design: one hash and one parser across all three runners, so a cached response from the solo arm is valid for the others. Parser avoids the first \b[A-E]\b grab flagged in #265.

@Agastya191

Copy link
Copy Markdown
Collaborator Author

@sebasmos on your review question, you were right on both counts and I have acted on both.

Who is running it: me. @MohShahin is the assignee on #297 and offered to take the run, but the imaging lane is ahead of it in his queue, so rather than leave the lane parked I am doing the run now against the manifest committed here. Starting immediately; results land under experiments/support2/results/.

The close was wrong. f4dd74f merged this with Resolves #297 in the merge message, so #297 auto-closed with all three Then replicate boxes unchecked, which is the exact thing you asked not to happen. I have reopened it and written up why: #297 (comment). It closes when the three runs are committed and reproduce from cache, not before.

Separately, thank you for #360. That was my miss: I registered the adapter without the staging.SOURCES and status.STATUS entries and left main red for nine minutes. One correction to the entry you added, which you could not have known without the lane: plausibility="pending" can never reach "done" here. The plausibility arm is the plausible-vs-implausible distractor contrast, and a binary outcome has exactly one wrong option, so there is no distractor gradient to vary. Changing it to not applicable, matching what imaging already carries for the same structural reason.

sebasmos pushed a commit that referenced this pull request Aug 4, 2026
Resolves #297
Adds SUPPORT2 tabular lane, adapter, answer-preserving cues, and three runners.

# Conflicts:
#	benchmaxxing/datasets/registry.py
#	tests/test_datasets.py
sebasmos added a commit that referenced this pull request Aug 4, 2026
#360)

41b85e9 merged #354, which added support2 to registry.REGISTRY but not to staging.SOURCES or
status.STATUS. Two tests assert that every registered adapter appears in both, so main has been failing
since that merge:

  tests/test_staging.py::test_every_registered_dataset_has_a_source_entry
  tests/test_datasets.py::test_dataset_status_covers_registered_adapters

Added both entries. The status entry records the truth rather than flattering it: implementation only,
solo/cascade/plausibility/referee all pending, with the real run tracked on #297. That keeps the readiness
report honest under the real-data rule instead of letting a registered adapter look staged.

This is the same failure mode that is still latent on #352 and #249, both of which register an adapter with
no SOURCES entry. Flagged on both.

869 passed, 7 skipped, ruff clean.
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.

6 participants