Fix: Override hparams via CLI#21455
Fix: Override hparams via CLI#21455Berezin-Leonid wants to merge 13 commits intoLightning-AI:masterfrom
Conversation
for more information, see https://pre-commit.ci
Updated `test_lightning_cli_ckpt_path_argument_hparams_subclass_mode` to match the new resolution logic. Previosly, the test expected the model class from the checkpoint to override the `--model` argument provided in the CLI. With this fix, explicit CLI arguments take precedence. the test now assert that `BoringCkptPathModel` (provided via CLI) is instantiated instead of `BoringCkptPathSubclass` (stored in checkpoint). Also updated `test_lightning_cli_ckpt_path_argument_hparams` to catch `KeyError` (NSKeyError) instaed of expecting `SystemExit`, as the new `set_defaults` mechanism raises a precise key error on mismatch.
mauvilsa
left a comment
There was a problem hiding this comment.
I like the idea of setting the hparams as defaults. So it seems this wasn't as difficult as I thought. There are a few details I think should change as I comment below.
Please change the pull request description avoiding Fixes #21255. I mean, merging this pull request should not close #21255, since it isn't an actual fix for it. It would be a fix for your specific comment, but not the entire issue. As a side note, the fix for #21255 would be #21408.
- Changed error in _parse_ckpt_path and tests for it
|
I've addressed all the feedback |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## master #21455 +/- ##
=========================================
- Coverage 87% 79% -8%
=========================================
Files 270 267 -3
Lines 24059 24008 -51
=========================================
- Hits 20855 18959 -1896
- Misses 3204 5049 +1845 |
- Implemented `_relax_model_requirements` to dynamically toggle the mandatory
status of the model argument during parsing.
- If `--ckpt_path` is present in the CLI arguments, the 'model' key is
temporarily removed from the subparser's required arguments.
- Updated `test_cli.py` to verify that:
1. The CLI works without `--model` when a valid checkpoint is used.
2. A `SystemExit` is still raised with an appropriate error message
if the checkpoint is invalid/empty.
for more information, see https://pre-commit.ci
What does this PR do?
Currently, when
LightningCLIloads a configuration from a checkpoint (using--ckpt_path), the hyperparameters stored in the checkpoint overwrite any arguments passed via the command line. This behavior prevents users from overriding specific parameters (e.g., changing the learning rate) when resuming training or fine-tuning.This PR adjusts the configuration merging logic. Now, arguments provided explicitly via the CLI (including parameters defined in config files via
--config) take precedence over the hyperparameters loaded from the checkpoint.Example:
With this change, the following command will correctly use
lr=0.001instead of the value stored inepoch=1.ckpt:Implementation Details
To achieve this, I modified the loading logic so that hyperparameters from the checkpoint are applied as parser defaults rather than being merged directly into the configuration object.
This leverages the standard
jsonargparsepriority order:--config)This ensures that any value explicitly provided by the user will correctly override the value stored in the checkpoint.
Addressing the conversation in #21255 about CLI override priority.
Before submitting
PR review
Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the review guidelines. In short, see the following bullet-list:
Reviewer checklist
📚 Documentation preview 📚: https://pytorch-lightning--21455.org.readthedocs.build/en/21455/