Simple reproducibility with minimum boilerplate train_cli

[mauvilsa]

What does this PR do?

This is a proposal which is much easier to explain with actual code in a pull request. The exact idea is not discussed in a single issue, instead it relates to multiple. Though the most relevant discussion would be the one in #3720.

I have added to the pull request an rst file which describes the proposal in more detail, so please have a look: lightning_cli.rst.

The main motivation I have for this is to standardize and remove as much as possible boilerplate code in order to have trainings that can be trivially reproduced with a single config file. This config file is automatically saved in the log dir.

The short summary is, users would run a training like:

    python trainer.py --config config.yaml

And then they would be able to reproduce this by running:

    python trainer.py --config lightning_logs/version_X/config.yaml

The point is not rerunning a training that has already been done. But that the config file includes all of the information needed to reproduce it. This is possible because the config file also includes all of the settings from user defined classes.

This proposal overlaps with other already existing features, namely argument parsing and omegaconf. Though I believe it goes way beyond those in a much simpler way, providing several additional advantages. Specifically:

• Single standardized config file including settings for trainer and user defined classes.
• Encourages users to write better code with good type hints and docstrings.
• Implementation of training command line tools require minimum boilerplate code.
• Supports more complex trainer arguments: list, dict, see test_lightning_cli.py#L157-L165.
• The training tool help automatically includes defaults and descriptions in docstrings.
• Supports parsing settings from command line, config file and environment variables.
• Supports tab argument completion if argcomplete is installed.
• Very small amount code in comparison to the number features it provides.

The unit tests test_lightning_cli.py are in part a copy from the existing test_trainer_cli.py tests. Almost all functionalities from previous code are supported. The only exception is that command line arguments for booleans are more strict, not allowing for example "0" or "t", see selected tests in test_trainer_cli.py#L110-L119. Certainly the support for these could be added, though being more strict could actually be a good thing. With argument completion this is even less of an issue.

Since parts of this proposal are new, I surely hope to get lots of feedback to improve the idea further.

Fixes #3076
Fixes #3720
Fixes #4119

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
• Did you read the contributor guideline, Pull Request section?
• Did you make sure your PR does only one thing, instead of bundling different changes together? Otherwise, we ask you to create a separate PR for every change.
• Did you make sure to update the documentation with your changes?
• Did you write any new necessary tests?
• Did you verify new and existing tests pass locally with your changes?
• If you made a notable change (that affects users), did you update the CHANGELOG?

PR review

• Is this pull request ready for review? (if not, please submit in draft mode)

Next steps:

• Deprecate the current argparse support
• Migrate examples

[pep8speaks]

Hello @mauvilsa! Thanks for updating this PR.

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-04-06 12:03:23 UTC

[mauvilsa]

I ping a few more people that could be interested in this. @carmocca @odedbd

[Borda]

@nateraw min have a look, this seems like complete redone our CLI... :]

[carmocca]

Would we deprecate the current argparse functionality (and bolts implementation) if this lands?

[SeanNaren]

This is so so epic! Thanks for an awesome PR 👍

How would this work with shared parameters, i.e I define two models with encoder_layers as an input arg, and I want the same arg to go to both of them? Also curious about inheritance.

I love the trainer_cli idea, but it seems as soon as you go towards custom args, we lose all the niceties of no boilerplate with the LightningArgsParser. It would be nice to have some form of intermediate that allows us to keep things self contained in PL without having to define the args object/instantiate everything ourselves like normal PL!

[mauvilsa]

How would this work with shared parameters, i.e I define two models with encoder_layers as an input arg, and I want the same arg to go to both of them?

I would say there are many ways in which shared parameters could be handled, and the best way to do it maybe would depend on the specific case. This would require a bit more thought.

Also curious about inheritance.

Arguments of base classes are also added correctly, see for instance the example in the unit test https://github.com/omni-us/jsonargparse/blob/master/jsonargparse_tests/signatures_tests.py#L16-L71 . The only requirement is that *args and/or **kwargs must be present. Not sure if there would be cases which this does not cover, but could be improved further.

I love the trainer_cli idea, but it seems as soon as you go towards custom args, we lose all the niceties of no boilerplate with the LightningArgsParser. It would be nice to have some form of intermediate that allows us to keep things self contained in PL without having to define the args object/instantiate everything ourselves like normal PL!

@SeanNaren I am not sure what you mean by "allows us to keep things self contained in PL without having to define the args object/instantiate everything ourselves like normal PL". Could you please explain a bit more.

[SeanNaren]

If I want to specify custom args, I shouldn't need to define all the boilerplate again. I would love to keep it like this (not a really well thought out proposal, but hopefully adds to discussion):

from pytorch_lightning.utilities.jsonargparse_utils import trainer_cli
from mycode import LitModel, LitDataModule, CustomCallback

custom_callback = CustomCallback()

trainer_cli(LitModel, LitDataModule, callbacks=[custom_callback])

from pytorch_lightning.utilities.jsonargparse_utils import LightningArgumentParser, trainer_cli
from mycode import LitModel, LitDataModule

# Define parser
parser = LightningArgumentParser(description='pytorch-lightning trainer',
                                 parse_as_dict=True)
parser.add_argument('--notification_email', default='will@email.com')

trainer_cli(LitModel, LitDataModule, parser=parser)

[teddykoker]

I think @SeanNaren has a good point. Should we implement a system like this, we would definitely require any arbitrary additional arguments to be added.

Additionally, I think we need to draw the line somewhere where lighting ends, and users can use their own solutions. Right now there are so many solutions for python configuration and CLI: typer, hydra, click, fire, I am not sure if it makes sense to build something like this into Lightning, when each user will have their own usecase, and this would add another dependency to worry about breaking.

That being said, I could be wrong, and nevertheless this PR should promote a thoughtful discussion.

[mauvilsa]

@teddykoker I was certainly expecting comments related to adding a dependency and this probably being the reason why in the end this is not merged. Though putting this aside for a moment, I think the most important topic to discuss here is if lightning should out of the box have this kind of easy reproducibility feature with no boilerplate. People that just want to run some experiment shouldn't need to know about the variety of python solutions for configuration or be required to learn how to use one of them and add this logic to their code. I personally think the core idea of this PR fits perfectly the lightning philosophy. But this is just one opinion. I would like to hear the opinion about this from as many people as possible.

[mauvilsa]

If in the end there is a consensus that this is a desired feature, the implementation details would be the second topic to discuss. Many options are possible, including implementing everything in lightning for the sake of avoiding a dependency. Though I would advice against this. Modularity and separation of concerns are basic software engineering principles. Makes more sense to use a package specifically designed for this than reimplementing. I can continue working on this pull request based on the ongoing discussion, though I would do so using jsonargparse, since I am the main developer and I can easily steer its development to whatever the lightning community requires.

[mauvilsa]

Added a unit test for callbacks as argument from the command line. Also included in the documentation the need for the all extras require.

[awaelchli]

Is LightningCLI restricted to Lightning classes?
So some_object = LightningCLI(MyCustomClass) won't work, even if type annotations are given? Will it be possible to support profilers, loggers etc too?
Why are the Trainer methods embedded inside the cli? Is it not possible to call trainer.fit() or test at the desired place?
I only read the docs so far, so apologies but it sounds like this is not possible.

[mauvilsa]

Hi @awaelchli

Is LightningCLI restricted to Lightning classes? So some_object = LightningCLI(MyCustomClass) won't work, even if type annotations are given?

It works for custom classes. That is the idea. People implement their own modules and data modules and then use the cli. The only requirement is that the classes inherit from LightningModule and LightningDataModule. This is the requirement for lightning's Trainer, right?

Will it be possible to support profilers, loggers etc too?

Yes, they should be working. There are no unit tests specifically for profilers and loggers. But these have type hints similar to callbacks for which there are unit tests so there is no reason why it wouldn't work. Moreover there are several unit tests in jsonargparse that already check this kind of behavior. Trainer has too many parameters and possible values for me to test everything. It is needed that people try it out in their use cases to identify problems.

Why are the Trainer methods embedded inside the cli? Is it not possible to call trainer.fit() or test at the desired place?

I am not entirely sure what you mean. The purpose of LightningCLI is to load configuration and then run trainer.fit avoiding to write this boilerplate. If you want to do something slightly different then you can implement a new cli inheriting from LightningCLI and only needing to modify some of its methods.

I only read the docs so far, so apologies but it sounds like this is not possible.

No problem. I am glad that someone else is looking at it. I guess it will become a bit clearer if you try it out with a simple example.

[carmocca]

Ready to go! 💃

[tchaton]

LGTM !

[Borda]

🎉 💟 🐰 great work @mauvilsa and @carmocca!