fix metrics in grpo_fast.py #1223
Conversation
move stop_rate and other metrics before mask_truncated_responses filtering that skews results add masked_truncated_responses as a metric to log
Summary of ChangesHello @mnoukhov, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the metric calculation logic within Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors metric calculations in grpo_fast.py to ensure they are computed before response filtering, which prevents skewed results. It also introduces a new metric to log the number of masked truncated responses. The changes are logical and align with the goal of improving metric accuracy. I have a few suggestions to improve the code further by simplifying some calculations, fixing a potential inconsistency in how 'unsolved' responses are defined, and using more idiomatic NumPy operations.
open_instruct/grpo_fast.py
Outdated
| sequence_length_solved = ( | ||
| np.array([]) if np.all(scores == 0) else np.array(sequence_lengths[scores == args.max_possible_score]) | ||
| ) | ||
| sequence_length_unsolved = ( | ||
| np.array([]) if np.all(scores == args.max_possible_score) else np.array(sequence_lengths[scores == 0]) | ||
| ) |
There was a problem hiding this comment.
The calculation for sequence_length_unsolved appears to be inconsistent with how unsolved_num_responses is calculated later (line 2124). unsolved_num_responses considers any score less than args.max_possible_score as unsolved, while sequence_length_unsolved only considers scores equal to 0. This can lead to confusing metrics if there are non-zero, non-max scores.
Additionally, the conditional logic for both sequence_length_solved and sequence_length_unsolved is redundant. NumPy's boolean array indexing will correctly return an empty array if no elements match the condition, making the if np.all(...) checks unnecessary.
I suggest simplifying these calculations and making sequence_length_unsolved consistent with unsolved_num_responses.
sequence_length_solved = sequence_lengths[scores == args.max_possible_score]
sequence_length_unsolved = sequence_lengths[scores < args.max_possible_score]
open_instruct/grpo_fast.py
Outdated
| stop_rate = sum(int(finish_reason == "stop") for finish_reason in result.finish_reasons) / len( | ||
| result.finish_reasons | ||
| ) |
There was a problem hiding this comment.
This calculation for stop_rate is correct, but it can be expressed more idiomatically and concisely using numpy.mean. This improves readability and leverages numpy's optimized operations for boolean arrays.
stop_rate = np.mean([fr == "stop" for fr in result.finish_reasons])
finbarrtimbers
left a comment
finbarrtimbers
left a comment
There was a problem hiding this comment.
Can we pull these out into two functions:
- calculate_sequence_lengths(result) -> dict[str, float]:
- maybe_mask_truncated_completions(result) -> tuple[GenerationResult, Batch, dict[str, float]]:
Then it becomes trivial to reorder them.
cleaner metric implementations separate out metric implementation into function separate out whether to truncate into function
|
/gemini review |
|
Hello @mnoukhov, it looks like you might have intended to use a command with |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| - mean, hist of correct lengths | ||
| - mean, hist of incorrect results | ||
| """ | ||
| sequence_lengths = np.array([len(response) for response in result.responses]) |
There was a problem hiding this comment.
Bug: Parameter name mismatch causes undefined variable error
The function calculate_sequence_length_metrics has a parameter named results (plural) but the function body on line 261 uses result.responses (singular). This variable result is undefined in the function scope, causing a NameError at runtime. The call site in grpo_fast.py passes a single GenerationResult object named result, so the parameter should be renamed from results to result and the type hint changed from list[GenerationResult] to GenerationResult.
move stop_rate and other metrics before mask_truncated_responses filtering that skews results
add masked_truncated_responses as a metric to log
Note
Compute metrics before truncation filtering, add masked_truncated_responses, refactor filtering, and centralize sequence-length metrics.
open_instruct/grpo_fast.py):val/stop_rate,good_outputs, and sequence-length metrics before applying truncation filtering.val/masked_truncated_responsesand include centralized sequence-length metrics in logged outputs.maybe_mask_truncated_completions(...)and use its outputs.stop_ratecalculation using boolean mean.open_instruct/rl_utils.py):calculate_sequence_length_metrics(...)to produce aggregate and per-class (solved/unsolved) length stats.grpo_fast.py.}Written by Cursor Bugbot for commit 735e876. This will update automatically on new commits. Configure here.