fix(bench): exit-2 on no-data + SKIP verdict in band-check (#479) - #485
Merged
Conversation
Per the 3-state exit-code contract ratified at PR #465 gate commit 60e1b68 and resurfaced by the StructMemEval calibration session in issue #473: adapters that find no data (data dir missing, dataset file absent, filters matching zero rows) now sys.exit(2) instead of print + plain `return`. The dispatcher (`benchmarks/run.py::run_invocation`) already classifies exit-code 2 as `_status: skipped_data_missing` per the 2026-05-06 contract; this commit closes the producer half so the canonical JSON distinguishes "data not installed" from "adapter crashed." Adapters touched (uniform pattern: try/except FileNotFoundError around the load, plus the existing post-load empty-list guard, both routing to sys.stderr + sys.exit(2)): - structmemeval_adapter.py — surfaced the bug via issue #473's /tmp/StructMemEval-empty trace. - locomo_adapter.py — load_locomo() reads a JSON file directly. - longmemeval_adapter.py — load_from_file() OR HuggingFace fallback. - mab_adapter.py — load_mab_split() can FileNotFoundError on absent HuggingFace cache. - mab_entity_index_adapter.py — same load_mab_split() path. - amabench_adapter.py — HuggingFace load_dataset() typically raises on absent data; if it returns zero rows after filtering, also exit 2. The dispatcher's existing fallback at run.py:167-172 (`adapter exited 0 but did not write $TMPDIR/T/<adapter>.json`) becomes a strict "adapter bug" signal rather than masking missing-data situations.
Pairs with the adapter-side change in this branch: when an observed sub-result has `_status: skipped_data_missing`, every canonical leaf under that sub-result is uncomputable, not a regression. Treating them as FAIL (today's behaviour) over-reports regressions whenever a fresh runner doesn't have /tmp/<dataset> populated. - New `Verdict.SKIP` enum member (`band_kind="skipped"` for matching BandCheck rows). - `_ancestor_skipped(obs, path)` walks the observed sub-tree and returns True at the first ancestor carrying `_status == "skipped_data_missing"`. `check_report()` consults this before its existing missing-leaf FAIL path. - `summarize()` tallies SKIP separately from PASS/WARN/FAIL and never promotes the rollup above PASS based on SKIP count alone. The CI band-check workflow consumes `summarize()`'s rollup verdict, so no workflow YAML edit is needed: skipped sub-results stop blocking green runs, and the per-leaf SKIP rows still surface in the BandCheck list for human review.
Two new test files: - `tests/test_bench_adapters_exit_code.py` — exercises each of the 6 adapters via main() with monkey-patched loaders. Both shapes from the issue covered: FileNotFoundError → exit 2, empty list → exit 2. Optional benchmark deps (nltk / datasets / tiktoken) are stubbed at sys.modules so the test runs in the CI's `--group dev --extra archive` environment without `--extra benchmarks`. - `tests/test_bench_tolerance_skip.py` — verifies the new SKIP verdict on the tolerance band-check: skip propagation to all canonical descendants, summarize() does not promote SKIP to FAIL/WARN, FAIL still dominates SKIP, sibling adapters still get checked normally, and the SKIP path triggers only on `_status: skipped_data_missing` (other statuses fall through to the existing FAIL branch). Together with the existing `test_skipped_data_missing_propagates` in test_bench_dispatcher.py, the producer + transport + consumer halves of the 3-state contract are all under test.
Reviewer's GuideImplements the producer-side 3-state exit code contract for six benchmark adapters (exit 2 on no-data) and adds a consumer-side SKIP verdict in the band-check tolerance logic so missing data is treated as skipped rather than failed, with tests covering both behaviors. Sequence diagram for adapter 3-state exit and tolerance SKIP handlingsequenceDiagram
actor CI
participant Adapter as BenchmarkAdapter
participant Dispatcher as RunInvocation
participant Tolerance as ToleranceCheckReport
CI->>Adapter: Run adapter
alt Data directory missing
Adapter->>Adapter: Load data
Adapter-->>Adapter: FileNotFoundError
Adapter->>CI: exit 2 (no data)
CI->>Dispatcher: Record exit code 2
Dispatcher->>Dispatcher: Set _status skipped_data_missing
Dispatcher->>Tolerance: check_report(canonical, observed)
loop For each canonical leaf
Tolerance->>Tolerance: _ancestor_skipped(obs_results, path)
Tolerance-->>Tolerance: True
Tolerance->>Tolerance: Create BandCheck with verdict SKIP
end
Tolerance->>Tolerance: summarize(checks)
Tolerance-->>CI: Overall PASS (with SKIP counts)
else Adapter crash or real regression
Adapter->>CI: exit 1 (error)
CI->>Dispatcher: Record exit code 1
Dispatcher->>Dispatcher: Set failure status
Dispatcher->>Tolerance: check_report(...)
Tolerance-->>CI: Overall FAIL
end
Class diagram for updated Verdict enum and tolerance functionsclassDiagram
class Verdict {
<<enumeration>>
PASS
WARN
FAIL
SKIP
}
class ToleranceModule {
+classify(canonical_results, observed_results, lower, upper) Verdict
+_ancestor_skipped(obs_results, path) bool
+_walk_leaves(obj, path) list~tuple~
+check_report(canonical_report, observed_report, bands) list~BandCheck~
+summarize(checks) tuple~Verdict, dict~
}
Verdict <.. ToleranceModule : uses
ToleranceModule ..> BandCheck : creates
Flow diagram for adapter main exit codes on no dataflowchart TD
Start([Adapter main]) --> ParseArgs[Parse CLI args]
ParseArgs --> LoadData[Load dataset]
LoadData --> |Success| DataLoaded
LoadData --> |FileNotFoundError| FileNotFound
FileNotFound --> LogMissing[Print data not found to stderr]
LogMissing --> Exit2A[exit 2]
DataLoaded --> CheckEmpty{Loaded collection empty?}
CheckEmpty --> |Yes| LogEmpty[Print no data message to stderr]
LogEmpty --> Exit2B[exit 2]
CheckEmpty --> |No| Continue[Proceed with benchmark logic]
Continue --> End([Generate metrics JSON and exit 0])
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
robotrocketscience
added
author-Kulili
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new
_ancestor_skippedhelper currently treats the leaf itself having_status: skipped_data_missingas a match, which is useful behavior but slightly at odds with the name/docstring (“ancestor sub-result”); consider either renaming or tightening the docstring to explicitly include the leaf node as a valid skip carrier. - The try/except + empty-list exit(2) pattern is duplicated across multiple adapters; consider extracting a small shared helper (e.g., a
load_or_exit_2(...)wrapper) to centralize the behavior and reduce future drift between adapters. - In the new FileNotFoundError branches (e.g.,
longmemeval_adapter), the error message printsargs.dataeven when loading from HuggingFace with no explicit path, which can showNoneor a misleading location; it may be clearer to special-case the remote-load path and log a message that reflects the actual failing source.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `_ancestor_skipped` helper currently treats the leaf itself having `_status: skipped_data_missing` as a match, which is useful behavior but slightly at odds with the name/docstring (“ancestor sub-result”); consider either renaming or tightening the docstring to explicitly include the leaf node as a valid skip carrier.
- The try/except + empty-list exit(2) pattern is duplicated across multiple adapters; consider extracting a small shared helper (e.g., a `load_or_exit_2(...)` wrapper) to centralize the behavior and reduce future drift between adapters.
- In the new FileNotFoundError branches (e.g., `longmemeval_adapter`), the error message prints `args.data` even when loading from HuggingFace with no explicit path, which can show `None` or a misleading location; it may be clearer to special-case the remote-load path and log a message that reflects the actual failing source.
## Individual Comments
### Comment 1
<location path="benchmarks/tolerance.py" line_range="191-197" />
<code_context>
obs_results = observed.get("results", {})
checks: list[BandCheck] = []
for path, cano_val in _walk_leaves(cano_results):
+ if _ancestor_skipped(obs_results, path):
+ checks.append(BandCheck(
+ path=path, canonical=cano_val, observed=float("nan"),
+ lower=cano_val, upper=cano_val, band_kind="skipped",
+ verdict=Verdict.SKIP,
+ note=(
+ f"observed sub-result skipped (data missing) at "
+ f"{'/'.join(path)}"
+ ),
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The SKIP note reports the leaf path, which may not be where the `_status` actually lives.
This always uses `'/'.join(path)`, even when `_status: skipped_data_missing` is set on an ancestor (e.g., adapter-level), so the note points to the metric leaf instead of the skipped node. To make the diagnostic accurate, consider having `_ancestor_skipped` return the path of the skipped node (or `None`) and using that in the note instead of the leaf path.
Suggested implementation:
```python
def _ancestor_skipped(
results: dict[str, Any], path: tuple[str, ...],
) -> tuple[str, ...] | None:
"""
Return the path of the nearest ancestor (including itself) that has
`_status: skipped_data_missing`, or None if no such ancestor exists.
"""
node: dict[str, Any] | None = results
# Walk down the path, checking each prefix for skipped status
# Prefixes are: (), (path[0],), (path[0], path[1]), ..., full path.
for i in range(len(path) + 1):
if node is None:
break
status = node.get("_status")
if status == "skipped_data_missing":
# `path[:i]` is the prefix corresponding to this node.
return path[:i]
if i == len(path):
break
key = path[i]
child = node.get(key)
if not isinstance(child, dict):
break
node = child
return None
def _walk_leaves(
```
```python
obs_results = observed.get("results", {})
checks: list[BandCheck] = []
for path, cano_val in _walk_leaves(cano_results):
skipped_path = _ancestor_skipped(obs_results, path)
if skipped_path is not None:
checks.append(BandCheck(
path=path, canonical=cano_val, observed=float("nan"),
lower=cano_val, upper=cano_val, band_kind="skipped",
verdict=Verdict.SKIP,
note=(
"observed sub-result skipped (data missing) at "
f"{'/'.join(skipped_path) or '<root>'}"
),
))
continue
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+191
to
+197
| if _ancestor_skipped(obs_results, path): | ||
| checks.append(BandCheck( | ||
| path=path, canonical=cano_val, observed=float("nan"), | ||
| lower=cano_val, upper=cano_val, band_kind="skipped", | ||
| verdict=Verdict.SKIP, | ||
| note=( | ||
| f"observed sub-result skipped (data missing) at " |
There was a problem hiding this comment.
suggestion (bug_risk): The SKIP note reports the leaf path, which may not be where the _status actually lives.
This always uses '/'.join(path), even when _status: skipped_data_missing is set on an ancestor (e.g., adapter-level), so the note points to the metric leaf instead of the skipped node. To make the diagnostic accurate, consider having _ancestor_skipped return the path of the skipped node (or None) and using that in the note instead of the leaf path.
Suggested implementation:
def _ancestor_skipped(
results: dict[str, Any], path: tuple[str, ...],
) -> tuple[str, ...] | None:
"""
Return the path of the nearest ancestor (including itself) that has
`_status: skipped_data_missing`, or None if no such ancestor exists.
"""
node: dict[str, Any] | None = results
# Walk down the path, checking each prefix for skipped status
# Prefixes are: (), (path[0],), (path[0], path[1]), ..., full path.
for i in range(len(path) + 1):
if node is None:
break
status = node.get("_status")
if status == "skipped_data_missing":
# `path[:i]` is the prefix corresponding to this node.
return path[:i]
if i == len(path):
break
key = path[i]
child = node.get(key)
if not isinstance(child, dict):
break
node = child
return None
def _walk_leaves( obs_results = observed.get("results", {})
checks: list[BandCheck] = []
for path, cano_val in _walk_leaves(cano_results):
skipped_path = _ancestor_skipped(obs_results, path)
if skipped_path is not None:
checks.append(BandCheck(
path=path, canonical=cano_val, observed=float("nan"),
lower=cano_val, upper=cano_val, band_kind="skipped",
verdict=Verdict.SKIP,
note=(
"observed sub-result skipped (data missing) at "
f"{'/'.join(skipped_path) or '<root>'}"
),
))
continue
Owner
Author
|
[claim:review:setr:2026-05-08T08:11:58Z] |
Owner
Author
|
[release:review:setr:2026-05-08T08:13:13Z] |
This was referenced May 8, 2026
Closed
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Closes #479.
Implements the 3-state exit-code contract producer side, plus the band-check consumer side. The transport layer (
benchmarks/run.py::run_invocation) was already 3-state aware from PR #465's gate commit; this PR closes the loop.What lands (3 atomic commits)
fix(bench/adapters)— uniform pattern across 6 adapters: try/except FileNotFoundError around the loader + post-load empty-list guard, both routing tosys.stderr+sys.exit(2).fix(bench/tolerance)— newVerdict.SKIPenum member._ancestor_skipped()walks the observed sub-tree;check_report()consults it before the existing missing-leaf FAIL branch.summarize()tallies SKIP separately and never promotes to FAIL/WARN.test(bench)— adapter exit-code coverage (10 cases, both FileNotFoundError and empty-list shapes for each of the 6 adapters; optional deps stubbed at sys.modules) + tolerance SKIP coverage (6 cases including FAIL-still-dominates and sibling-adapter isolation).Why this resolves the surfaced incident
Before this PR:
/tmp/StructMemEvalempty → adapter exits 0 with no output file → dispatcher logs"adapter exited 0 but did not write $TMPDIR/T/structmemeval_<task>.json". Same surface as a real adapter bug.After this PR: empty data → adapter exits 2 → dispatcher classifies as
_status: skipped_data_missing→ tolerance treats nested canonical leaves asVerdict.SKIP→ CI rollup stays PASS. Real adapter crashes (exit 1) still surface as FAIL.Acceptance crosscheck (vs issue body)
mab_entity_indexwhich shares the load path) —sys.exit(2)on no-data.run.pybecause it was already 3-state aware (PR feat(bench): aelf bench all reproducibility harness (#437) #465 /run.py:154-160)._statusalready getsskipped_data_missingat dispatcher level — verified by existingtest_skipped_data_missing_propagates.skipped_data_missingdoes NOT count as a regression — implemented asVerdict.SKIP+summarize()rollup behaviour.test_bench_dispatcher.pyalready cover the dispatcher path.test_bench_adapters_exit_code.pycover the producer half (the new addition in this PR).Out of scope
Test plan
uv run pytest tests/ --ignore=tests/e2e -q→ 2737 passed, 41 skipped (no regressions on existing 2734 baseline).uv run pytest tests/test_bench_adapters_exit_code.py tests/test_bench_tolerance_skip.py tests/test_bench_tolerance.py tests/test_bench_dispatcher.py tests/test_benchmarks_badge.py -q→ 70 passed (focused).Refs
benchmarks/run.py::run_invocation(already 3-state, PR feat(bench): aelf bench all reproducibility harness (#437) #465)benchmarks/tolerance.py::Verdict.SKIP+_ancestor_skippedSummary by Sourcery
Implement three-state handling for benchmark adapters and band-check tolerance when benchmark data is missing.
Bug Fixes:
Enhancements:
Tests: