Changed basic_examples to use LightningCLI

[mauvilsa]

What does this PR do?

Changed autoencoder.py, backbone_image_classifier.py, profiler_example.py and simple_image_classifier.py in pl_examples to use the new LightningCLI. Related to the examples some improvements are also made to LightningCLI that seemed necessary.

Before submitting

• Was this discussed/approved via a GitHub issue? (not for typos and docs)
• 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?
• Did you make sure to update the documentation with your changes? (if necessary)
• Did you write any new necessary tests? (not for typos and docs)
• Did you verify new and existing tests pass locally with your changes?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or internal minor changes/refactorings)

PR review

Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:

• Is this pull request ready for review? (if not, please submit in draft mode)
• Check that all items from Before submitting are resolved
• Make sure the title is self-explanatory and the description concisely explains the PR
• Add labels and milestones (and optionally projects) to the PR so it can be classified

Did you have fun?

Make sure you had fun coding 🙃

[mauvilsa]

@carmocca An example changed to use LightningCLI. It could be argued that seed_everything and test could become part of LightningCLI. But with the current implementation this could be the example and illustrates how to use a couple of methods.

[carmocca]

Should the LightningCLI allow {fit,validate,test,tune,predict}? So by default you can do:

python simple_image_classifier.py fit --trainer.max_epochs=50 or python simple_image_classifier.py validate --trainer.limit_val_batches=30

Also yes, seed_everything should be inside before_instantiate_classes by default as the LightningCLI is about simple reproducibility.

And what do you think about LightningCLI returning the trainer, model, and data instances?

trainer, model, data = MyLightningCLI(LitClassifier, MNISTDataModule)

[mauvilsa]

Should the LightningCLI allow {fit,validate,test,tune,predict}? So by default you can do:

python simple_image_classifier.py fit --trainer.max_epochs=50 or python simple_image_classifier.py validate --trainer.limit_val_batches=30

At first glance it makes sense. But I have only used lightning for training. I will need to look into {validate,test,tune,predict} to be able to have a more informed opinion about this and be able to propose how to implement it.

Also yes, seed_everything should be inside before_instantiate_classes by default as the LightningCLI is about simple reproducibility.

Should I add in this pull request the seed_everything change in LightningCLI?

And what do you think about LightningCLI returning the trainer, model, and data instances?

trainer, model, data = MyLightningCLI(LitClassifier, MNISTDataModule)

This is not possible. It is a class so it can only return the instance. Anyway the instance has these as attributes so there is no much difference.

[carmocca]

I will need to look into {validate,test,tune,predict} to be able to have a more informed opinion about this and be able to propose how to implement it.

Awesome!

Should I add in this pull request the seed_everything change in LightningCLI?

Yes, I think that's okay

[codecov]

Codecov Report

Merging #6862 (e37e754) into master (f645df5) will decrease coverage by 5%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master   #6862    +/-   ##
=======================================
- Coverage      92%     87%    -5%     
=======================================
  Files         194     194            
  Lines       12386   12391     +5     
=======================================
- Hits        11414   10770   -644     
- Misses        972    1621   +649     

[mauvilsa]

Implemented seed_everything in LightningCLI. Also updated two other examples required so that pl_examples/test_examples.py does not fail and give the same result as before.

[carmocca]

The following files will also need to be updated:

https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/run_ddp-examples.sh
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/run_examples-args.sh
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/test_examples.py
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/basic_examples/submit_ddp2_job.sh
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/basic_examples/submit_ddp_job.sh
https://github.com/PyTorchLightning/pytorch-lightning/blob/master/pl_examples/basic_examples/README.md

[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-13 17:46:05 UTC

[mauvilsa]

From the basic_examples only two have not been updated yet: conv_sequential_example.py and profiler_example.py.

conv_sequential_example.py is a special case because both the datamodule and the model require the same parameter batch_size. I think this could be common so I was thinking of adding a feature to jsonargparse to simplify things. Being required to give in a config file the same value in several locations is redundant and error prone. The idea would be to allow linking arguments so that one value is derived from another. In the conv example the cli would only receive data.batch_size but internally the parser would copy this value to model.batch_size. To configure this could be something like parser.link_arguments('data.batch_size', 'model.batch_size'). What do you think?

[carmocca]

conv_sequential_example is probably going to be removed soon, so feel free to skip on updating it.

In the conv example the cli would only receive data.batch_size but internally the parser would copy this value to model.batch_size. To configure this could be something like parser.link_arguments('data.batch_size', 'model.batch_size'). What do you think?

So this ties into value interpolation. Would something like this be possible?

parser.add_argument("--data.batch_size", default="${model.batch_size}")

[mauvilsa]

conv_sequential_example is probably going to be removed soon, so feel free to skip on updating it.

In the conv example the cli would only receive data.batch_size but internally the parser would copy this value to model.batch_size. To configure this could be something like parser.link_arguments('data.batch_size', 'model.batch_size'). What do you think?

So this ties into value interpolation.

It is intended to avoid the need for doing interpolation for cases in which interpolation shouldn't be used because it just adds complexity to the config file even though it is something that should be hard coded. There are other cases in which interpolation is a valid need.

Would something like this be possible?

parser.add_argument("--data.batch_size", default="${model.batch_size}")

No, my idea is not having some special values in default. This would be limiting anyway because this is normally needed when arguments are added automatically from class signatures. It would be configured after the arguments are already added making it possible to still validate using the original type. So it could be running parser.link_arguments('source_key', 'target_key') or even parser.link_arguments('source_key', 'target_key', adapt_function) if the desired link is not just a copy.

[carmocca]

So it could be running parser.link_arguments('source_key', 'target_key') or even parser.link_arguments('source_key', 'target_key', adapt_function) if the desired link is not just a copy.

Would this have the transitive property?

Also, this discussion should continue in the jsonargparse repository for better visibility

[mauvilsa]

Also, this discussion should continue in the jsonargparse repository for better visibility

The question relevant for this pull request was how to handle a case like conv_sequential_example.py and just get an opinion if what I was thinking made sense. If we don't update the conv example in this pull request we can skip any further discussion on this. Anyway the same case happens in computer_vision_fine_tuning.py. So this can be further discussed when that example is being updated.