Move validator business logic into every_eval_ever and update to de-duplication logic by nelaturuharsha · Pull Request #194 · evaleval/every_eval_ever

nelaturuharsha · 2026-06-29T10:18:35Z

Summary: Single source of truth for validation rules added to cli, and dedup operations now are part of the cli. PR to merge is up. README is officially out of date.
Changes:
- Warning when score_type, max_score and min_score are not mentioned.
- score should be between [min_score, max_score]
- If HF model is provided, it should exist - wellness check for both model and dataset.
- Will now check to ensure mandatory metadata under additional details for model_info i.e.
- deployment_type and model_availability are present. Adapters need to be updated to fix this.
- new commands/functionality added to cli
- Users will have to submit deployment_type and model_availability in additional_details which the bot will enforce.
- Three options: api, local, unknown
  - if api: model_availability can be -> closed_source, open_weights_deployment, other
  - if local: model_availability can be. -> hf, unavailable, other
  - if hf -> wellness check will ensure it exists

Ruff/Ty were run which formatted some adapters.

Erotemic

Mostly just a python-level review, I didn't consider anything holistically.

Erotemic · 2026-06-29T18:11:47Z

+    file_fingerprints: dict[str, str] = {}
+    warnings: list[str] = []
+    for file_path in sorted(file_paths):
+        if not file_path.endswith('.json'):


This is a footgun. If you are expecting this to be called with only json paths, and you ever hit this case, you are silently dropping information about some bad pre-filtering done when constructing the content passed to this function. I would either remove this continue or raise an error.

I had this in place because we don't have a good recipe to reliably de-duplicate jsonls at the moment and we have only jsons or jsonls in the target datastore.

Erotemic · 2026-06-29T18:27:24Z

+def compute_fingerprint(content: bytes) -> str:
+    """Compute the semantic duplicate fingerprint for aggregate JSON bytes."""
+    try:
+        data = json.loads(content)


This might end up being a bottleneck. It may be worth it to abstract the specific json reader / writer so the stdlib json is always an option, but you can opt-in to something like orjson or ujson. (Note: orjson is a bit opinionated. It's fast, but it also means you can't serialize generic floats like nans or infs, which technically isn't json, but the stdlib and ujson aren't so strict about it).

I've handled this in a few ways in the past: simple wrappers: https://gitlab.kitware.com/computer-vision/kwutil/-/blob/main/kwutil/util_json.py?blame=1&ref_type=heads#L391

And module level globals: https://gitlab.kitware.com/computer-vision/kwcoco/-/blob/main/kwcoco/coco_dataset.py?blame=1&ref_type=heads#L117

Erotemic · 2026-06-29T18:31:31Z

+    return value
+
+
+def _canon(obj: Any) -> Any:


The recursive implementation could probably be much faster using an iterator and maintaining your own stack: https://gitlab.kitware.com/computer-vision/kwutil/-/blob/main/kwutil/util_json.py?ref_type=heads#L29

A utility I wrote makes this fairly easy to express without too much extra complication: https://github.com/Erotemic/ubelt/blob/main/ubelt/util_indexable.py#L52

Think we're just going fingerprints that we created and not the JSON, would this still be a big performance gap you think?

I'd have to profile it to know, but recursion can get pretty costly. I just tend to avoid it when writing new code. Might be fine the way it is.

FWIW: a quick pass with sample data on both variants with line-profiler will tell you a lot.

yananlong · 2026-07-06T22:38:44Z

 .sass-cache/
 .jekyll-cache/
-.jekyll-metadata
+.jekyll-metadataUSAGE_*


This was a typo?

yananlong · 2026-07-07T07:37:00Z

+    local_paths = {file_path: file_path for file_path in file_paths}
+    dedup_report = check_duplicates(file_paths, local_paths, {'files': {}})
+    duplicate_results = [
+        result for result in dedup_report.results if result.duplicate_of
    ]


This looks like it only does local same-batch duplicate detection now. Since the manifest is always {'files': {}}, this path can’t catch “this candidate already exists in the accepted datastore,” even though the docs say the PR bot compares candidates against accepted records. Could we either load/pass the real manifest here, or make this command/docs explicit that this is local-only? Also, since file_paths are local CLI paths, we probably want to normalize them through repo_path_from_path; otherwise ./data/... or absolute paths won’t scope correctly by data/.

yananlong · 2026-07-07T08:05:42Z

+def _source_identity(source_data: Any) -> dict[str, Any]:
+    if not isinstance(source_data, dict):
+        return {}
+    identity = {
+        'type': _norm_str(source_data.get('source_type')),
+        'name': _norm_str(source_data.get('dataset_name')),
+    }
+    if source_data.get('source_type') == 'hf_dataset':
+        repo = _norm_str(source_data.get('hf_repo'))
+        if repo:
+            identity['hf'] = repo
+    return identity


This seems too loose for non-HF sources. For url or leaderboard-style sources, two different datasets with the same dataset_name would collapse to the same source identity because the URL/source artifact/version is ignored. That could create false duplicate hits across different leaderboards or dataset releases. Could we include a normalized URL / source artifact / version field when present, not just dataset_name?

yananlong · 2026-07-07T08:24:06Z

+def check_companion_exists(
+    repo_path: str,
+    aggregate_data: dict[str, Any],
+    available_files: Container[str],
+) -> list[str]:
+    """Warn when aggregate detailed results point to a missing JSONL companion."""
+    detail = aggregate_data.get('detailed_evaluation_results')
+    if not isinstance(detail, dict) or not detail.get('file_path'):
+        return []
+
+    folder = Path(repo_path).parent
+    uuid = Path(repo_path).stem
+    expected = {
+        str(folder / f'{uuid}.jsonl'),
+        str(folder / f'{uuid}_samples.jsonl'),
+    }
+    if not any(path in available_files for path in expected):
+        return [
+            f"Companion .jsonl for '{Path(repo_path).name}' not found "
+            'in the dataset or this PR'
+        ]
+    return []


This ignores the actual detailed_evaluation_results.file_path and just infers the companion path from the aggregate UUID. That can pass when the referenced file is wrong but a same-UUID JSONL exists, and fail when the aggregate intentionally points to a different relative path. Could we validate the referenced path directly, maybe with same-folder UUID aliases as a fallback?

yananlong · 2026-07-07T08:30:01Z

+    for index, result in enumerate(results):
+        if not isinstance(result, dict):
+            continue
+        metric = result.get('metric_config')
+        if not isinstance(metric, dict):
+            continue
+        for key in ('score_type', 'min_score', 'max_score'):
+            if key not in metric:
+                warnings.append(
+                    f"evaluation_results[{index}].metric_config: missing '{key}'"
+                )
+
+        score_details = result.get('score_details')
+        if not isinstance(score_details, dict):
+            continue
+        score = score_details.get('score')
+        lo = metric.get('min_score')
+        hi = metric.get('max_score')
+        if (
+            isinstance(score, (int, float))
+            and not isinstance(score, bool)
+            and isinstance(lo, (int, float))
+            and not isinstance(lo, bool)
+            and isinstance(hi, (int, float))
+            and not isinstance(hi, bool)
+            and (score < lo or score > hi)
+        ):
+            warnings.append(
+                f'evaluation_results[{index}]: score {score} is outside '
+                f'[min_score={lo}, max_score={hi}]'
+            )


This only warns when the key is absent, but fails silently when they are present but null or NaN, and the bound checks for lo and hi are also silently skipped. If these fields are now expected, can we treat None/blank the same as missing?

update validation workflow

e44c9ce

nelaturuharsha requested review from Erotemic, akornilo, damian1996 and tommasocerruti June 29, 2026 10:18

Erotemic reviewed Jun 29, 2026

View reviewed changes

addressing comments

d27666b

yananlong reviewed Jul 6, 2026

View reviewed changes

yananlong reviewed Jul 7, 2026

View reviewed changes

Uh oh!

Conversation

nelaturuharsha commented Jun 29, 2026

Uh oh!

Erotemic left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants