Skip to content

Add paired eval lift reporter#901

Merged
bingran-you merged 1 commit into
mainfrom
bry/benchflow-grpo-lift-report
Jul 9, 2026
Merged

Add paired eval lift reporter#901
bingran-you merged 1 commit into
mainfrom
bry/benchflow-grpo-lift-report

Conversation

@bingran-you

Copy link
Copy Markdown
Collaborator

Summary

  • add bench eval compare-lift for paired baseline-vs-trained jobs
  • compute pass-rate and mean-reward deltas with deterministic bootstrap CIs
  • report coverage, exclusions, duplicate healthy rollouts, and metadata breakdowns when present

Validation

  • uv run pytest tests/test_eval_lift.py tests/test_eval_artifact_cli.py
  • uv run ruff check src/benchflow/eval_lift.py src/benchflow/cli/eval_lift.py tests/test_eval_lift.py

Notes

  • pairs only healthy scored tasks present on both sides
  • duplicate healthy rollouts are de-duplicated deterministically and reported; canonicalize jobs first when trial selection matters.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

@bingran-you
bingran-you temporarily deployed to pypi-internal-preview July 8, 2026 22:32 — with GitHub Actions Inactive
@bingran-you
bingran-you force-pushed the bry/benchflow-grpo-lift-report branch from 3f4b854 to 130a8b7 Compare July 8, 2026 22:54
@bingran-you
bingran-you temporarily deployed to pypi-internal-preview July 8, 2026 22:54 — with GitHub Actions Inactive
@bingran-you
bingran-you force-pushed the bry/benchflow-grpo-lift-report branch from 130a8b7 to bae3f3c Compare July 8, 2026 22:55
@bingran-you
bingran-you temporarily deployed to pypi-internal-preview July 8, 2026 22:56 — with GitHub Actions Inactive
@bingran-you
bingran-you force-pushed the bry/benchflow-grpo-lift-report branch from bae3f3c to 6a885e1 Compare July 8, 2026 23:01
@bingran-you
bingran-you temporarily deployed to pypi-internal-preview July 8, 2026 23:01 — with GitHub Actions Inactive
@bingran-you

Copy link
Copy Markdown
Collaborator Author

Thermo-nuclear maintainability review for current head 6a885e14ee7a9a77d41dae51156e4a16481c9765.

Verdict: request changes before merge. The reporter is cleanly scoped and the CLI wiring is small, but the metric semantics are not yet safe enough for future eval-maintenance workflows.

Findings

  1. Duplicate healthy rollouts are selected by path order.
    src/benchflow/eval_lift.py:345-356 keeps the first healthy rollout for each task after sorting by (task_id, rollout_dir). On retry or multi-trial jobs, that can pick an arbitrary trial and flip the reported lift. The limitation text notes de-duping, but the CLI still emits authoritative metrics. Prefer reusing/accepting canonical selection from the existing eval artifact flow, or fail fast on duplicate healthy rollouts unless the caller explicitly opts into deterministic first-by-path selection.

  2. The reporter excludes scored rows with error fields before applying shared scoring semantics.
    src/benchflow/eval_lift.py:332-341 treats any error, verifier_error, or export_error as exclusion even when a numeric reward exists. The shared scorer contract in src/benchflow/_utils/scoring.py gives explicit reward precedence over error text, so this can silently drop scored failures/passes from denominators. Either reuse the shared classification semantics or make the stricter “error-free scored lift” contract explicit in naming/output and tests.

  3. Regression test docstrings do not name this PR/commit.
    tests/test_eval_lift.py:110 and tests/test_eval_lift.py:151 say “PR slice 3”; repo instructions require regression tests to name the PR/commit they guard. Please update these to reference PR Add paired eval lift reporter #901 or the final commit.

Validation/handoff

  • GitHub checks are green on the reviewed head (test, pip-audit, parity, rollout-smoke; integration matrix jobs skipped as expected).
  • Main-thread focused validation: uv run --extra dev python -m pytest tests/test_eval_lift.py, ruff check, and ty check on the new reporter passed in a disposable PR worktree.
  • Independent subagent pass also checked git diff --check and AST parse, then flagged the duplicate-rollout and scoring-semantics issues above.

Next owner steps

  • Decide the canonical input contract: canonical selection required/accepted, or duplicates fail closed.
  • Align reward/error classification with shared scoring semantics, or make this stricter denominator explicit.
  • Add tests for duplicate same-task rollouts and reward-plus-error rows.
  • Fix the regression-test docstrings and rerun the targeted checks above.

@bingran-you
bingran-you force-pushed the bry/benchflow-grpo-lift-report branch from 6a885e1 to a7edbaa Compare July 9, 2026 01:01
@bingran-you
bingran-you temporarily deployed to pypi-internal-preview July 9, 2026 01:01 — with GitHub Actions Inactive
@bingran-you

Copy link
Copy Markdown
Collaborator Author

Addressed review comments in latest push.

Changes:

  • duplicate healthy rollouts now fail fast by default
  • added explicit --allow-duplicate-first-by-path opt-in
  • numeric reward rows stay scored even when error fields are present
  • regression docstrings now reference PR Add paired eval lift reporter #901

Validation:

  • uv run pytest tests/test_eval_lift.py tests/test_eval_artifact_cli.py
  • uv run ruff check src/benchflow/eval_lift.py src/benchflow/cli/eval_lift.py tests/test_eval_lift.py
  • uv run ruff format --check src/benchflow/eval_lift.py src/benchflow/cli/eval_lift.py tests/test_eval_lift.py
  • uv run ty check src/benchflow/eval_lift.py src/benchflow/cli/eval_lift.py

GitHub checks are green on the updated head.

@bingran-you
bingran-you merged commit 23f79bb into main Jul 9, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant