fix: reject scheduler min_lr greater than optimizer lr#3091
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea9b161e37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| raise ValueError(f"scheduler.min_lr ({scheduler_config.min_lr}) must be <= optim.lr ({lr})") | ||
|
|
There was a problem hiding this comment.
When max_concurrent_runs > 1, a run-specific orch.toml can set optim.lr below the trainer scheduler's min_lr. This check then runs inside MultiLoRAOptimizer.optimizer_creation_hook during MultiRunManager.synchronize_state() (which invokes creation hooks on every rank without exception handling), so one invalid newly discovered run raises here and terminates the shared trainer instead of being rejected through the existing register_config_validation_hook / config_validation_error.txt path in runs.py. Register this constraint as a multi-run config validation hook before the run is synchronized.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in 6baedcb. In multi-run this now registers a config-validation hook in setup_multi_scheduler (master rank, alongside validate_lora_rank) that rejects a run whose per-run optim.lr < scheduler.min_lr via the normal config_validation_error.txt path, so one bad run is skipped instead of crashing the shared trainer. The setup_scheduler raise stays as a defensive last line — it's now unreachable for bad multi-run configs since the hook rejects them before the run is created.
…r crash) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. 👍 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
x |
2 participants
Summary
scheduler.min_lris boundedge=0but never checked againstoptim.lr. Whenmin_lr > lr,CosineAnnealingLR(eta_min=min_lr)interpolates upward, so the "minimum" LR becomes the peak and the run trains above its configuredlrfor the whole schedule. Nothing raises.lr=1e-3,min_lr=1e-2: the LR climbs from 0.001 to 0.010 across the decay. The linear path already errors onLinearLR(start_factor=min_lr/lr), but only from deep inside torch.TrainerConfigvalidator fails fast at config load.setup_multi_scheduler, master-rank only, alongsidevalidate_lora_rank) rejects a run whose per-runoptim.lr < scheduler.min_lrthrough the normalconfig_validation_error.txtpath, so one bad run is skipped instead of crashing the shared trainer.setup_schedulerkeeps a defensiveValueErroras a last line. The constant scheduler has nomin_lrand is untouched.Verification
A single-run cosine config with
min_lr > optim.lrraisesValueErrorat construction. The multi-run hook returns(False, ...)for a per-runoptim.lrbelowmin_lrand(True, "")otherwise.min_lr <= lris accepted.