[eval] Consolidate Eval Functions for Step-Wise and Non-Step Wise

[kyuds]

Previously in evaluate.py, we had separate functions for evaluate and evaluate_step_wise. This poses some problems:

1. Maintainability: there is a maintenance burden of two functions.
2. There are some inconsistencies in logic.

Apart from combining the logic, we make some fixes in this PR (for step-wise eval):

1. Pass in step_wise=True for concatenate_generator_outputs.
2. Pretty print examples like non-step wise counterpart.
3. Add rollout_metrics to eval_metrics.
4. Fix bug in validate_generator_output call where previous step-wise eval function took in generator_input, not len(generator_input["prompts"]), which was a typing mismatch
5. Fix incorrect counting of num_turns_list when eval_n_samples_per_prompt > 1.

---

Note

Medium Risk
Changes how eval metrics and logging are computed for step-wise trajectories; incorrect scoring would skew reported pass@n and dataset metrics, though behavior is heavily tested.

Overview
Merges evaluate and evaluate_step_wise into a single evaluate() path driven by cfg.generator.step_wise_trajectories. The trainer and eval-only entrypoint no longer branch on mode; evaluate_step_wise is removed.

The unified loop uses _EvalRows, _rows_for_output, and _scored_view so row metadata stays aligned with generator output across batches. Metrics and trajectory logging use one row per trajectory (last step only when step-wise); eval dumps still write every step. Step-wise behavior is corrected vs the old duplicate function: concatenate_generator_outputs(..., step_wise=True), validate_generator_output with prompt count + step_wise, pretty_print_example, and rollout_metrics on eval/all/*. Turn counts for logging use full TrajectoryID keys so pass@n repetitions get per-repetition step counts.

tests/train/test_eval.py adds broad coverage for both modes (last-step scoring, logger turns, multi-batch alignment, dumps, rollout metrics).

^{Reviewed by Cursor Bugbot for commit a972450. Bugbot is set up for automated code reviews on this repo. Configure here.}

[gemini-code-assist]

Code Review

This pull request consolidates plain and step-wise evaluation logic into a single, unified evaluate function, removing the redundant evaluate_step_wise implementation and introducing helper structures like _EvalRows to manage the differences. The review feedback highlights several critical issues in this consolidation: a potential AssertionError in _scored_view when handling trajectory-aligned list fields, incorrect turn counts when eval_n_samples_per_prompt > 1 due to counting by prompt IDs instead of unique trajectory IDs, and potential TypeError or AttributeError exceptions if env_extras is None or contains None elements.

[greptile-apps]

Confidence Score: 4/5

The PR should not merge until step-wise scored-view filtering handles trajectory-level timing lists without aborting multi-step evaluation.

Enabling step-wise concatenation compacts timing fields to one entry per trajectory, after which _scored_view incorrectly requires those fields to match the number of steps and raises an assertion.

Files Needing Attention: skyrl/train/evaluate.py

Important Files Changed

Filename	Overview
skyrl/train/evaluate.py	Consolidates evaluation modes and introduces row/scored views, but step-wise timing-field compaction conflicts with the new all-list alignment assertion.
skyrl/train/trainer.py	Replaces mode-specific dispatch with the consolidated evaluation call.
skyrl/train/entrypoints/main_generate.py	Routes eval-only generation through the consolidated evaluation function.
tests/train/test_eval.py	Adds useful step-wise coverage, but fixtures omit trajectory timing fields and therefore do not exercise the failing alignment path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Evaluation batches] --> B[Generate outputs]
    B --> C[Build aligned EvalRows]
    C --> D[Concatenate outputs]
    D -->|Plain| E[Score every row]
    D -->|Step-wise| F[Select last-step rows]
    F --> G[Metrics and trajectory logging]
    E --> G
    D --> H[Dump all generated rows]

Loading

_{Reviews (1): Last reviewed commit: "consolidate evals" | Re-trigger Greptile}

[greptile-apps]

Timing fields break step filtering

When step-wise evaluation includes a multi-step trajectory, concatenate_generator_outputs(..., step_wise=True) reduces the trajectory timing fields to one entry per trajectory, but _scored_view requires every list field to match the per-step is_last_step mask. The resulting length assertion aborts evaluation before metrics or dumps are produced.

[kyuds]

this is not true. in skyrl.train.generators.utils.concatenate_generator_outputs, timing fields are still concatenated as-is (look at line 294 where result is first instantiated). The reduction that greptile points out happens in rollout_metrics, but this is a non-list item anyways so it doesn't matter.

[kyuds]

cc @CharlieFRuan for one more verification

[greptile-apps]

You're right — my comment was incorrect. concatenate_generator_outputs leaves result["trajectory_generation_times"] and result["trajectory_time_splits"] as flattened per-row fields. _last_step_only only creates local values passed to get_rollout_metrics; it does not overwrite those fields in result. Therefore _scored_view still receives list fields aligned with is_last_step, and the timing fields do not cause a length-mismatch assertion. This comment should be withdrawn. Thanks for catching it.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.

[CharlieFRuan]

Mostly looks good, thank you! Let's fix the eval_n_samples_per_prompt > 1 case and slightly more tests; then we can merge!