Fix local variables being collected into module_arguments dict

[awaelchli]

Before submitting

• Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • not these changes specifically but it was discussed that we should fix any issues
• 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?

Fixes issues 6 and 7 in #1937

• local variables that are not also init args will not be collected
• "self", "*args" or "**kwargs" can now be renamed to something else and collection still works
• removed the redundant recursive child search because the "self" instance is ALWAYS the most specific type anyway.

The new tests that I added fail on master, proofing that the this PR actually fixes the issue.
I will follow up with another PR that will add even more tests for things like multiple inheritance etc.

[codecov]

Codecov Report

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

@@          Coverage Diff           @@
##           master   #2048   +/-   ##
======================================
  Coverage      86%     86%           
======================================
  Files          75      75           
  Lines        4705    4709    +4     
======================================
+ Hits         4064    4068    +4     
  Misses        641     641           

[awaelchli]

kindly asking @yukw777 to also have a look at this since you were involved in the hparams PR.

[justusschock]

`some questions, but otherwise looks good to me :)

[justusschock]

Wasn't there a reason with inheritance for this?

[awaelchli]

I tested it and I found that child is always the specific type, i.e. "self", even if self.auto_collect() is called in a super class.
So in a concrete example:

class A(object):
    def __init__(self, *args, **kwargs):
        print(self)


class B(A):
    def __init__(self, *args, **kwargs):
        super().__init__(*args, **kwargs)
        print(self)

B()  # prints twice <__main__.B object at 0x0000021540C94288>

so really it does not matter where we collect "self", it will always be the child (leaf node) in the inheritance tree.
The previous inheritance tests still pass.

[awaelchli]

The child = _get_latest_child(frame) does not break anything, it's just redudant as far as I can tell.

[justusschock]

why also ignore *args and **kwargs?

Shouldn't we collect them as well and use them for reinstantiation/loading? Otherwise the module might be instantiated with different params which could cause incompatibilities with module and loaded checkpoint.

[awaelchli]

Because if we have this class

class B(object):

    def __init__(self, *args, **kwargs):
        super().__init__()
        something = kwargs.get('something')
        self.auto_collect_arguments()

and we call it like this
B(something=1, other=2)
then we want to module arguments to be
dict(something=1, other=2)
and not
dict(args=[], kwargs={something=1, other=2})

It was like this before, this has not changed. The code that I added there just makes it so that the name *args and **kwargs is not hardcoded, but can be named whatever the user wants.
The Pyhton inspection magic makes it possible to determine which is which.

[awaelchli]

That's the explanation why the ignore, and the second part of your question:
The args inside **kwargs are still collected, see this line
local_args.update(local_args.get('kwargs', {}))

[Borda]

the same as here #2039 (comment)

[yukw777]

I don't have any huge concerns about this. I do worry _collect_init_args() may be too brittle given the heavy usage of reflections, but I recognize why we need it... good test coverage and documentation would go a long way.

[awaelchli]

good test coverage and documentation would go a long way.

by a long way you mean it's not ready yet?
I would agree, yes. The todo list is still long and I have test cases in mind already to follow up. One thing we should take a close look at soon is multiple inheritance.

[mergify]

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

[williamFalcon]

@awaelchli @Borda good effort on tackling this!

I propose the final API for init interactions is as follows:

case 1

User explicitly says what they want to save.

class LitModel(...):

    def __init__(self, conf):
        self.save_hyperparameters(conf)

Case 2:

User wants to save all the init stuff.
They can do it all manually or ask us to do it automatically

class LitModel(...):

    def __init__(self, arg1, arg2, arg3):
        # manually
        self.save_hyperparameters(arg_name=arg1, arg_name=arg2, arg_name=arg3)
        
        # equivalent automatic
        self.save_hyperparameters()

Case 3:

They want to save ONLY some of the init stuff

class LitModel(...):

    def __init__(self, arg1, arg2, arg3):
        # manually
        self.save_hyperparameters(arg_name=arg2)

Special cases:

• namespace

    def __init__(self, hparams):
        # manually
        self.save_hyperparameters(hparams)

• dict

    def __init__(self, some_dict):
        # manually
        self.save_hyperparameters(some_dict)

• omniconf

    def __init__(self, conf):
        # manually
        self.save_hyperparameters(conf)

• anything they want

    def __init__(self, some_random_alternative_to_config):
        # manually
        self.save_hyperparameters(some_random_alternative_to_config)

@PyTorchLightning/core-contributors

[yukw777]

@awaelchli nah i just meant that as a general observation, not specific to this PR, which looks good to me!

[Borda]

the API changes will be applied in #2047