Navigation Menu

Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add default_config_files argument to LightningCLI.__init__ #15174

Closed
VictorY-1Qbit opened this issue Oct 18, 2022 · 10 comments 路 Fixed by #15651
Closed

Add default_config_files argument to LightningCLI.__init__ #15174

VictorY-1Qbit opened this issue Oct 18, 2022 · 10 comments 路 Fixed by #15651
Assignees
Labels
docs Documentation related lightningcli pl.cli.LightningCLI

Comments

@VictorY-1Qbit
Copy link

VictorY-1Qbit commented Oct 18, 2022

馃殌 Feature

I suggest to add a default_config_files parameter in LightningCLI.__init__.

Motivation

The only way I found to change the default file to load the CLI configuration is though the parser_kwargs :

LightningCLI(parser_kwargs={'default_config_files': ['config.yaml']})

Which took me a while to find because my first try was to override add_default_arguments_to_parser that way :

def add_default_arguments_to_parser(self, parser: LightningArgumentParser) -> None:
         super().add_default_arguments_to_parser(parser)
         parser.set_defaults(config='config.yaml')

Which is not working for some reason (related to #15038 ?).

I think that using a configuration file is a common practice (or at least it should be), thus setting up a default path should be straightforward.

Pitch

It seems more natural to pass this argument directly to LightningCLI (it seems to already work that way for description, env_prefix and default_env).

According to what I undestrand the change is fairly easy to make. Just by adding default_config_files in LightningCLI.__init__, then sending to _setup_parser_kwargs in this section:

main_kwargs, subparser_kwargs = self._setup_parser_kwargs(
    parser_kwargs or {},  # type: ignore  # github.com/python/mypy/issues/6463
    {"description": description, "env_prefix": env_prefix, "default_env": env_parse, "default_config_files": default_config_files},
)

cc @carmocca @mauvilsa

@VictorY-1Qbit VictorY-1Qbit added the needs triage Waiting to be triaged by maintainers label Oct 18, 2022
@awaelchli awaelchli added feature Is an improvement or enhancement lightningcli pl.cli.LightningCLI and removed needs triage Waiting to be triaged by maintainers labels Oct 18, 2022
@mauvilsa
Copy link
Contributor

Adding more parameters to the __init__ of LightningCLI increases duplication which goes against the "don't repeat yourself" (DRY) principle. In fact, I kind of regret adding the env and description parameters. Having the parameters in the __init__ might have some benefits, but since LightningCLI joins together many components it is simpler to explain that parameters related to the parser go in parser_kwargs, parameters related to save config go in save_config_kwargs, override of trainer defaults go in trainer_defaults. So I have some mixed feelings.

@mauvilsa
Copy link
Contributor

Regarding parser.set_defaults(config='config.yaml'), this is a misunderstanding of the difference between a config command line argument and default config files. I am not sure what happens when someone does that, but I have taken note of it to improve the documentation and give a clear error message when someone tries to do this.

@VictorY-1Qbit
Copy link
Author

VictorY-1Qbit commented Oct 26, 2022

I totally understand your concern about duplication, this is a complicated tradeoff. You are in a better position than me to decide.

Then, I suggest to clarify the documentation for these points:

  1. An example of utilisation for parser_kwargs and save_config_kwargs
  2. A clear explanation of the configuration override order
  3. The difference between default config files and the config command line argument

Also I wonder if these two "Common Workflows" sections shouldn't grouped since they are related:

@carmocca carmocca added docs Documentation related and removed feature Is an improvement or enhancement labels Oct 26, 2022
@mauvilsa
Copy link
Contributor

Also I wonder if these two "Common Workflows" sections shouldn't grouped since they are related:
* Configure hyperparameters from the CLI
* Eliminate config boilerplate

This is part of what I proposed in pull request #14976. You are welcome to review it if you like.

Then, I suggest to clarify the documentation for these points:
1. An example of utilisation for parser_kwargs and save_config_kwargs
2. A clear explanation of the configuration override order
3. The difference between default config files and the config command line argument

I completely agree. Though it might be best to do this after #14976 is merged.

@mauvilsa
Copy link
Contributor

For the time being some answers to your questions:

  1. The difference between default config files and the config command line argument

default_config_files are location pattern(s) that are searched for, and if found, are used to override the defaults from the source code. A common usage is typical linux config locations for an app, e.g. ['/etc/myapp.yaml', '~/.myapp.yaml']. default_config_files can be defined for a parser that does not expose a command line option to manually provide a config file (though LightningCLI always add one).

  1. A clear explanation of the configuration override order

The parsing override order is a follows:

  • defaults defined in the source code
  • matched default config files in the order defined in default_config_files
  • environment variables (if enabled) first entire config variable and then individual argument variables
  • command line arguments in order left to right (might include config files)

@mauvilsa
Copy link
Contributor

@carmocca @Borda @awaelchli how about if we deprecate description, env_prefix and env_parse from LightningCLI.__init__ such that all parameters related to the parser should be given through parser_kwargs?

@carmocca
Copy link
Member

I agree.

@mauvilsa
Copy link
Contributor

@VictorY-1Qbit you can review #15651 since it adds your suggestions in #15174 (comment).

@VictorY-1Qbit
Copy link
Author

@mauvilsa Yes, I'm a bit busy but I do that ASAP.

@victor-yon
Copy link

@mauvilsa I reviewed it with my personal account (this one)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation related lightningcli pl.cli.LightningCLI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants