WIP:removed unused Trainer kwargs from init

[oplatek]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  discussed on slack
• Did you read the contributor guideline, Pull Request section?
• Did you make sure to update the docs?
• Did you write any new necessary tests?
  current tests should cover this
• If you made a notable change (that affects users), did you update the CHANGELOG?

What does this PR do?

It removes kwargs from Trainer which are not used.

My take on using kwargs on function:
All kwargs should have a consumer.

f_ok(a, b, **kwargs):
    # ok
     f2(**kwargs) 

f_ko(a, b, **kwargs):
    # kwargs are just for future convenience for future implementations/ for overriden functions
    # allows passing unsupported arguments and creates confusion that the args are supported
    # if the kwargs are not consumed
     f3(a, b)

f_also_ko(a, b, **kwargs):
     # same arguments as above - all kwargs needs to be consumed
     f4(a, b, kwargs['c'])

Did you have fun?

🙃

[oplatek]

FYI: all other usecases seems to be ok
https://github.com/PyTorchLightning/pytorch-lightning/search?q=kwargs&unscoped_q=kwargs

[Borda]

@tullie @williamFalcon do you remember why we had it there?

[Borda]

LGTM 🚀

[oplatek]

@Borda You were right about the inheritance.

The base class needs to pass the arguments to the child class (the base class cannot know the argument at the time of writing).

See search order and practical advice section here
https://rhettinger.wordpress.com/2011/05/26/super-considered-super/

Marking the PR WIP

[oplatek]

Maybe we can check if the Trainer is a final class i.e. the constructor is not called from derived class and then performed check on arguments

let me think about how to do it properly

[codecov]

Codecov Report

Merging #1180 into master will not change coverage by %.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #1180   +/-   ##
======================================
  Coverage      89%     89%           
======================================
  Files          62      62           
  Lines        3164    3164           
======================================
  Hits         2810    2810           
  Misses        354     354           

[williamFalcon]

i think it’s there to support inheritance. this is kind of a trivial change that might do more harm than good down the road as we keep evolving the package. my suggestion is to keep it as is.

[Borda]

i think it’s there to support inheritance. this is kind of a trivial change that might do more harm than good down the road as we keep evolving the package. my suggestion is to keep it as is.

exactly it was my justification when we talked about it in slack and later it was verified by @oplatek so I am closing this on...