Skip to content

fix(cli): reject --trials > 1 without --matrix - #1064

Merged
bingran-you merged 3 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-trials-requires-matrix
Sep 1, 2026
Merged

fix(cli): reject --trials > 1 without --matrix#1064
bingran-you merged 3 commits into
benchflow-ai:mainfrom
Benjamin-eecs:fix-trials-requires-matrix

Conversation

@Benjamin-eecs

@Benjamin-eecs Benjamin-eecs commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Description

Reject --trials > 1 without --matrix in build_eval_plan with a clean, actionable EvalPlanError that explains the supported single-entry-matrix path.

Motivation and Context

Only the matrix expansion consumes trials; a plain run silently does one trial per task while the caller believes it ran N. The guard matches the neighboring flag-dependency validations (--registry requires --dataset, --matrix currently requires --tasks-dir). Matrix-generated per-trial requests bypass this planner by construction.

Closes #1060.

Types of changes

  • Bug fix
  • New feature
  • Breaking change: an invalid command that previously exited 0 now exits 1
  • Documentation

Implemented Tasks

  • Add the --trials > 1 requires --matrix guard to build_eval_plan
  • Explain the single-entry-matrix escape hatch
  • Add a PR-named CLI regression test

Validation

  • tests/: 5932 passed, 48 skipped, 7 deselected
  • ruff format --check, ruff check, and ty check src/
  • Real gemini-3.1-pro-preview Docker matrix with one alias and two trials: both scored reward 1.0
  • Artifact validator: 2/2 healthy, both with ACP + provider LLM trajectories and training-ready result rows

Copilot AI lite review requested due to automatic review settings August 29, 2026 07:38

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Devin Review: 1 flag

Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)

Devin Review

@JeremyJC67

Copy link
Copy Markdown

Clean, minimal fix in exactly the right place. I verified the guard sits at the single validation chokepoint — build_eval_plan's only caller is cli/main.py:694 — so it covers every entry path (CLI, --config, --dataset, --source-repo, --source-env; the YAML path has no trials key and sharded workers receive an EvaluationConfig, not flags). The new test is red on main (exit 0, silently one trial) and green here; test_eval_artifact_cli and test_eval_sharding pass, and the --matrix --trials 2 path is unaffected. I also think rejecting is the right call over making plain --trials work: the trials loop is keyed by the matrix alias for the jobs-dir layout (root//trial-NN) and per-trial artifact paths, and plain runs span five source types including hosted source-env — honoring trials everywhere is a feature with layout decisions attached, not a bugfix.

Three suggestions, none blocking:

  1. Teach the escape hatch in the error message. The natural intent behind --trials 3 without --matrix is "run N trials of one model" — which is exactly what FrontierPhysics' contributor guidance asks for. The supported incantation is a single-entry matrix (models: {default: <model>}; _load_eval_matrix accepts a bare string per alias), but neither the error nor the docs say so. Suggest: --trials > 1 requires --matrix (to repeat one model, use a single-entry matrix file: "models: {default: <model>}"). It converts a dead-end error into a self-service one.

  2. Document the non-re-entry invariant at the guard. run_matrix_eval's per-trial requests (cli/eval_artifacts.py:297-312) are built with replace(req, ..., matrix=None) while keeping trials > 1 — they violate this guard's predicate and stay valid only because they never re-enter build_eval_plan. One extra sentence in your comment ("per-trial requests from run_matrix_eval bypass this by construction") protects whoever later adds re-validation of trial plans.

  3. Consider ticking "breaking change" (or noting it) in the description: a script passing --trials 3 without --matrix flips from exit 0 to exit 1. That flip is the entire point of the fix, but it should reach release notes so a red CI is explained by the changelog rather than a debugging session.

Ordering vs #1046 (feat/ablate-cli): I trial-merged the two branches — clean auto-merge, no textual conflict (your guard is at eval_plan.py:267; our reasoning-effort preflight lands ~170 lines below in the same function), and the merged tree passes all 48 arg-validation tests. Semantically independent too: our ablation paths construct EvalCreateRequest with the default trials=1, so the guard never fires on them. Whichever lands second rebases mechanically — happy to rebase whichever lands second.

@Benjamin-eecs

Copy link
Copy Markdown
Contributor Author

Thanks for checking the chokepoint coverage.

@bingran-you bingran-you left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Validated at exact head b186cfb. The guard remains at the canonical planner boundary, now documents the matrix re-entry invariant and gives users the supported single-entry-matrix escape hatch. Focused tests, the complete 5,932-test suite, Ruff format/lint, and ty all pass. Real-key Docker E2E with gemini-3.1-pro-preview completed both matrix trials at reward 1.0; the artifact validator reports 2/2 healthy rollouts with complete ACP/provider trajectories and training-ready rows. Ready to squash merge.

@bingran-you
bingran-you merged commit 73c9169 into benchflow-ai:main Sep 1, 2026
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.

[BUG] --trials is silently ignored without --matrix

4 participants