Update logic to parse trainer settings from env vars #18876
Conversation
awaelchli
added
bug
awaelchli
force-pushed
the
bugfix/cli-env-parse
branch
from
c63f5d1 to
c3b3a78
Compare
awaelchli
force-pushed
the
bugfix/cli-env-parse
branch
from
8baf30e to
09b6707
Compare
awaelchli
force-pushed
the
bugfix/cli-env-parse
branch
from
8e296b0 to
363ed83
Compare
| @@ -64,9 +64,9 @@ def insert_env_defaults(self: Any, *args: Any, **kwargs: Any) -> Any: | |||
| kwargs.update(dict(zip(cls_arg_names, args))) | |||
| env_variables = vars(_parse_env_variables(cls)) | |||
| # update the kwargs by env variables | |||
| kwargs = dict(list(env_variables.items()) + list(kwargs.items())) | |||
| kwargs = dict(list(kwargs.items()) + list(env_variables.items())) | |||
There was a problem hiding this comment.
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.
| @@ -288,6 +290,21 @@ def test_lightning_env_parse(cleandir): | |||
| assert cli.config.fit.trainer.logger is False | |||
|
|
|||
|
|
|||
| def test_lightning_cli_and_trainer_kwargs_override(cleandir): | |||
There was a problem hiding this comment.
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.
awaelchli
requested review from
williamFalcon,
tchaton,
carmocca,
justusschock and
Borda
as code owners
for more information, see https://pre-commit.ci
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #18876 +/- ##
=========================================
- Coverage 75% 48% -27%
=========================================
Files 443 435 -8
Lines 35377 35224 -153
=========================================
- Hits 26641 16926 -9715
- Misses 8736 18298 +9562 |
There was a problem hiding this comment.
This change worries me a bit. If a unit test now fails, it means it is a breaking change. Though, most worrying is that I am not sure if the change breaks reproducibility, which is one of the main features of LightningCLI. I mean, if an environment variable is set, would this value be in the automatically saved config?
Note that if environment variables should always be enabled, then LightningArgumentParsercan be changed such that default_env is True by default. People could change it to False if they want to. Also, even if default_env=False, environment variable parsing can be enabled without changing the code by setting environment variable JSONARGPARSE_DEFAULT_ENV=true.
|
I agree with Mauricio's comment. If this is only meant for the CLI, we can either change the default or use that special environment variable. I am hesitant about changing the trainer env vars logic like this. It is definitely a breaking change. The original feature, added in 1.0 (#4022) overwrote the Trainer values. But it was changed to the current behaviour in 1.3 (#6510). There must have been a reason to change it in 1.3 like this, so if you still want to change that (unrelated to the CLI decision), it should be explained why that reason would not matter anymore. |
|
I'm sorry, I didn't know that I could set But I definitely strongly object to the accusations that this is a breaking change. The feature for setting Trainer arguments through environment variables is completely undocumented. If it is documented somewhere I'm missing it, I request that you provide proof. |
|
Across the history of the project, many changed features that were undocumented have been considered "breaking changes". I guess each person has their own slightly different sense of what is a "breaking change" and if "is it in the docs?" is yours, then I would understand that you don't see it as a breaking change. With my comment, I did not mean that it was documented, but that users have been able to use these environment variables the way it is now since 1.3. |
What does this PR do?
Fixes #18874
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--18876.org.readthedocs.build/en/18876/
cc @carmocca @mauvilsa @justusschock @awaelchli @Borda