Recursive inspection of subclasses init in from_argparse_args

[nathanpainchaud]

What does this PR do?

Fixes #3315

This PR improves add_argparse_args and from_argparse_args methods in LightningDataModule and Trainer so that they can support more than one level of subclassing from their base Lightning class.

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

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

Did you have fun?

Make sure you had fun coding 🙃

[Borda]

what would be the case you want Trainer subclass?

[nathanpainchaud]

I haven't personally encountered a need to subclass Trainer, but I thought some other might have (I don't know about any reason why we shouldn't, if there are).

My main motivation to apply it to both LightningDataModule and Trainer was to keep the behavior the same. The methods with the same name behave the same way, and I thought it should be kept that way. I saw talks in this issue that some might want to refactor the argparse methods to avoid duplicating them. So I figured having similar but not identical functions would be more trouble in the end.

[nathanpainchaud]

A question about the exact behavior this should implement: do we want to inspect the signature of the base Lightning class (e.g. LigthningDataModule or Trainer) as well as that of the subclasses? Because right now, I stop at the user class that directly subclasses the base Ligthing class, but I realized this might not be the best decision. Any thoughts?

[codecov]

Codecov Report

Merging #3316 into master will decrease coverage by 7%.
The diff coverage is 84%.

@@           Coverage Diff           @@
##           master   #3316    +/-   ##
=======================================
- Coverage      90%     83%    -7%     
=======================================
  Files          90      90            
  Lines        8157    8552   +395     
=======================================
- Hits         7362    7078   -284     
- Misses        795    1474   +679     

[codecov]

Codecov Report

Merging #3316 into master will increase coverage by 0%.
The diff coverage is 84%.

@@          Coverage Diff           @@
##           master   #3316   +/-   ##
======================================
  Coverage      90%     90%           
======================================
  Files          90      90           
  Lines        8138    8160   +22     
======================================
+ Hits         7349    7369   +20     
- Misses        789     791    +2     

[mergify]

This pull request is now in conflict... :(

[nathanpainchaud]

I noticed that some changes might conflict in the future with PR #3428 which also deals with argparse for Trainer (and I expect LightningDataModule as well eventually). So how do you want to proceed between the 2 PRs @Borda ?

[williamFalcon]

yeah, that PR is an intermediate step.
I think the goal is to use the same logic for the trainer, datamodule and lightning module since we currently have multiple implementations

[mergify]

This pull request is now in conflict... :(

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. If you need further help see our docs: https://pytorch-lightning.readthedocs.io/en/latest/CONTRIBUTING.html#pull-request or ask the assistance of a core contributor here or on Slack. Thank you for your contributions.

[stale]

This pull request is going to be closed. Please feel free to reopen it create a new from the actual master.