refactor: drop benchmarks/-importing targets from aelf bench (closes #342) - #348
Conversation
…val_score / posterior_ranking `benchmarks/verify_clean.py` and `benchmarks/longmemeval_score.py` already had `if __name__ == '__main__':` blocks, so `python -m benchmarks.verify_clean` and `python -m benchmarks.longmemeval_score` are now invokable from a source checkout. Add `benchmarks/posterior_ranking/__main__.py` with the same argparse surface the removed `aelf bench posterior-residual` subcommand had: --fixtures, --seeds, --mrr-threshold, --ece-threshold, --json, --heat-kernel. Also fix `longmemeval_budget_sweep.py` to import via the package path (`benchmarks.longmemeval_adapter`) so deptry sees a resolvable first-party import.
…loses #342) The three subcommands (`verify-clean`, `longmemeval-score`, `posterior-residual`) imported from the dev-only `benchmarks/` tree. For wheel users they always failed with the "requires the source tree" hint; for dev users they were a redundant alias for the underlying scripts. Each one tied a runtime CLI surface to dev-script knowledge (deptry DEP001). Replace each `if target == ...` branch with a single redirect that points the user at the equivalent `python -m benchmarks.<name>` entry point. The unknown-target error message lists the moved targets too. Synthetic (default) and `_BENCH_INERT_TARGETS` paths are unchanged. Drop the `DEP001 = ["benchmarks"]` ignore in pyproject.toml: `src/` no longer imports the dev-only package, so the layering allowlist (the real fix #325 had silenced) is no longer needed.
…ry points `tests/test_benchmarks_dir.py`: replace the dispatch-to-module assertions for verify-clean / longmemeval-score with assertions that the CLI exits 2 and points at the new `python -m benchmarks.<name>` entry. Added cases for posterior-residual redirect and unknown-target listing all three moved targets. `tests/test_posterior_ranking_eval.py`: rewrite the three CLI integration tests to call `benchmarks.posterior_ranking.__main__.main` directly with the same argparse flags. Drop the now-unused `_run_cli` helper plus its `io` and `cli_main` imports.
- benchmarks/README.md: `aelf bench verify-clean` / `aelf bench longmemeval-score` examples → `python -m benchmarks.<name>`. - docs/BENCHMARKS.md: same for `verify-clean` examples. - docs/bayesian_ranking.md: `aelf bench posterior-residual --heat-kernel` → `python -m benchmarks.posterior_ranking --heat-kernel`. - CHANGELOG.md: Unreleased / Changed entry covering the move and the deptry DEP001-ignore drop.
Reviewer's GuideRefactors the Sequence diagram for python -m benchmarks.posterior_ranking CLI entrysequenceDiagram
actor User
participant PythonInterpreter
participant BenchPosteriorMain as benchmarks_posterior_ranking___main__
participant PosteriorRun as benchmarks_posterior_ranking_run
User->>PythonInterpreter: run python -m benchmarks.posterior_ranking [flags]
PythonInterpreter->>BenchPosteriorMain: import benchmarks.posterior_ranking.__main__
PythonInterpreter->>BenchPosteriorMain: call main(argv)
BenchPosteriorMain->>BenchPosteriorMain: parser = ArgumentParser(...)
BenchPosteriorMain->>BenchPosteriorMain: parser.add_argument(... for fixtures, seeds, thresholds, json_out, heat_kernel)
BenchPosteriorMain->>BenchPosteriorMain: args = parser.parse_args(argv)
alt fixtures not provided
BenchPosteriorMain->>BenchPosteriorMain: fixtures_path = _default_fixtures()
else fixtures provided
BenchPosteriorMain->>BenchPosteriorMain: fixtures_path = Path(args.fixtures)
end
BenchPosteriorMain->>PosteriorRun: run(fixtures_path, n_seeds=args.seeds, mrr_threshold=args.mrr_threshold, ece_threshold=args.ece_threshold, heat_kernel=args.heat_kernel)
PosteriorRun-->>BenchPosteriorMain: result dict with mrr, ece, overall_pass
alt args.json_out is True
BenchPosteriorMain->>BenchPosteriorMain: payload = json.dumps({mrr, ece, overall_pass})
BenchPosteriorMain->>User: print payload (JSON)
else args.json_out is False
BenchPosteriorMain->>BenchPosteriorMain: format human-readable summary from result
BenchPosteriorMain->>User: print summary text
end
BenchPosteriorMain-->>PythonInterpreter: return exit_code (0 if overall_pass else 1)
PythonInterpreter-->>User: process exit with exit_code
Sequence diagram for aelf bench handling moved dev-only targetssequenceDiagram
actor User
participant AelfCli as aelf_cli_entry
participant CmdBench as _cmd_bench
User->>AelfCli: run aelf bench target [rest]
AelfCli->>CmdBench: call _cmd_bench(args, out)
CmdBench->>CmdBench: parse args.target into target
alt target in _DEV_TARGETS_MOVED
CmdBench->>CmdBench: lookup redirect = _DEV_TARGETS_MOVED[target]
CmdBench->>User: print "aelf bench target has moved. Run `redirect ...` from a source checkout."
CmdBench-->>AelfCli: return 2
else target in _BENCH_INERT_TARGETS
CmdBench->>CmdBench: map target to phase via _BENCH_INERT_TARGETS
CmdBench->>User: run synthetic benchmark for phase
CmdBench-->>AelfCli: return synthetic exit code
else unknown target
CmdBench->>CmdBench: known_targets = sorted(_BENCH_INERT_TARGETS)
CmdBench->>CmdBench: moved_targets = sorted(_DEV_TARGETS_MOVED)
CmdBench->>User: print unknown-target error with known_targets and moved_targets hint
CmdBench-->>AelfCli: return 2
end
AelfCli-->>User: process exit with exit code
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
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ 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 ignored due to path filters (1)
📒 Files selected for processing (10)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 44 minutes and 2 seconds.Comment |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="CHANGELOG.md" line_range="15" />
<code_context>
+- **Dev-only benchmark targets moved out of the `aelf` CLI** ([#342](https://github.com/robotrocketscience/aelfrice/issues/342)). `aelf bench verify-clean | longmemeval-score | posterior-residual` were always-failing in shipped wheels (the `benchmarks/` tree is dev-only and not packaged) and put dev-script knowledge in the runtime CLI surface. Replaced with module-level entry points runnable from a source checkout: `python -m benchmarks.verify_clean PATH ...`, `python -m benchmarks.longmemeval_score PREDS GT JUDGE`, `python -m benchmarks.posterior_ranking [--fixtures ... --seeds N --mrr-threshold X --ece-threshold Y --json --heat-kernel]`. The unknown-target error in `aelf bench` now points at the new entry points. The default `aelf bench` (synthetic harness in `src/aelfrice/benchmark.py`) and the `_BENCH_INERT_TARGETS` placeholders are unchanged. The deptry `DEP001 = ["benchmarks"]` ignore is removed — `src/aelfrice` no longer imports the dev-only package.
</code_context>
<issue_to_address>
**issue:** The documented arguments for `benchmarks.longmemeval_score` here differ from the other docs in this repo.
Here you show `benchmarks.longmemeval_score` taking three arguments (`PREDS GT JUDGE`), while other docs (e.g., benchmark README and BENCHMARKS) show only two. Please update this entry or the other docs so they all describe the same invocation signature for `longmemeval_score`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| ### Changed | ||
|
|
||
| - **Dev-only benchmark targets moved out of the `aelf` CLI** ([#342](https://github.com/robotrocketscience/aelfrice/issues/342)). `aelf bench verify-clean | longmemeval-score | posterior-residual` were always-failing in shipped wheels (the `benchmarks/` tree is dev-only and not packaged) and put dev-script knowledge in the runtime CLI surface. Replaced with module-level entry points runnable from a source checkout: `python -m benchmarks.verify_clean PATH ...`, `python -m benchmarks.longmemeval_score PREDS GT JUDGE`, `python -m benchmarks.posterior_ranking [--fixtures ... --seeds N --mrr-threshold X --ece-threshold Y --json --heat-kernel]`. The unknown-target error in `aelf bench` now points at the new entry points. The default `aelf bench` (synthetic harness in `src/aelfrice/benchmark.py`) and the `_BENCH_INERT_TARGETS` placeholders are unchanged. The deptry `DEP001 = ["benchmarks"]` ignore is removed — `src/aelfrice` no longer imports the dev-only package. |
There was a problem hiding this comment.
issue: The documented arguments for benchmarks.longmemeval_score here differ from the other docs in this repo.
Here you show benchmarks.longmemeval_score taking three arguments (PREDS GT JUDGE), while other docs (e.g., benchmark README and BENCHMARKS) show only two. Please update this entry or the other docs so they all describe the same invocation signature for longmemeval_score.
Summary
Closes #342, supersedes #329.
src/aelfrice/cli.pyreached into the dev-onlybenchmarks/tree at three points (verify-clean,longmemeval-score,posterior-residual). For wheel users those subcommands always failed with the "requires the source tree" hint; for dev users they were a redundant alias for the underlying scripts. Each one tied a runtime CLI surface to dev-script knowledge — deptry flagged this as DEP001 and PR #325 silenced it via an allowlist.This PR removes the layering violation outright. The three subcommands are replaced with module-level entry points runnable from a source checkout, and the deptry allowlist for
benchmarksis dropped.Behavior
aelf bench verify-clean PATH ...python -m benchmarks.verify_clean PATH ...aelf bench longmemeval-score PREDS GT JUDGEpython -m benchmarks.longmemeval_score PREDS GT JUDGEaelf bench posterior-residual [flags]python -m benchmarks.posterior_ranking [flags]The unknown-target error in
aelf benchlists the three moved entry points so a user typing the old subcommand gets a clean redirect:aelf bench(default / synthetic) and the_BENCH_INERT_TARGETSplaceholders are unchanged.Implementation notes
benchmarks/verify_clean.pyandbenchmarks/longmemeval_score.pyalready hadif __name__ == "__main__":blocks, sopython -mworks against them as-is.benchmarks/posterior_ranking/is a package — added__main__.pythat mirrors the argparse surface previously living insidecli.py(--fixtures,--seeds,--mrr-threshold,--ece-threshold,--json,--heat-kernel).benchmarks/longmemeval_budget_sweep.pyhad a barefrom longmemeval_adapter import ...that worked only because of theDEP001ignore. Switched tofrom benchmarks.longmemeval_adapter import ...so deptry resolves it as first-party.pyproject.toml: removedDEP001 = ["benchmarks"]from[tool.deptry.per_rule_ignores].deptry .is clean.tests/test_benchmarks_dir.py: replaced the dispatch-to-module assertions with redirect-message assertions; added an unknown-target case.tests/test_posterior_ranking_eval.py: rewrote the three CLI integration tests to invokebenchmarks.posterior_ranking.__main__.maindirectly with the same argparse flags. Dropped the unused_run_clihelper + its imports.benchmarks/README.md,docs/BENCHMARKS.md,docs/bayesian_ranking.md, plus CHANGELOGUnreleasedentry.Tradeoffs
Wheel users who somehow had the source tree alongside an installed wheel and were running these three subcommands lose them. Mitigation: the redirect message points them at the new entry points. Acceptable — these subcommands were guarded against the wheel-only case anyway.
Test plan
uv run pytest(1981 passed, 20 skipped)uvx deptry .cleanSummary by Sourcery
Move dev-only benchmark subcommands out of the
aelf benchCLI into module-level entry points and update messaging, tests, and docs accordingly.New Features:
python -m benchmarks.posterior_rankingas the command-line entry point for the posterior ranking benchmark.Enhancements:
aelf benchdev-only targets (verify-clean,longmemeval-score,posterior-residual) with redirect messages pointing to the correspondingpython -m benchmarks.<module>invocations.aelf benchunknown-target error to list only supported targets and separately mention the moved dev-only benchmarks.benchmarkspackage and fixing an import inlongmemeval_budget_sweepso it resolves as first-party.Documentation:
python -m benchmarks.*entry points and describe the new behavior in the changelog.Tests: