Fix save_hyperparameters for multiple inheritance and mixins

[RuRo]

What does this PR do?

This PR slightly changes the save_hyperparameters logic.

Originally, save_hyperparameters captured the arguments from all stack frames. #14151 changed this behaviour. I believe, the intention was to only capture the arguments from the stack frames that correspond to LightningModule/LightningDataModule/etc children.

This was originally implemented by checking if the current class was a subclass of LightningModule/LightningDataModule/etc. Unfortunately, this turned out to be too restrictive of a requirement (imho) and it broke multiple inheritance and mixins in particular. (See #16206 and the added tests in tests/tests_pytorch/models/test_hparams.py)

This PR slightly relaxes this check. Instead of checking issubclass(current_class, ...) we now check isinstance(current_self, ...) which correctly accounts for multiple inheritance MRO shenanigans.

Fixes #16206

Does your PR introduce any breaking changes? If yes, please list them.

I modified the get_init_args function in src/pytorch_lightning/utilities/parsing.py to also return the self argument alongside the local_args dict. I am assuming that get_init_args is not part of the public API. If it is, then this is a breaking change. Please, let me know if this was a breaking change.

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 list all the breaking changes introduced by this pull request?
• Did you update the CHANGELOG? (not for typos, docs, test updates, or minor internal changes/refactors)

Not sure, where is the "unreleased section" in the CHANGELOG. I am assuming, that it's currently missing because you are going through the release process for 1.9.0?

PR review

Anyone in the community is welcome to review the PR.
Before you start reviewing, make sure you have read the 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?

Yes, mother, I had fun.

[awaelchli]

I'm ok with this change since it doesn't contradict #14151 and the existing tests :)
However, I believe in the future we need to be able to guarantee that calling save_hyperparameters leads to a configuration that always can restore the model instance. We can't guarantee it at the moment, because it is allowed to call save_hyperparameters multiple times (for example like here in multiple-inheritance) and this can easily break the contract unless the user handles the passing of args to super-inits in particular ways (like in the test).

[RuRo]

However, I believe in the future we need to be able to guarantee that calling save_hyperparameters leads to a configuration that always can restore the model instance. We can't guarantee it at the moment, because it is allowed to call save_hyperparameters multiple times (for example like here in multiple-inheritance) and this can easily break the contract unless the user handles the passing of args to super-inits in particular ways (like in the test).

To be perfectly honest, I think that the current behaviour in terms of how save_hyperparameters and inheritance interact is already fine.

class A(pl.LightningModule):
    def __init__(self, foo, bar):
        print(f"A({foo=}, {bar=})")
        super().__init__()
        self.save_hyperparameters()

class B(A):
    def __init__(self, bam):
        print(f"B({bam=})")
        super().__init__(foo=2, bar=3)
        self.save_hyperparameters()

model = B(bam=1)
print(model.hparams)
model_copy = B(**model.hparams) # Boom!

This code is broken. But I don't think that this code is broken because of our save_hyperparameters implementation. The reason, why it's broken, is that the __init__ method in B doesn't follow the Liskov substitution principle. If B inherits from A, then you should be able to pass instances of B to any place that expects an instance of A and it should work. This is not the case for the above classes.

Note, that this doesn't have anything to do with pytorch_lightning. This kind of code is broken even in pure python:

class A:
    def __init__(self, foo, bar):
        print(f"A({foo=}, {bar=})")
        super().__init__()

class B(A):
    def __init__(self, bam):
        print(f"B({bam=})")
        super().__init__(foo=2, bar=3)

kwargs_for_A = {"foo": 2, "bar": 3}
model = A(**kwargs_for_A)
model_b = B(bam=1)

# This should work under Liskov substitution:
assert isinstance(model_b, A)
model_specialization = type(model_b)(**kwargs_for_A) # Boom!

So in my opinion, we shouldn't aim to support this kind of broken inheritance. IMHO, it should be sufficient to warn the user or maybe raise an Exception, if we are able to detect such situations, but I don't think that we should try to do anything "smart" to try and "support" this kind of broken inheritance.

[carmocca]

If it is, then this is a breaking change. Please, let me know if this was a breaking change.

This was a breaking change and it broke https://github.com/jdb78/pytorch-forecasting/blob/4ba7a6ec213c395e0903a38a1a6429517dea3464/pytorch_forecasting/models/base_model.py#L260

I'll open a PR adding back compatibility for this