fix(bench/tolerance): _walk_leaves recurses into bare '_' sentinel - #492
Conversation
) `benchmarks/run.py:241` uses `"_"` as the sub-bucket key for single-invocation adapters (locomo, longmemeval, amabench). The previous filter `k.startswith("_")` skipped that bucket entirely, so their `output.*` metrics under `results.<adapter>._.output.*` were never band-checked by the cron — a 100% latency regression on longmemeval would have silently passed. The fix narrows the filter to skip metadata keys (`_status`, `_elapsed_sec`, `_error_message`) but recurse into the bare `"_"` sentinel. Recursion re-filters at each level, so metadata nested inside `"_"` is still skipped. Multi-invocation adapters (mab, structmemeval) are unaffected — their sub_keys (e.g. `Conflict_Resolution`, `accounting`) don't start with `_`. Verification: - existing 19 tolerance tests still pass - 3 new tests cover: walker recurses into `_`, single-invocation band-check FAILs on regression, single-invocation band-check PASSes inside band - full suite 2834 passed, 41 skipped Setr flagged this in PR #489 review notes ("Side effects to flag" §1, 2026-05-08). Closes #490.
Reviewer's GuideAdjusts the benchmarks tolerance leaf-walking logic so it still skips underscore-prefixed metadata keys but recurses into the bare '_' sentinel used for single-invocation adapters, and adds tests to ensure single-invocation metrics are band-checked correctly for latency and counts. Sequence diagram for tolerance.check_report handling single-invocation '_' sentinelsequenceDiagram
actor Cron
participant BenchmarksRun as BenchmarksRun_py
participant Results as Results_dict
participant Tolerance as tolerance_check_report
participant Walker as _walk_leaves
Cron->>BenchmarksRun: run_benchmarks()
BenchmarksRun->>Results: build results with sub_key "_" for single-invocation adapters
BenchmarksRun-->>Cron: results
Cron->>Tolerance: check_report(results)
Tolerance->>Walker: _walk_leaves(results)
loop traverse_results
Walker->>Results: iterate keys
alt key starts_with_underscore and key != "_"
Walker-->>Results: skip metadata key (e.g. _status, _elapsed_sec)
else key == "_"
Walker->>Walker: recurse into "_" sub-dict
end
end
Walker-->>Tolerance: list of metric_leaves (includes results.adapter._.output.*)
Tolerance->>Tolerance: compute_bands_and_check(latency, counts, other_metrics)
Tolerance-->>Cron: pass_or_fail_based_on_bands
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 (2)
✨ 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 |
|
[claim:review:kulili:2026-05-08T16:02:58Z] |
|
[release:review:kulili:2026-05-08T16:03:45Z] |
Why
benchmarks/run.py:241uses"_"as the sub-bucket key for single-invocation adapters:Three adapters route through
_: locomo, longmemeval, amabench. Theiroutput.*metric leaves (overall_f1,avg_latency_ms,total_qa, …) live underresults.<adapter>._.output.*.benchmarks/tolerance._walk_leavespreviously skipped every key starting with_, so those metrics were never enumerated and the cron'stolerance.check_reportwould silently pass even on a 100% latency regression. Multi-invocation adapters (mab, structmemeval) were unaffected — their sub-keys are e.g.Conflict_Resolution, not_.Fix
One-line change to
_walk_leaves:Walker recurses into
"_"and the next level (output,_status,_elapsed_sec) is re-filtered by the same rule, so metadata nested under"_"still gets skipped.Verification
test_bench_tolerance.pytests still passtest_walk_leaves_recurses_into_bare_underscore_sentinel— direct_walk_leavestest,_recursed,_status/_elapsed_secinside_skippedtest_check_report_band_checks_single_invocation_adapter— the regression case: 100% latency drift onlongmemeval._.output.avg_latency_msnow FAILs (instead of silently passing)test_check_report_passes_inside_band_for_single_invocation_adapter— confirms band classification works through the sentinel, not just visibilityOut of scope
metric_overridesdefaultsv2.0.0.jsoncanonical (locomo currently has_status: errorand nooutput; longmemeval / amabenchoutputwill start being band-checked once cron rebuilds with this fix)Source: Setr's PR #489 review notes, "Side effects to flag" §1, 2026-05-08.
Closes #490.
Summary by Sourcery
Ensure benchmark tolerance checking walks metrics under the single-invocation '_' bucket while still skipping underscore-prefixed metadata keys.
Bug Fixes:
Tests: