add support for save_hyperparameters with Python Data Class

[tbenst]

Python Data Classes are convenient in that they automatically generate a bunch of boilerplate code for assigning data to a class. They are particularly useful for PyTorch models that have a lot of hyperparameters and thus a lot of boilerplate.

🐛 Bug

I believe #1896 introduced a new bug: when using a data class, save_hyperparameters no longer works since it depends on init args and we instead use __post_init__ with dataclasses. Explicitly passing strings does not work either. Perhaps when passing

Code sample

import pytorch_lightning as pl
from dataclasses import dataclass

@dataclass()
class ConvDecoder(pl.LightningModule):
    imageChannels:int = 3
        
    def __post_init__(self):
        super().__init__()
        # both fail
#         self.save_hyperparameters()
        self.save_hyperparameters('imageChannels')
        
model = ConvDecoder()
model.hparams

Expected behavior

There should be a way to use save_hyperparameters with Data Classes

[awaelchli]

It would be great to have this. However, I wouldn't say this is a bug. save_hyperparameters was never designed with data classes in mind, but it was mentioned before.

or does it say in the docs somewhere that it is supported?

[tbenst]

@awaelchli I think it's a bug as a method should depend on the object not the context in which it's called--IIUC in this case the method can only be called from __init__, which is not documented.

Edit: there is a reference to __init__ in the docs: https://pytorch-lightning.readthedocs.io/en/latest/hyperparameters.html#lightningmodule-hyperparameters. But worth noting that a Python Data class still passes all these args to __init__ so it does seem to meet the requirements as currently worded.

Edit2: just realize that another issue with current implementation based on init args is that if you use the learning rate finder, which mutates the lr in the model, then this updated lr is not logged. The model logs the wrong lr since it is mutated after init

[awaelchli]

ok adding back the bug label.

just realize that another issue with current implementation based on init args is that if you use the learning rate finder, which mutates the lr in the model, then this updated lr is not logged.

that's with the latest version? can you make a separate issue please? thanks

[tbenst]

Yes, I think it’s appropriate to leave in same issue as the learning rate finder mutates the lr parameter of the LightningModule, which is reasonable, and the problem is that save_hyperparameters is based on the init args, when it should save parameters based on their current state when save_hyperparameters is called. Let me know if you think differently and happy to make another issue?

[edenlightning]

@tbenst would you like to send a PR with the changes? I'm still not convinced what the bug is.