-
Notifications
You must be signed in to change notification settings - Fork 44
Test that a converted record actually passes the merge gate #244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
borgr
wants to merge
4
commits into
main
Choose a base branch
from
tests/converter-merge-gate
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
1267c92
Test that a converter's output still passes the merge gate
borgr c03e845
Fail the conversion test on two results that share a join key
borgr 78f9606
Merge remote-tracking branch 'upstream/main' into pr244-fix
borgr 9258a74
Point helm converter case at renamed dash-form fixture dir
borgr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,212 @@ | ||
| """Shared machinery for testing a converter against a committed upstream log. | ||
|
|
||
| One `ConverterCase` per converter is all it takes to be covered by | ||
| `tests/test_converter_conversion.py`: point it at a real log the upstream tool wrote, | ||
| give the CLI arguments a user would give, and state what the conversion should yield. | ||
|
|
||
| Adding a converter here does not require writing a test. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import importlib | ||
| import json | ||
| from dataclasses import dataclass | ||
| from pathlib import Path | ||
| from typing import Any | ||
|
|
||
| REPO_ROOT = Path(__file__).resolve().parents[1] | ||
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class ConverterCase: | ||
| """One converter, one committed upstream log, and what converting it must yield.""" | ||
|
|
||
| source: str | ||
| log_path: Path | ||
| # What the conversion is expected to produce. Kept small on purpose: enough that a | ||
| # silently dropped score, task, or sidecar fails, without restating the whole record. | ||
| aggregates: int | ||
| sidecars: int = 0 | ||
| # Total `evaluation_results` across every aggregate. For a converter that emits more | ||
| # results than are worth listing in `scores`, this is what catches one going missing. | ||
| results: int | None = None | ||
| model_id: str | None = None | ||
| # Keyed by `<evaluation_name>/<metric>`, since one task can be scored by several | ||
| # metrics and each becomes its own result. | ||
| scores: dict[str, float] | None = None | ||
| extra_argv: tuple[str, ...] = () | ||
| # Upstream key paths the converter cannot work without, `*` matching any one key. | ||
| required_source_paths: tuple[str, ...] = () | ||
|
|
||
| @property | ||
| def id(self) -> str: | ||
| return self.source | ||
|
|
||
| def source_payload(self) -> Any: | ||
| return json.loads(self.log_path.read_text(encoding='utf-8')) | ||
|
|
||
|
|
||
| CASES: tuple[ConverterCase, ...] = ( | ||
| ConverterCase( | ||
| source='lm_eval', | ||
| log_path=REPO_ROOT | ||
| / 'tests/data/lm_eval/results_2026-01-21T03-44-18.458309.json', | ||
| aggregates=2, | ||
| # Two tasks, one `exact_match` result each. Stated even though `scores` | ||
| # already lists both, because `scores` is a dict: without a count, a | ||
| # third result that collided on an existing key would be merged away. | ||
| results=2, | ||
| # The fixture ships a samples file for only one of its two tasks, so | ||
| # --include_samples would (correctly) report a partial conversion. | ||
| model_id=( | ||
| 'RylanSchaeffer/mem_Qwen3-93M_minerva_math_rep_0_sbst_1.0000_epch_1_ot_1' | ||
| ), | ||
| scores={ | ||
| 'math_perturbed_full/exact_match': 0.0, | ||
| 'math_rephrased_full/exact_match': 0.0004, | ||
| }, | ||
| required_source_paths=( | ||
| 'config.model', | ||
| 'config.model_args', | ||
| 'results.*', | ||
| 'configs.*.dataset_path', | ||
| ), | ||
| ), | ||
| ConverterCase( | ||
| source='inspect', | ||
| log_path=REPO_ROOT | ||
| / 'tests/data/inspect/data_cyse2_vuln_exploit_challenges.json', | ||
| aggregates=1, | ||
| sidecars=1, | ||
| # One scorer reporting three metrics, which is what makes this fixture worth | ||
| # using: a converter that collapses them to one result fails here. | ||
| results=3, | ||
| model_id='mistral/mistral-large-latest', | ||
| required_source_paths=( | ||
| 'eval.model', | ||
| 'eval.task', | ||
| 'eval.dataset.name', | ||
| 'results.scores.*.name', | ||
| 'results.scores.*.scorer', | ||
| 'results.scores.*.metrics', | ||
| 'results.total_samples', | ||
| ), | ||
| ), | ||
| ConverterCase( | ||
| source='helm', | ||
| log_path=REPO_ROOT | ||
| / 'tests/data/helm' | ||
| / 'commonsense-dataset=hellaswag,method=multiple_choice_joint,' | ||
| 'model=eleutherai_pythia-1b-v0', | ||
| aggregates=1, | ||
| sidecars=1, | ||
| # Eight metrics on the `valid` split, each also reported worst-case over the | ||
| # robustness and fairness perturbations. | ||
| results=24, | ||
| model_id='eleutherai/pythia-1b-v0', | ||
| # `--log_path` is a HELM run directory, not one file, so there is no single | ||
| # payload for `missing_paths` to address. The gate and the counts above are | ||
| # what cover this converter. | ||
| required_source_paths=(), | ||
| ), | ||
| ) | ||
|
|
||
|
|
||
| def unavailable(case: ConverterCase) -> str | None: | ||
| """Why this case cannot run here, or None if it can. | ||
|
|
||
| A converter behind an optional extra states its own missing dependency in an | ||
| `_..._IMPORT_ERROR` module global and raises from it when used. Reading that is what | ||
| lets a case be declared once and skip, rather than fail, in the core install. | ||
| """ | ||
| module = importlib.import_module( | ||
| f'every_eval_ever.converters.{case.source}.adapter' | ||
| ) | ||
| for name, value in vars(module).items(): | ||
| if name.endswith('_IMPORT_ERROR') and value is not None: | ||
| return ( | ||
| f'{case.source} converter dependencies are missing: {value!r}. ' | ||
| f'Install with: uv sync --extra {case.source}' | ||
| ) | ||
| return None | ||
|
|
||
|
|
||
| def convert(case: ConverterCase, tmp_path: Path) -> list[Path]: | ||
| """Run the real CLI over a case's log; return the published record files.""" | ||
| from every_eval_ever import cli | ||
|
|
||
| data_dir = tmp_path / 'data' | ||
| exit_code = cli.main( | ||
| [ | ||
| 'convert', | ||
| case.source, | ||
| '--log_path', | ||
| str(case.log_path), | ||
| '--output_dir', | ||
| str(data_dir), | ||
| '--source_organization_name', | ||
| 'every-eval-ever-tests', | ||
| *case.extra_argv, | ||
| ] | ||
| ) | ||
| assert exit_code == 0, f'{case.source} conversion exited {exit_code}' | ||
| return sorted( | ||
| path | ||
| for path in data_dir.rglob('*') | ||
| if path.is_file() and path.suffix in {'.json', '.jsonl'} | ||
| ) | ||
|
|
||
|
|
||
| def gate_complaints(paths: list[Path], capsys) -> list[dict[str, Any]]: | ||
| """Run the validator CLI over `paths`; return every error and warning it reports. | ||
|
|
||
| This is the merge gate: the semantic checks only run for a file at a canonical | ||
| `data/<collection>/<developer>/<model>/` path, which is what the converters | ||
| publish to, and `validate_file()` on its own leaves them off. | ||
| """ | ||
| from every_eval_ever.validate import main as validate_main | ||
|
|
||
| capsys.readouterr() # drop what the conversion printed | ||
| exit_code = validate_main( | ||
| [str(path) for path in paths] + ['--format', 'json'] | ||
| ) | ||
| reports = json.loads(capsys.readouterr().out) | ||
| complaints = [ | ||
| { | ||
| 'file': report['file'], | ||
| 'errors': report['errors'], | ||
| 'warnings': report['warnings'], | ||
| } | ||
| for report in reports | ||
| if not report['valid'] or report['errors'] or report['warnings'] | ||
| ] | ||
| if exit_code != 0 and not complaints: | ||
| complaints.append( | ||
| { | ||
| 'file': '<all>', | ||
| 'errors': ['validate exited non-zero'], | ||
| 'warnings': [], | ||
| } | ||
| ) | ||
| return complaints | ||
|
|
||
|
|
||
| def missing_paths(payload: Any, paths: tuple[str, ...]) -> list[str]: | ||
| """Return the declared key paths that the given source payload does not have.""" | ||
|
|
||
| def resolve(node: Any, parts: list[str]) -> bool: | ||
| if not parts: | ||
| return True | ||
| head, rest = parts[0], parts[1:] | ||
| if head == '*': | ||
| if isinstance(node, dict): | ||
| return any(resolve(value, rest) for value in node.values()) | ||
| if isinstance(node, list): | ||
| return any(resolve(item, rest) for item in node) | ||
| return False | ||
| if isinstance(node, dict) and head in node: | ||
| return resolve(node[head], rest) | ||
| return False | ||
|
|
||
| return [path for path in paths if not resolve(payload, path.split('.'))] | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,124 @@ | ||
| """Convert a committed upstream log with the real CLI and put it through the gate. | ||
|
|
||
| Someone ran an evaluation: can this repo still turn that output into a record the | ||
| datastore accepts? Each case converts a log the upstream tool wrote — never a | ||
| hand-built one — and requires the published files to pass `validate` with the semantic | ||
| checks on and no warnings, since a warning here is one every user inherits. | ||
|
|
||
| A *new version* of the upstream tool is out of reach here; that is what a | ||
| `tools/upstream_smoke/` script covers, on a schedule. Neither layer checks semantics: a | ||
| metric that changes from a percentage to a proportion upstream passes both. | ||
|
|
||
| Cases live in `tests/converter_cases.py`. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import json | ||
|
|
||
| import pytest | ||
|
|
||
| from tests.converter_cases import ( | ||
| CASES, | ||
| ConverterCase, | ||
| convert, | ||
| gate_complaints, | ||
| missing_paths, | ||
| unavailable, | ||
| ) | ||
|
|
||
| CASE_IDS = [case.id for case in CASES] | ||
|
|
||
|
|
||
| @pytest.fixture(params=CASES, ids=CASE_IDS) | ||
| def case(request) -> ConverterCase: | ||
| reason = unavailable(request.param) | ||
| if reason is not None: | ||
| pytest.skip(reason) | ||
| return request.param | ||
|
|
||
|
|
||
| def test_conversion_passes_the_merge_gate(case, tmp_path, capsys): | ||
| """Everything the converter publishes must be submittable as-is.""" | ||
| paths = convert(case, tmp_path) | ||
| complaints = gate_complaints(paths, capsys) | ||
|
|
||
| assert not complaints, ( | ||
| f'{case.source} conversion no longer passes the merge gate:\n' | ||
| + json.dumps(complaints, indent=2) | ||
| + f'\nFix every_eval_ever/converters/{case.source}/, or the schema, ' | ||
| 'validator or publisher change that caused it.' | ||
| ) | ||
|
|
||
|
|
||
| def test_conversion_yields_the_expected_records(case, tmp_path): | ||
| """Guard against a conversion that validates but quietly loses data.""" | ||
| paths = convert(case, tmp_path) | ||
| aggregates = [path for path in paths if path.suffix == '.json'] | ||
| sidecars = [path for path in paths if path.suffix == '.jsonl'] | ||
|
|
||
| assert len(aggregates) == case.aggregates, ( | ||
| f'{case.source} produced {len(aggregates)} aggregate record(s), ' | ||
| f'expected {case.aggregates}: {[path.name for path in aggregates]}' | ||
| ) | ||
| assert len(sidecars) == case.sidecars, ( | ||
| f'{case.source} produced {len(sidecars)} instance-level sidecar(s), ' | ||
| f'expected {case.sidecars}' | ||
| ) | ||
|
|
||
| logs = [json.loads(path.read_text(encoding='utf-8')) for path in aggregates] | ||
| if case.model_id is not None: | ||
| assert {log['model_info']['id'] for log in logs} == {case.model_id} | ||
| if case.results is not None: | ||
| converted = sum(len(log['evaluation_results']) for log in logs) | ||
| assert converted == case.results, ( | ||
| f'{case.source} converted {converted} result(s), ' | ||
| f'expected {case.results}' | ||
| ) | ||
| if case.scores is not None: | ||
| scored = [ | ||
| ( | ||
| f'{result["evaluation_name"]}/' | ||
| f'{result.get("evaluation_result_id") or result["metric_config"]["evaluation_description"]}', | ||
| result['score_details']['score'], | ||
| ) | ||
| for log in logs | ||
| for result in log['evaluation_results'] | ||
| ] | ||
| # Collected as pairs, not straight into a dict: two results sharing a key | ||
| # is the quietly-lost data this test exists to catch, and a dict would | ||
| # merge them before the comparison below could see the collision. | ||
| keys = [key for key, _ in scored] | ||
| assert len(keys) == len(set(keys)), ( | ||
| f'{case.source} produced two results with the same ' | ||
| f'evaluation_name/evaluation_result_id: ' | ||
| f'{sorted(key for key in set(keys) if keys.count(key) > 1)}' | ||
| ) | ||
| assert dict(scored) == case.scores | ||
|
|
||
| for log, path in zip(logs, aggregates, strict=True): | ||
| detailed = log.get('detailed_evaluation_results') | ||
| if detailed is None: | ||
| continue | ||
| # The sidecar pointer must name the file that was written beside it, since | ||
| # the gate resolves it as a repository path. | ||
| assert detailed['file_path'].endswith(f'{path.stem}_samples.jsonl') | ||
| assert detailed['total_rows'] > 0 | ||
|
|
||
|
|
||
| def test_required_source_paths_are_present_in_the_fixture(case): | ||
| """The keys the converter reads must still exist in the committed log. | ||
|
|
||
| A fixture refreshed from a newer upstream release fails here by name, instead of | ||
| as a stack trace from inside the converter. | ||
| """ | ||
| if not case.required_source_paths: | ||
| pytest.skip(f'{case.source} declares no required source paths') | ||
|
|
||
| absent = missing_paths(case.source_payload(), case.required_source_paths) | ||
|
|
||
| assert not absent, ( | ||
| f'{case.source} reads upstream keys that {case.log_path.name} does not have: ' | ||
| f'{absent}\nEither the fixture came from an incompatible version, or the ' | ||
| f'converter and tests/converter_cases.py disagree about what it reads.' | ||
| ) |
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RAV-RUN1-R1-F002 [medium] optional dependencies — Every stored import error becomes an unavailable skip here. Because Inspect and HELM capture broad exceptions, an installed but incompatible release can be skipped even in full CI. Declaring each optional distribution explicitly would let core skip a genuinely absent package while surfacing an installed package's captured import error.