fix num_labels= 1 test fail #3493
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughA new pre-validation hook was added to automatically set default values for reward model configurations. When Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/axolotl/utils/schemas/validation.py (1)
260-270: Consider adding mutual exclusivity validation forreward_modelandprocess_reward_model.If both flags are set simultaneously,
process_reward_modelvalues will silently overridereward_modeldefaults. While this edge case is unlikely in practice, you may want to add explicit validation to catch misconfiguration early.♻️ Optional enhancement to validate mutual exclusivity
`@model_validator`(mode="before") `@classmethod` def set_reward_model_defaults(cls, data): + if data.get("reward_model") and data.get("process_reward_model"): + raise ValueError( + "reward_model and process_reward_model are mutually exclusive" + ) + if data.get("reward_model"): if data.get("num_labels") is None: data["num_labels"] = 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/axolotl/utils/schemas/validation.py` around lines 260 - 270, Add a mutual-exclusivity check before the existing defaulting logic so both "reward_model" and "process_reward_model" cannot be set simultaneously: inspect the incoming data dict at the top of the block that sets defaults for reward_model/process_reward_model and if both data.get("reward_model") and data.get("process_reward_model") are truthy raise a validation error (e.g., ValueError or the module's ValidationError) with a clear message; keep the rest of the defaulting code for "model_type" and "num_labels" unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/axolotl/utils/schemas/validation.py`:
- Around line 260-270: Add a mutual-exclusivity check before the existing
defaulting logic so both "reward_model" and "process_reward_model" cannot be set
simultaneously: inspect the incoming data dict at the top of the block that sets
defaults for reward_model/process_reward_model and if both
data.get("reward_model") and data.get("process_reward_model") are truthy raise a
validation error (e.g., ValueError or the module's ValidationError) with a clear
message; keep the rest of the defaulting code for "model_type" and "num_labels"
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aae302b6-5fcf-4ae3-97b9-144cb6dfddba
📒 Files selected for processing (3)
src/axolotl/utils/schemas/validation.pytests/core/test_builders.pytests/patched/test_validation.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Description
Add a
set_reward_model_defaultsmodel validator invalidation.pythat automatically sets num_labels=1 for reward_model: True and num_labels=2 for process_reward_model: True, and defaults the model_type to the appropriate AutoModel class.Re-enable rm_cfg in test_builder_w_rm_trainers which was previously disabled pending this fix.
How has this been tested?
test_reward_model_defaultstest_process_reward_model_defaultsSummary by CodeRabbit
Release Notes
New Features
Tests