-
Notifications
You must be signed in to change notification settings - Fork 3k
[misc] feat: add importlib option to import external reward loop module #4635
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a valuable feature by allowing external reward loop modules to be imported via importlib, increasing the framework's flexibility. The implementation in reward_loop.py is straightforward, and the configuration changes in the YAML files are clear.
My review includes two main points of feedback with high severity:
- The use of
assertfor configuration validation should be replaced with explicitifchecks andValueErrorexceptions for better robustness, especially in production environments. - The instantiation of the custom reward manager class is very rigid due to the use of hardcoded positional arguments. This limits the utility of the new feature and can be confusing for developers. I've suggested a more flexible design using keyword arguments.
Addressing these points will improve the robustness and usability of this new feature.
| assert reward_loop_module_path is not None, ( | ||
| "reward_loop_module_path must be set when reward_loop_source='importlib'" | ||
| ) | ||
| assert reward_loop_class_name is not None, ( | ||
| "reward_loop_class_name must be set when reward_loop_source='importlib'" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using assert for validating configuration parameters is not robust. Assertions can be disabled with the -O Python flag, which would cause these checks to be skipped in production environments. This could lead to cryptic errors later in the code if the configuration is missing required fields. It's better to use explicit if checks and raise a ValueError for invalid configuration.
if reward_loop_module_path is None:
raise ValueError("reward_loop_module_path must be set when reward_loop_source='importlib'")
if reward_loop_class_name is None:
raise ValueError("reward_loop_class_name must be set when reward_loop_source='importlib'")| self.reward_loop = reward_manager_cls( | ||
| self.config, self.input_tokenizer, self.reward_fn, self.reward_router_address, self.reward_model_tokenizer | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The instantiation of the reward_manager_cls uses a fixed set of positional arguments. This creates a rigid contract for any custom reward manager, which must accept these exact arguments in this specific order. This reduces the flexibility that this PR aims to provide and can be error-prone for developers implementing their own reward managers, especially since the base class RewardManagerBase has a different __init__ signature.
A more flexible and robust approach would be to use keyword arguments. This would make the interface for custom reward managers clearer and less brittle. For example:
self.reward_loop = reward_manager_cls(
config=self.config,
tokenizer=self.input_tokenizer,
reward_fn=self.reward_fn,
reward_router_address=self.reward_router_address,
reward_model_tokenizer=self.reward_model_tokenizer,
)This would likely require changes to RewardManagerBase to accept **kwargs, but it would significantly improve the design and usability of this feature. Please consider refactoring the instantiation to be more flexible.
…le (volcengine#4635) ### What does this PR do? Currently, we can only implement the reward manager inside verl. This PR supports importing the new reward module from outside, similar to our reward function solution. ### Checklist Before Starting - [ ] Search for similar PRs. Paste at least one query link here: ... - [ ] Format the PR title as `[{modules}] {type}: {description}` (This will be checked by the CI) - `{modules}` include `fsdp`, `megatron`, `sglang`, `vllm`, `rollout`, `trainer`, `ci`, `training_utils`, `recipe`, `hardware`, `deployment`, `ray`, `worker`, `single_controller`, `misc`, `perf`, `model`, `algo`, `env`, `tool`, `ckpt`, `doc`, `data`, `cfg`, `reward` - If this PR involves multiple modules, separate them with `,` like `[megatron, fsdp, doc]` - `{type}` is in `feat`, `fix`, `refactor`, `chore`, `test` - If this PR breaks any API (CLI arguments, config, function signature, etc.), add `[BREAKING]` to the beginning of the title. - Example: `[BREAKING][fsdp, megatron] feat: dynamic batching` ### Test > For changes that can not be tested by CI (e.g., algorithm implementation, new model support), validate by experiment(s) and show results like training curve plots, evaluation results, etc. ### API and Usage Example > Demonstrate how the API changes if any, and provide usage example(s) if possible. ```python # Add code snippet or script demonstrating how to use this ``` ### Design & Code Changes > Demonstrate the high-level design if this PR is complex, and list the specific changes. ### Checklist Before Submitting > [!IMPORTANT] > Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review. - [ ] Read the [Contribute Guide](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md). - [ ] Apply [pre-commit checks](https://github.com/volcengine/verl/blob/main/CONTRIBUTING.md#code-linting-and-formatting): `pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=always` - [ ] Add / Update [the documentation](https://github.com/volcengine/verl/tree/main/docs). - [ ] Add unit or end-to-end test(s) to [the CI workflow](https://github.com/volcengine/verl/tree/main/.github/workflows) to cover all the code. If not feasible, explain why: ... - [ ] Once your PR is ready for CI, send a message in [the `ci-request` channel](https://verl-project.slack.com/archives/C091TCESWB1) in [the `verl` Slack workspace](https://join.slack.com/t/verl-project/shared_invite/zt-3855yhg8g-CTkqXu~hKojPCmo7k_yXTQ). (If not accessible, please try [the Feishu group (飞书群)](https://applink.larkoffice.com/client/chat/chatter/add_by_link?link_token=772jd4f1-cd91-441e-a820-498c6614126a).)
What does this PR do?
Currently, we can only implement the reward manager inside verl.
This PR supports importing the new reward module from outside, similar to our reward function solution.
Checklist Before Starting
[{modules}] {type}: {description}(This will be checked by the CI){modules}includefsdp,megatron,sglang,vllm,rollout,trainer,ci,training_utils,recipe,hardware,deployment,ray,worker,single_controller,misc,perf,model,algo,env,tool,ckpt,doc,data,cfg,reward,like[megatron, fsdp, doc]{type}is infeat,fix,refactor,chore,test[BREAKING]to the beginning of the title.[BREAKING][fsdp, megatron] feat: dynamic batchingTest
API and Usage Example
# Add code snippet or script demonstrating how to use thisDesign & Code Changes
Checklist Before Submitting
Important
Please check all the following items before requesting a review, otherwise the reviewer might deprioritize this PR for review.
pre-commit install && pre-commit run --all-files --show-diff-on-failure --color=alwaysci-requestchannel in theverlSlack workspace. (If not accessible, please try the Feishu group (飞书群).)