Refactor and Merge several compute_score functions
#430
Conversation
Summary of ChangesHello @hiyuchang, 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 significantly refactors the codebase by centralizing and clarifying math-related scoring utilities. It addresses redundancy by merging duplicate functions, renames existing functions for better semantic distinction, and reorganizes files to improve modularity and maintainability. The changes aim to streamline the evaluation logic across the project, making it easier to manage and extend in the future. 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
|
There was a problem hiding this comment.
Code Review
This pull request effectively refactors and consolidates several compute_score functions, which significantly improves code clarity and reduces redundancy. The renaming of functions and files also makes their purpose clearer. My review includes one suggestion for examples/bots/workflow/bots_reward.py to refactor a complex block of code, aiming to improve its readability and efficiency.
|
I ran the BOTS example for tens of steps and the training curves are also aligned with the ones before, so the changes in this PR are alright to me. |
|
/unittest-diff |
Summary
Tests
Github Test Reporter by CTRF 💚 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request primarily refactors and consolidates math evaluation utilities across the codebase. Key changes include moving compute_score and related helper functions from trinity.utils.eval_utils to trinity.common.rewards.eval_utils and trinity.common.rewards.naive_dapo_score, with compute_score being renamed to compute_score_v0 in some contexts and compute_score_bots in others. Many internal math normalization and parsing functions were removed from examples/bots/workflow/bots_reward.py, indicating a shift towards using shared utilities like verl_math_equal and functions from trinity.common.rewards.naive_dapo_score and trinity.common.rewards.qwen25_eval. Test files were updated to reflect these new import paths and function names. Review comments highlighted an issue where verl_math_equal was called without a pi argument within a loop designed to test different pi values, and suggested improving the docstring for last_boxed_only_string in trinity/common/rewards/eval_utils.py for better clarity.
|
/unittest-module-trainer |
Summary
Skipped
Tests
Github Test Reporter by CTRF 💚 |
Description
This PR aims to reduce redundancy and improve clarity of math-related scoring utilities across the codebase.
Merge duplicate implementations of
compute_scoreinbenchmark/plugins/guru_math/naive_dapo.pyandexamples/bots/workflow/bots_reward.pyinto a single, unified implementation located attrinity/utils/math_utils.py.Rename the old
compute_scorefunction intrinity/utils/eval_utils.pytocompute_score_v0.Rename the (toolcall-related)
compute_score_v0function intrinity/common/workflows/customized_toolcall_workflows.pytocompute_score_with_toolN1.Rename
trinity/utils/math_eval_utils.pytotrinity/utils/qwen25_eval_utils.pyto clearly indicate its origin and scope.Checklist
Please check the following items before code is ready to be reviewed.