remove extra kwargs from Trainer init

[awaelchli]

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 to update the docs?
• Did you write any new necessary tests?
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

• If you misspell a Trainer arg, it will go to kwargs and there will be no error. This should not be.
  You can actually observe this in tests!

[awaelchli]

as you can see in the first commit here, the tests fail because the flags train_check_interval and test_check_interval do not actually exist!

[Borda]

we already had this discussion and it breaks inheritance...

[awaelchli]

where was the discussion? I would like to know how it breaks inheritance.

[Borda]

where was the discussion? I would like to know how it breaks inheritance.

at least in IDE if the child has different arguments, @justusschock we talked about it, right?
see: #1180

[awaelchli]

well, that's very unfortunate, because this PR shows then the tests are flawed if we allow unknown flags to be passed in without throwing an error

[awaelchli]

So I looked at the discussion you link #1180 and I really don't get it.

1. We don't have any subclasses of Trainer
2. Trainer doesn't even call super().init()
3. Some of the Mixins that Trainer inherits from have a init (e.g. TrainerCallbackHookMixin) but they are never called because Trainer does not call super

So I would rather conclude that inheritance already IS broken

@oplatek maybe you can explain it to me in super simple words? :)

[Borda]

@awaelchli I agree with you that the benefit of more strict testing is larger than having Trainer simple to inherit... @PyTorchLightning/core-contributors do you see use-case when user need to inherit Trainer and add/remove some arguments?

[oplatek]

I thought the main arguments were:

1. Users will subclass Trainer class for example create class SubTrainer(Trainer)
2. Users will call super.init() in their SubTrainer class
3. For the SubTrainer is super convenient to just pass **kwargs in SubTrainer.init to the Trainer.init method

Ad 3 I see it as is super convenient but for very rare use case and I see it as error prone when creating Trainer instance e.g. Trainer(max_epochs) vs Trainer(max_pochs) which fails silently and it is quite frequent case

[pep8speaks]

Hello @awaelchli! Thanks for updating this PR.

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

Comment last updated at 2020-05-15 00:07:00 UTC

[awaelchli]

@oplatek I agree with you, except that we don't have to make any compromises.
See the test that I added, it shows that subclassing and parsing the arguments is not problem.
At least I could not find one, but the test in my view proofs the basic use cases of inheritance as I know it.

[codecov]

Codecov Report

Merging #1820 into master will increase coverage by 0%.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1820   +/-   ##
======================================
  Coverage      88%     88%           
======================================
  Files          71      71           
  Lines        4487    4487           
======================================
+ Hits         3947    3954    +7     
+ Misses        540     533    -7     

[justusschock]

@Borda I don't remember having such a discussion with you, but that may also be on me :D

In general I would also opt for removing kwargs wherever possible

[williamFalcon]

nice @awaelchli