Add WILD-raw item-level adapter (every_eval_ever/adapters/wild) - #203
Add WILD-raw item-level adapter (every_eval_ever/adapters/wild)#203borgr wants to merge 12 commits into
Conversation
Converts kensho/WILD-raw (65 models x 27 benchmarks, ~7.5M item responses, run with Inspect AI) to Every Eval Ever: one aggregate EvaluationLog per (model, benchmark) with overall + per-subtask accuracy (binary items -> proportion with analytic standard error), and optional per-item instance _samples.jsonl (--include-instances) linked via detailed_evaluation_results. Streams parquet from HF per row group (never the full 7GB). source_type=evaluation_run, eval_library= inspect_ai; source_data points at each benchmark's own dataset repo; input.raw is the prompt only (assistant turn -> output.raw); sample_hash for cross-model joins. Adds README + a utils/README.md row + tests/test_wild_adapter.py (3 tests).
The core test matrix installs no extras; pyarrow (used by the adapter/tests) only arrives via --all-extras, so a top-level import broke core collection. Guard with pytest.importorskip like the inspect/helm adapter tests.
mrshu
left a comment
There was a problem hiding this comment.
This is really nice work. The per-row-group streaming and the docstrings walking through each mapping decision made it easy to follow. I left a few small suggestions inline, all optional and nothing blocking. Thanks for putting this together!
| subtask = row['subtask'] if row['subtask'] not in (None, '') else '_' | ||
| # attach to the leaf result when the benchmark is split by subtask, else the | ||
| # single overall result (matches build_log's dedup so the FK always resolves). | ||
| if multi_subtask: |
There was a problem hiding this comment.
One thought here: since each multi-subtask item feeds both the overall and the leaf result, it might be nice to also emit an instance record for the overall one, since instance_level_types.py:170,174 leans that way, so the overall result isn't left without linked instances. It does roughly double the instance rows for these benchmarks, so leaving it as is would be totally reasonable too. Maybe just a short comment noting the choice either way?
| def resolve_eval_timestamp(override: str | None) -> str: | ||
| """When the evaluation was RUN. We don't have per-run times, so use the WILD | ||
| dataset's HF lastModified as a stable proxy (override with --evaluation-timestamp).""" | ||
| if override: | ||
| return str(override) | ||
| try: | ||
| from huggingface_hub import HfApi | ||
| info = HfApi().dataset_info(HF_REPO_ID) | ||
| if info.lastModified: | ||
| dt = info.lastModified | ||
| if dt.tzinfo is None: | ||
| dt = dt.replace(tzinfo=timezone.utc) | ||
| return repr(dt.timestamp()) | ||
| except Exception: # noqa: BLE001 - fall back to now | ||
| pass | ||
| return str(time.time()) |
There was a problem hiding this comment.
Small suggestion: since this timestamp feeds into evaluation_id, it could be nice to pin a commit SHA for remote runs so reruns stay idempotent even if the HF lookup hiccups, and maybe log a note rather than quietly falling back to time.time(). The same pin would also keep the two passes over main (aggregate and instances) on the same revision. Definitely a nice to have, not urgent.
| from pathlib import Path | ||
| from typing import Iterator | ||
|
|
||
| import pyarrow.parquet as pq |
There was a problem hiding this comment.
Tiny one: pyarrow is imported at import time but isn't a declared dependency, though the importorskip guard handles CI nicely. Might be worth a wild extra or a README note about --all-extras, just so someone running the adapter fresh gets a clear signal. Totally up to you.
| def _scorer_name(scores_json: str | None) -> str: | ||
| """The Inspect scorer that produced the item score = the key of the scores | ||
| JSON (e.g. 'match', 'choice', 'model_graded_qa').""" | ||
| if scores_json: | ||
| try: | ||
| keys = list(json.loads(scores_json).keys()) | ||
| if keys: | ||
| return str(keys[0]) | ||
| except (ValueError, TypeError, AttributeError): | ||
| pass | ||
| return 'unknown' | ||
|
|
||
|
|
||
| def _raw_output(scores_json: str | None, extracted: str) -> str: | ||
| """The model's full generation lives in the scorer output (`scores.<scorer>.answer`); | ||
| fall back to the extracted `answer` if absent.""" | ||
| if scores_json: | ||
| try: | ||
| for scorer in json.loads(scores_json).values(): | ||
| if isinstance(scorer, dict) and scorer.get('answer'): | ||
| return str(scorer['answer']) | ||
| except (ValueError, TypeError, AttributeError): | ||
| pass | ||
| return str(extracted or '') |
There was a problem hiding this comment.
Minor thought: _scorer_name grabs the first key while _raw_output scans all the values, so if scores ever has more than one key the two could point at different scorers. If it's always single-key then this is a non-issue. Might be worth a quick confirm, and maybe a note reconciling output.raw (which comes from scores.answer) with the docstring that mentions the assistant turn.
| overall = next(r for r in log["evaluation_results"] if r["evaluation_name"] == "wild.mmlu") | ||
| assert overall["metric_config"]["score_type"] == "continuous" | ||
| assert (overall["metric_config"]["min_score"], overall["metric_config"]["max_score"]) == (0.0, 1.0) | ||
| assert abs(overall["score_details"]["score"] - 2 / 3) < 1e-9 |
There was a problem hiding this comment.
Could be nice to also assert the standard_error here (sqrt((2/3)(1/3)/6) for this fixture) so the SE math has a little regression guard. Optional, since the formula is pretty straightforward.
…y, deps, SE guard) Addresses @mrshu's review on evaleval#203 (all optional, none blocking): - Idempotency: pin a concrete kensho/WILD-raw commit SHA up front (resolve main once), reuse it across BOTH the aggregate and instance passes, derive evaluation_timestamp from that commit's date, record dataset_revision in source_metadata, and add --revision to reproduce a past snapshot. Warn loudly instead of silently falling back to now() (which made evaluation_id non-idempotent). - Scorer consistency: read the scorer name AND output.raw from the SAME scorer (_primary_scorer) so they can't diverge if `scores` ever has >1 key. - Deps: declare pyarrow as a `wild` extra (it was undeclared) + a README install note, so a fresh run fails clearly instead of relying on a transitive dep. - Test: assert the analytic standard_error sqrt(p(1-p)/n) as a regression guard. - Instances: document that instances attach to the finest-grain result (leaf); the overall roll-up is intentionally not re-emitted (it is the union of the leaves). Verified: ruff clean; pytest tests/test_wild_adapter.py green; live smoke (shard 0, --include-instances) validates 125/125 with a real pinned commit SHA in every record.
|
Thanks @mrshu — really appreciate the careful read. Pushed a commit addressing all five:
Verified: |
mrshu
left a comment
There was a problem hiding this comment.
⚒️ review-anvil report
Review decision: COMMENT — The adapter is a strong, well-tested addition, and a few data-contract edges would benefit from follow-up.
Result: One confirmed high-priority mapping issue and two medium-priority repeatability issues remain.
Scope: This PR adds a streaming WILD-raw adapter for aggregate and item-level Inspect AI results.
Checks: 2 concerns checked; 2 confirmed, 0 ruled out, 0 set aside, 0 lowered in priority.
Second check: targeted, 2 reviewers; 5 kept, 4 fix paths clarified, 0 set aside, 0 removed.
Earlier review comments
Earlier review comments (5 items)
- Still open: Overall instance linkage remains a useful suggestion. Multi-subtask items still link only to leaf results.
- Still present at the current head: Revision fallback still uses mutable
mainwhen SHA lookup fails. The old anchor is outdated. - Fixed: The
pyarrowdependency is now declared in thewildextra and lockfile. - Fixed: Scorer selection now keeps the scorer name and answer together.
- Fixed: Standard-error coverage now checks the analytic value and sample count.
What I noticed
- [high] instance output
utils/wild/adapter.py:361—output.rawstoresScore.answer, which Inspect defines as an extracted answer. WILD-raw keeps the complete response in assistant turns, but the adapter discards those turns. Rows where extraction shortens the response lose the full model output. (RAVF001; inline) - [medium] repeat runs
utils/wild/adapter.py:473— Stable evaluation IDs do not make writes idempotent. Each run creates a new UUID aggregate and sidecar, so a reused output directory can collect duplicate logical evaluations. (RAVF002; inline) - [medium] local provenance
utils/wild/adapter.py:421— Local parquet runs have no remote revision, but the output recordsmain. They also use the current time without an explicit evaluation timestamp, so the provenance and identity change between runs. (RAVF005; inline)
Things to try
- [high] instance output — The last non-empty assistant turn can populate
output.raw. A missing assistant turn can remain explicit instead of labeling extracted scorer text as a complete response. (RAVF001) - [medium] repeat runs — An adapter-owned output set can replace the aggregate and sidecar together. The sidecar must be rebuilt rather than appended. (
RAVF002) - [medium] local provenance — Unknown local revisions can stay unset. An explicit evaluation timestamp can keep local runs stable without inventing remote provenance. (
RAVF005)
Run details
- Target: PR #203 (
add-wild-adapter, 7 files, +775/-1) - Rounds: 1/1 completed; adaptive off; material findings remained
- Mix: 3 codex-exec
- Focus: correctness, maintainability, simplicity, production impact, and constructive suggestion-oriented language
- Earlier review comments: 5 comments; 2 still present and 3 fixed
- Finding counts: 0 critical, 1 high, 4 medium, 0 low, 0 nit
- Checks: concerns=2; confirmed=2/ruled-out=0/set-aside=0/lowered=0
- Second check: targeted; reviewers=2; kept=5/clarified=4/set-aside=0/removed=0; approval changed no
- Set aside: 0 items
Reviewed with review-anvil.
| sample_hash=sample_hash, | ||
| interaction_type=InteractionType.single_turn, | ||
| input=Input(raw=prompt, reference=reference), | ||
| output=Output(raw=[raw_out]), |
There was a problem hiding this comment.
[high] instance output — output.raw stores the extracted scorer answer
_primary_scorer() reads Score.answer, and make_instance() stores it as the complete response. Inspect defines this value as an extracted answer. WILD-raw keeps the full response in assistant turns, but _prompt_from_conversation() discards those turns.
The last non-empty assistant turn can populate output.raw. A missing assistant turn can stay explicit, and a fixture with different full and extracted answers would cover the mapping.
| for (model, task), subs in groups.items(): | ||
| log, developer, model_slug = build_log(model, task, subs, eval_ts, | ||
| retrieved_ts, revision) | ||
| path = save_evaluation_log(log, args.output_dir, developer, model_slug) |
There was a problem hiding this comment.
[medium] repeat runs — Reusing an output directory creates duplicate logical evaluations
save_evaluation_log() creates a new UUID filename on every call. Instance mode also creates another full sidecar. Stable evaluation_id values therefore do not make the filesystem output idempotent.
An adapter-owned aggregate and sidecar can be replaced as one set. The sidecar needs a fresh write because its current append mode would otherwise duplicate rows.
| revision. Warns loudly rather than silently degrading to a mutable ref.""" | ||
| if parquet: # local files: no remote revision to pin | ||
| return None, None | ||
| try: |
There was a problem hiding this comment.
[medium] local provenance — Local parquet input is recorded as remote main
resolve_source_revision() returns no revision for local files. build_log() then records revision or HF_REVISION, while the timestamp path falls back to the current time. The same local input can therefore report false remote provenance and get a new identity on each run.
Unknown local revisions can stay unset. Requiring an explicit evaluation timestamp would keep durable local runs stable.
- sample_hash: use the canonical cross-adapter recipe (sha256 over canonical
JSON of {"raw","reference"}) instead of a bespoke string concat, so items
join across adapters for the same example
- output.raw: emit the model's full generation (assistant turn) rather than
the scorer's parsed answer; split conversation into prompt vs generation
(prompt is user+system only -> answer-free, hashes across models)
- provenance: don't stamp dataset_revision='main' for local --parquet runs
(a false remote-provenance claim); record a local-source marker instead
|
Thanks for the review — pushed a commit addressing these:
|
mrshu
left a comment
There was a problem hiding this comment.
⚒️ review-anvil report
Review decision: COMMENT — The advertised instance mode still fails after writing partial output.
Result: One high and ten medium concerns remain. Five earlier concerns are fixed.
Scope: This PR adds a WILD-raw aggregate and optional item-level adapter.
Checks: 11 concerns checked; 8 confirmed and 3 narrowed.
Second check: targeted, 2 reviewers; 11 kept, 5 fix paths clarified, 0 set aside, 0 removed.
Earlier review comments
Earlier review comments (14 items)
- The dependency declaration, scorer pairing, standard-error test, and false local
mainlabel are fixed. - Missing-response handling, revision pinning, repeat publication, local identity, and overall-instance linkage remain present.
- Existing discussions remain the place for those carry-forward items. This review does not create duplicate inline threads.
What I noticed
- RAV-RUN2-R1-F001 [high] instance-publication
utils/wild/adapter.py:528— Instance mode passes a sidecar basename where the schema requires a canonical datastore path. Construction fails after aggregate and sidecar files are written. (inline) - RAV-RUN2-R1-F002 [medium] documentation
utils/wild/README.md:32— The smoke command passes a directory to a file-only validator. Its output root also cannot satisfy datastore path checks. (inline) - RAV-RUN2-R1-F003 [medium] memory-use
utils/wild/adapter.py:142— Capped instance runs still materialize all selected columns for a complete row group before the cap is checked. (inline) - RAV-RUN2-R1-F004 [medium] data-validation
utils/wild/adapter.py:157— Missing scores become incorrect answers and stay in aggregate denominators. Missing token data becomes measured zero instead of being omitted. (inline) - RAV-RUN2-R1-F005 [medium] metric-identity
utils/wild/adapter.py:210— Each task receives a different metric ID even though every result is canonical accuracy. Cross-source joins by metric ID become fragmented. (inline) - RAV-RUN2-R1-F006 [medium] instance-linkage
utils/wild/adapter.py:367— Multi-subtask rows contribute to overall and leaf results, but instances link only to leaves. This conflicts with the one-record-per-contributing-result guidance. (earlier comment remains open) - RAV-RUN2-R1-F007 [medium] instance-output
utils/wild/adapter.py:383— When no assistant response exists, the adapter substitutes an extracted answer intooutput.raw. Scorer data is then labeled as complete model output. (earlier comment remains open) - RAV-RUN2-R1-F008 [medium] revision-pinning
utils/wild/adapter.py:471— When metadata lookup fails without--revision, both remote passes continue against mutablemain. (earlier comment remains open) - RAV-RUN2-R1-F009 [medium] local-provenance
utils/wild/adapter.py:478— Local runs without--evaluation-timestampkeyevaluation_idon current time. Identical reruns receive different logical identities. (earlier comment remains open) - RAV-RUN2-R1-F010 [medium] repeatability
utils/wild/adapter.py:510— Reruns create fresh UUID files for stable evaluation IDs. Incremental writes also leave partial batches when later work fails. (earlier comment remains open) - RAV-RUN2-R1-F011 [medium] cli
utils/wild/adapter.py:546— A bare--parquetflag yields an empty list. The adapter silently starts the full remote 15-shard conversion. (inline)
ID legend: RUN is the observed PR review run, R is the immutable origin round, F is a finding, and P is a plan.
Things to try (10 items)
- [high] publication — Complete output can stage before publication. Existing logical output can be rejected until replacement is atomic. Sidecar validation and hashing must stay streaming for WILD's size. (
RAV-RUN2-R1-P001; coversRAV-RUN2-R1-F001,RAV-RUN2-R1-F010) - [medium] documentation — Smoke output can use the required
data/<collection>path. The validator can receive a quoted fixed-depth file glob. (RAV-RUN2-R1-P002; coversRAV-RUN2-R1-F002) - [medium] memory-use — Bounded record batches can flush and hash incrementally. (
RAV-RUN2-R1-P003; coversRAV-RUN2-R1-F003) - [medium] data-validation — Rows with invalid scores can enter the failure report. Incomplete token usage can be omitted, with token means based only on complete values. (
RAV-RUN2-R1-P004; coversRAV-RUN2-R1-F004) - [medium] metric-identity —
accuracycan remain the metric ID whileevaluation_namedistinguishes tasks. (RAV-RUN2-R1-P005; coversRAV-RUN2-R1-F005) - [medium] instance-linkage — Overall-leaf pairs can emit as one unit. The cap can stop before a pair that does not fit. (
RAV-RUN2-R1-P006; coversRAV-RUN2-R1-F006) - [medium] instance-output — Missing assistant output can remain empty. Attribution can name the actual answer or scorer source. (
RAV-RUN2-R1-P007; coversRAV-RUN2-R1-F007) - [medium] revision-pinning — Default remote runs can stop when a concrete revision cannot resolve. Verified immutable overrides can remain usable. (
RAV-RUN2-R1-P008; coversRAV-RUN2-R1-F008) - [medium] local-provenance — Local parquet runs can require an explicit evaluation timestamp. (
RAV-RUN2-R1-P009; coversRAV-RUN2-R1-F009) - [medium] cli —
--parquetcan require at least one path. (RAV-RUN2-R1-P010; coversRAV-RUN2-R1-F011)
Run details
- Target: PR #203 at
6c8e3b20a4fbff2d332cab3ccbe544bac618f87a(7 files, +855/-1) - Run ordinal: 2
- Rounds: 1/1 completed; adaptive off; material findings remain
- Mix: 3 codex-exec
- Focus: correctness, maintainability, simplicity, production blast-radius, and constructive optional suggestions
- Earlier review comments: 14 ledger entries; fixed and carry-forward items revalidated
- Finding counts: 0 critical, 1 high, 10 medium, 0 low, 0 nit
- Checks: concerns=11; confirmed=8/ruled-out=0/set-aside=0/narrowed=3
- Second check: targeted; reviewers=2; kept=11/clarified=5/set-aside=0/removed=0; approval changed no
- Fixes applied: 0 (review-only)
Reviewed with review-anvil.
| for key, count in counts.items(): | ||
| if not count: | ||
| continue | ||
| path = agg_paths[key] |
There was a problem hiding this comment.
RAV-RUN2-R1-F001 [high] instance-publication — Instance mode passes a sidecar basename where the schema requires a canonical datastore path. Construction fails after aggregate and sidecar files are written.
Complete output can stage before publication. Existing logical output can be rejected until replacement is atomic, with sidecar validation kept streaming.
|
|
||
| ```bash | ||
| uv run python -m utils.wild.adapter --output-dir /tmp/eee-wild --limit-shards 1 | ||
| uv run python -m every_eval_ever validate /tmp/eee-wild |
There was a problem hiding this comment.
RAV-RUN2-R1-F002 [medium] documentation — This smoke command passes a directory to a file-only validator. Its output root also cannot satisfy datastore path checks.
Smoke output can use the required data/<collection> path, and validation can receive a quoted fixed-depth file glob.
| with opener() as fh: | ||
| pf = pq.ParquetFile(fh) | ||
| for rg in range(pf.num_row_groups): | ||
| tbl = pf.read_row_group(rg, columns=columns) |
There was a problem hiding this comment.
RAV-RUN2-R1-F003 [medium] memory-use — Capped instance runs still materialize all selected columns for a complete row group before the cap is checked. The cap does not bound this allocation.
Bounded record batches can flush and hash incrementally.
| in_tok: int = 0 | ||
| out_tok: int = 0 | ||
|
|
||
| def add(self, score, in_t, out_t): |
There was a problem hiding this comment.
RAV-RUN2-R1-F004 [medium] data-validation — Missing scores become incorrect answers and stay in aggregate denominators. Missing token data becomes measured zero instead of being omitted.
Invalid scores can enter the failure report. Incomplete token usage can be omitted, with token means based only on complete values.
| return EvaluationResult( | ||
| evaluation_result_id=rid, | ||
| evaluation_name=name, | ||
| source_data=_source_data(task, agg.n, subtask), |
There was a problem hiding this comment.
RAV-RUN2-R1-F005 [medium] metric-identity — Each task receives a different metric ID even though every result is canonical accuracy. Cross-source joins by metric ID become fragmented.
accuracy can remain the metric ID while evaluation_name distinguishes tasks.
|
|
||
| def parse_args() -> argparse.Namespace: | ||
| p = argparse.ArgumentParser(description='Convert kensho/WILD-raw to Every Eval Ever.') | ||
| p.add_argument('--parquet', nargs='*', default=None, |
There was a problem hiding this comment.
RAV-RUN2-R1-F011 [medium] cli — A bare --parquet flag yields an empty list. The adapter treats that as no local input and silently starts the full remote 15-shard conversion.
The option can require at least one path.
Relocates the adapter to every_eval_ever/adapters/wild/ for the post-evaleval#218 layout, moves its README row to the adapters table, and relocks the wild extra.
Publication (F001, F010, P001): stage every sidecar into a temporary datastore tree, link it with the canonical repository-relative path, and publish through the shared publish_evaluation_logs, which re-validates and re-checksums the staged bytes and rolls back what it created. Publication is per log so peak memory stays at one model x benchmark, and files created by earlier logs are removed if a later one fails. A populated output directory is now an error rather than a second copy of every evaluation_id; --replace-existing removes the previous set, and only once the replacement is fully staged. Memory (F003): read bounded record batches instead of whole row groups. A WILD shard is a single 500,000-row row group, so --max-instances previously still materialized every selected column for all of them. Data validation (F004): a row whose score is not a usable 0/1 enters the failure report and leaves the aggregate instead of counting as a wrong answer; incomplete token usage is omitted rather than measured as zero, and token means cover only the rows that carried usage. The run exits non-zero after publishing whatever was valid. Metric identity (F005): every result keeps the canonical `accuracy` metric id; evaluation_name distinguishes the tasks. Instance output (F007): output.raw holds the assistant turns only, empty when there are none, and answer_attribution.source names where the parsed answer came from. Provenance (F008, F009): a remote run stops rather than falling back to the mutable main ref, and a local run without --evaluation-timestamp stops rather than keying evaluation_id on now(). CLI (F011): --parquet requires at least one path. Docs (F002): smoke commands use a data/<collection> output path and a quoted fixed-depth file glob.
mrshu
left a comment
There was a problem hiding this comment.
⚒️ review-anvil report
Review decision: COMMENT — This pass is comment-only, and replacement publication still risks losing valid prior output.
Result: The adapter now resolves most earlier review items well. Four focused suggestions remain for publication, pinning, and CLI safety.
Scope: Add a streaming WILD-raw adapter for aggregate and optional item-level Inspect AI records.
Checks: 4 concerns checked; 3 confirmed and 1 ruled out.
Second check: 2 reviewers checked 4 findings; all 4 stayed, and 3 fix paths were clarified.
Earlier review comments (14 grouped concerns)
- Still present: replacement publication (
RAV-RUN2-R1-F010) and metadata-failure pinning (RAV-RUN2-R1-F008) remain relevant at the moved adapter path. - No longer relevant: instance linkage (
RAV-RUN2-R1-F006) was checked against the current schema guidance. The documented leaf-only choice is permitted. - Fixed: sidecar paths, scorer/output semantics, local provenance, memory bounds, score and token validation, canonical metric identity,
--parquet, dependency packaging, tests, and README validation commands.
What I noticed
| ID | Priority | Topic | Code location | What I noticed |
|---|---|---|---|---|
| RAV-RUN2-R1-F010 | high | publication | every_eval_ever/adapters/wild/adapter.py:660 |
Replacement deletes every prior file for selected model directories before new publication. Partial inputs can remove absent benchmarks, and later failures cannot restore deleted files. |
| RAV-RUN2-R1-F008 | medium | revision pinning | every_eval_ever/adapters/wild/adapter.py:516 |
After metadata failure, an explicit symbolic revision can reach both remote passes when an evaluation timestamp is also supplied. |
| RAV-RUN3-R1-F001 | medium | models CLI | every_eval_ever/adapters/wild/adapter.py:678 |
A bare --models disables filtering. A fully unmatched filter exits successfully after publishing zero logs. (inline) |
| RAV-RUN3-R1-F002 | medium | output path | every_eval_ever/adapters/wild/adapter.py:608 |
An arbitrary output path loses its final component, so publication and replacement can target a different wild directory. (inline) |
Things to try
- [high] publication — New output can be staged and checked while prior adapter-owned files remain recoverable. Replacement can use logical evaluation identity and preserve unrelated tasks. (
RAV-RUN3-R1-P001; coversRAV-RUN2-R1-F010) - [medium] revision pinning — Symbolic refs can resolve once to
info.sha. A metadata-failure fallback can accept only a full SHA with an explicit timestamp. (RAV-RUN2-R1-P008; coversRAV-RUN2-R1-F008) - [medium] models CLI —
--modelscan require one value. A selection with no source matches can fail before stale-file discovery or publication. (RAV-RUN3-R1-P002; coversRAV-RUN3-R1-F001) - [medium] output path — The documented
data/wildshape can be validated before metadata lookup, reads, or replacement scans. (RAV-RUN3-R1-P003; coversRAV-RUN3-R1-F002)
Run details
- Target: PR #203 (
add-wild-adapter, 7 files, +1137/-1) ated99266c74958f79249a2d0ac68618c0b76b7f8b - Run ordinal: 3
- Rounds: 1/1 completed; adaptive off in review-only mode
- Mix: 3 codex-exec
- Focus: correctness, maintainability, simplicity, production blast radius, and constructive suggestions
- Earlier review comments: 14 grouped concerns; 2 still present, 1 ruled out, and 11 fixed
- Finding counts: 0 critical, 1 high, 3 medium, 0 low, 0 nit
- Checks: concerns=4; confirmed=3, ruled-out=1
- Second check: targeted; reviewers=2; kept=4, clarified=3, removed=0; approval unchanged
Reviewed with review-anvil.
| p.add_argument('--output-dir', type=Path, default=Path(DEFAULT_OUTPUT_DIR)) | ||
| p.add_argument('--limit-shards', type=int, default=None, | ||
| help='Only read the first N shards (for smoke runs).') | ||
| p.add_argument('--models', nargs='*', default=None, help='Filter to these model ids.') |
There was a problem hiding this comment.
RAV-RUN3-R1-F001 [medium] cli — A bare --models parses as an empty list, which disables filtering and starts the full source run. A nonempty filter with no matches instead exits successfully with zero logs.
Requiring one or more values would block the first path. A no-match check can stop before existing-output discovery or publication.
There was a problem hiding this comment.
Both fixed in a5bef76. --models is nargs='+', so a bare flag is an argument error instead of [] → no filtering → a full remote conversion. An entirely unmatched selection raises straight after aggregation — before the replacement scan and before publication — naming the ids, since nothing would be published and that is a typo rather than an empty refresh; a partial match warns and publishes what matched. test_models_filter_matching_nothing_publishes_nothing, test_bare_models_flag_is_an_error. Full reply
| file_uuids.append(str(uuid.uuid4())) | ||
|
|
||
| # Checked before the instance pass so a rejected rerun costs nothing. | ||
| stale = existing_records(args.output_dir.parent, logs) |
There was a problem hiding this comment.
RAV-RUN3-R1-F002 [medium] output-path — The code passes only args.output_dir.parent to lookup and publication, then derives wild below it. An arbitrary path such as /tmp/intended therefore targets /tmp/wild while messages name the requested path.
Early validation of the documented data/wild shape would keep lookup, replacement, and status output on one destination.
There was a problem hiding this comment.
Fixed in a5bef76. resolve_base_output_dir() is now the first statement in run() — ahead of the metadata lookup, any parquet read, and the replacement scan — and requires --output-dir to end in the collection directory publication writes into. Your /tmp/i... case is exactly it: the run would have published into <parent>/wild/ while every message named the directory asked for, and the replacement scan would have read that other directory too. test_output_dir_must_be_the_collection_directory also asserts the rejected run wrote nothing. Full reply
|
Thanks — this was a useful pass. All eleven findings are addressed in P001 / F001 / F010 — publicationRewritten to go through the shared machinery rather than writing sidecars in place:
One deliberate divergence in how: I publish per log rather than one batched F003 — memoryReads are now bounded by F004 — score and token handling
Worth stating plainly: this is a guard, not a repair of observed corruption. I F005 — metric identityEvery result now carries the registry's canonical F006 — instance linkage → declining, with a comment in the codeInstances still attach to the finest-grain result only. The skill's The rest
Verification
One thing this breaks downstreamThe companion datastore submission |
…leting Replacement keyed on logical evaluation identity (the evaluation_id without its timestamp) read from each prior file, instead of globbing every file in the selected model directories: a run over one benchmark no longer removes the other benchmarks that share the model's directory. The removal also moves after publication, so a run that fails partway leaves the previous refresh whole. A metadata-lookup failure now accepts an explicit --revision only when it is a 40-character SHA; a branch or tag can move between the aggregate and instance passes, so it cannot stand in for the pin the lookup failed to produce. --output-dir must end in the collection directory, validated before any lookup, read or replacement scan. --models takes nargs='+' so a bare flag errors rather than silently converting every model, and a selection the source cannot match at all is an error rather than an empty success.
|
Run 3's four findings are fixed in a5bef76 (CI green on all four jobs). Two of them RAV-RUN2-R1-F010 · publication — replacement could delete what it wasn't replacingWhat the code actually did: stale = existing_records(args.output_dir.parent, logs) # dir.glob('*.json*') per model
...
for path in stale:
path.unlink()
published = publish(logs, file_uuids, args.output_dir.parent, staging_root)Both halves of your finding follow from those three lines. The scan was keyed on the Fixed along the fix path you gave — logical evaluation identity, unrelated tasks
Two tests: RAV-RUN2-R1-F008 · revision pinning — a symbolic ref could outlive the failureAlso correct, and also mis-reported by me: I fixed the no-override branch — which The metadata-failure fallback now accepts an override only when it is 40 hex RAV-RUN3-R1-F001 · models CLIBoth parts, in the order you asked for them. RAV-RUN3-R1-F002 · output path
Verification
|
15 of WILD's 65 model ids carry no namespace ("gpt-4o", "nova-pro",
"llama-3.1-8b"). build_log derived the developer from the id prefix, so those
became 'unknown', which the datastore path helper rejects — the full 65-model
run aborted after aggregating all 1,755 groups and published nothing. Delegate
to every_eval_ever.helpers.developer.get_developer, which keeps the prefix for a
namespaced id and pattern-matches a flat one, and stop the run naming the
offending ids if any still resolve to nothing.
Two tests cover the four publishers the flat ids resolve to and the refusal;
_task_rows is shared with the existing single-task fixture.
|
Ran the adapter at full scale for the first time and it did not work. Fixed in The full run published nothing
developer = model.split('/')[0] if '/' in model else 'unknown'15 of WILD's 65 model ids carry no namespace — Not a regression — The fix delegates to
First full-scale run7,237,945 item rows → 1,755 aggregates across 65 models and 17 publishers in 68 s, Correcting the record on discussions/174I called it "generated by the pre-review adapter" and named three missing fields. It is
The missing deployment axes mean every record there fails the current validator — the So the regeneration replaces all 1,755 rather than patching the 405 — every record One thing it does not change: the draft is aggregate-only, and so is the |
It is a usage note, not a registry follow-up.
⚒️ review-anvil reportReview decision: COMMENT — This run is comment-only; one earlier high-priority publication issue remains. Earlier review comments (13 canonical items)
What I noticed
Things to try
Run details
Reviewed with review-anvil. |
What / source
Adds
every_eval_ever/adapters/wild/— an adapter for WILD-raw(
kensho/WILD-raw, arXiv:2604.01418): item-level eval responses for 65 models ×27 benchmarks (~7.5M
(model, item)rows), run by Kensho with Inspect AI.Grain. One aggregate
EvaluationLogper (model, benchmark) — the overallaccuracy (
wild.<task>) plus one result per subtask (wild.<task>.<subtask>), eacha
continuous [0,1]mean of the binary item scores with an analytic proportionstandard_error+num_samples. A benchmark with ≤1 distinct subtask emits only theoverall (no byte-identical duplicate leaf). Every result carries the registry's
canonical
accuracymetric id; the task lives inevaluation_name, so thecross-source accuracy join stays whole.
--include-instancesadditionally writes theper-item
<uuid>_samples.jsonlsidecar linked fromdetailed_evaluation_results.source_type=evaluation_run,evaluator_relationship=third_party,eval_library=inspect_ai(paper-confirmed).source_datanames each benchmark'sown dataset repo (
mmlu→cais/mmlu,arc_*→allenai/ai2_arc,finance_fundamentals→kensho/bizbench, …; each verified on HF), not WILD-raw —WILD-raw is the results source and lives in
source_metadata.input.raw= the prompt turns only (no answer leak, so it hashesidentically across models);
output.raw= the assistant turn(s), i.e. the model'sfull generation, empty when the row has none; the scorer's parsed answer goes to
answer_attribution.extracted_value, withsourcenaming where it came from andextraction_method= the real Inspect scorer.sample_hashuses the sharedcross-adapter recipe. Instances link to the finest-grain result only — every item
belongs to exactly one subtask, so also linking the overall would duplicate ~7.5M
rows for no new information (
reference/instance-level.mdsanctions leaf-only witha comment, which the code carries).
shared
publish_evaluation_logs: sidecars are staged into a temporary datastoretree, re-validated and re-checksummed, and nothing is left behind if any log fails.
A populated output directory is an error rather than a second copy of every
evaluation_id;--replace-existingsupersedes only the prior copies of the(model, benchmark) pairs this run rewrites — matched on the logical
evaluation_idread out of each file, and removed only once the replacements are published — so a
run whose input covers just some of a model's benchmarks leaves the rest of that
model's directory intact, and one that fails partway leaves the previous refresh
whole.
scoreis not a usable 0/1, and rows with incomplete token usage, arehandled rather than silently absorbed: the former enter
adapter_reports/wild_failures.jsonand leave the denominator (the command thenexits non-zero), the latter are omitted from
token_usageand from the token meansinstead of being measured as zero.
Review lane
under ~1000 hand-written lines
change in outcome, or large agent-authored change
Scoped to one adapter and structurally additive — publication reuses
every_eval_ever/converters/common/publication.pyandhelpers.ioas they alreadyexist — but it is over the Fast lane on size: a 786-line module, 143 lines of README
and 425 lines of tests. Four review rounds have run on it.
Checklist
python -m every_eval_ever validateclean — no warnings — at the finaldata/wild/<dev>/<model>/path, on the whole conversion: 1,755 aggregates,0 invalid, 0 errors, 0 warnings, semantic checks on
adapter_reports/, and the command exitsnon-zero (
SourceConversionResult.raise_if_incomplete, report written beforepublication so a publication error cannot take the accounting with it)
pytest testsgreen(409 passed, 20 skipped; 431 with current
mainmerged in)ruff checkclean--output-dirmust be the
data/wildcollection directory publication writes into, and--modelsrequires at least one value and fails when the source matches noneof them rather than publishing nothing successfully
helpers.developer— 15 of the 65 ids carry no namespace, so a prefix splitcannot name one; benchmark canonicalization is a registry follow-up (enumerated
in the adapter README), not a blocker here
input.raw, instances notdouble-counted across overall+leaf,
evaluation_idkeyed on the pinned sourcecommit date (stable across reruns; no
now()fallback)Decisions & coverage
publish().Chose / instead of: one
publish_evaluation_logscall for all ~1,755 logs.Why: the shared publisher buffers a batch's bytes before creating any file, so one
call would hold the entire sidecar corpus in memory. Per-log calls bound that to one
model×benchmark, and
publish()removes anything earlier logs created if a laterone fails, so the run still leaves the whole conversion or nothing.
Confidence: med. General? yes — any item-level source large enough to matter
hits this; a streaming/batched mode in the shared publisher would remove the need
for each adapter to solve it.
make_instance.Chose / instead of: also emitting them under the benchmark overall (~2× rows).
Confidence: high (explicitly one of the two sanctioned options). General? no.
--parquetrun now requires--evaluation-timestamp.Chose / instead of:
time.time().evaluation_idis keyed on this value, so anow()fallback gives identical reruns different logical identities.Confidence: high. General? yes — the same trap exists in any adapter whose id
includes a timestamp it cannot read from the source.
Chose / instead of: reading the mutable
mainref (which would let the aggregateand instance passes read different data).
--revision <sha>pins it explicitly.If the metadata lookup fails with a
--revisionin hand, that value is acceptedonly when it is a 40-character SHA — a branch or tag can still move between the two
passes — and since the lookup was also the source of the commit date,
--evaluation-timestampthen becomes required rather than guessed.Confidence: high. General? yes.
Coverage: the adapter converts every item row it reads. Verified end to end on the
whole source, pinned to
06af2cc48c351a9c67b9caec4dff7d1c43d451dd: 7,237,945 sourcerows → 1,755 logs across 65 models and 17 publishers, 0 dropped, 0 failures, in 68 s.
All 1,755 validate clean at their datastore paths with semantic checks on, and produce
no warnings under the registry-alias check in #230 either. A full-corpus scan of shard 0
found 0 rows with a missing score and 0 with missing token counts, so the score and
token guards are defensive rather than a repair of observed corruption.
Operator asked about policy calls? Re-hosting large data: instance sidecars stay
behind
--include-instancesand the companion datastore PR ships aggregates only(1,755 records) — WILD-raw is already public, so the ~7.5M item rows are not
re-hosted by default. New canonical benchmark ids (
squad,paws,chembench,finance_fundamentals,pre_flight) and thearc_easy/arc_challenge→AI2-ARC aliases go through the registry, not this PR.
Companion work (paired): data
https://huggingface.co/datasets/evaleval/EEE_datastore/discussions/174 · registry
benchmark ids — not yet opened; the seven aliases/canonicals are enumerated in
wild/README.mdunder Benchmarkcanonicalization. This PR does not depend on it:
evaluation_nameiswild.<task>[.<subtask>]andsource_datacarries each benchmark's dataset repo, sothe registry change is additive.
The datastore PR is still a draft, and its 1,755 records predate this review round
on six counts:
schema_version0.2.2, per-task metric ids(
wild.winogrande.accuracy), nodeployment_type/model_availability(so every recordthere fails the current validator), no pinned
dataset_revision, non_items_with_token_usage, and 405 records filed underdata/wild/unknown/— theflat-id bug, published. It is regenerated in full from this adapter before the set
merges;
evaluation_idis unchanged by any of the fixes, sological_identity()supersedes each
wild/<model>/<benchmark>pair rather than adding a second copy.