-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Update logic to parse trainer settings from env vars #18876
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
Closed
Closed
Changes from all commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
12443ef
update connector
awaelchli 78ea214
undo
awaelchli c3b3a78
precedence
awaelchli 996363f
update
awaelchli 09b6707
test
awaelchli 363ed83
chlog
awaelchli 4c939a9
reset
awaelchli e8e4d84
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] f83aa13
Merge branch 'master' into bugfix/cli-env-parse
awaelchli 6b22a0f
fixtypo
awaelchli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ | |
| import yaml | ||
| from lightning.fabric.plugins.environments import SLURMEnvironment | ||
| from lightning.pytorch import Callback, LightningDataModule, LightningModule, Trainer, __version__, seed_everything | ||
| from lightning.pytorch.accelerators import CPUAccelerator | ||
| from lightning.pytorch.callbacks import LearningRateMonitor, ModelCheckpoint | ||
| from lightning.pytorch.cli import ( | ||
| _JSONARGPARSE_SIGNATURES_AVAILABLE, | ||
|
|
@@ -288,6 +289,21 @@ def test_lightning_env_parse(cleandir): | |
| assert cli.config.fit.trainer.logger is False | ||
|
|
||
|
|
||
| def test_lightning_cli_and_trainer_kwargs_override(cleandir): | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Test doesn't pass on master, because the CLI passes all arguments to the Trainer, including the defaults, and so env variables would never take precedence. |
||
| env_vars = { | ||
| "PL_TRAINER_ACCELERATOR": "cpu", | ||
| "PL_TRAINER_DEVICES": "2", | ||
| "PL_TRAINER_NUM_NODES": "4", | ||
| "PL_TRAINER_PRECISION": "16-true", | ||
| } | ||
| with mock.patch.dict(os.environ, env_vars), mock.patch("sys.argv", [""]): | ||
| cli = LightningCLI(BoringModel, run=False) | ||
| assert isinstance(cli.trainer.accelerator, CPUAccelerator) | ||
| assert cli.trainer.num_devices == 2 | ||
| assert cli.trainer.num_nodes == 4 | ||
| assert cli.trainer.precision == "16-true" | ||
|
|
||
|
|
||
| def test_lightning_cli_save_config_cases(cleandir): | ||
| config_path = "config.yaml" | ||
| cli_args = ["fit", "--trainer.logger=false", "--trainer.fast_dev_run=1"] | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
This is the fix. The env vars now take precendence over the arguments passed by the user.
This functionality is undocumented and so I am ok with changing that. I am confident that this was always the intention.