LightningCLI trainer_defaults get dumped as Python object

[awaelchli]

Bug description

When providing an object like the logger to the trainer_defaults, it gets dumped as a Python object to the config file, which results in an invalid config file to load back into the CLI.

trainer:
  logger: <lightning.pytorch.loggers.wandb.WandbLogger object at 0x16e4db050>

What version are you seeing the problem on?

v2.0, master

How to reproduce the bug

import torch
from lightning.pytorch import LightningModule
from torch.utils.data import DataLoader, Dataset

from lightning.pytorch.cli import LightningCLI
from lightning.pytorch.loggers import WandbLogger


class RandomDataset(Dataset):
    def __init__(self, size, length):
        self.len = length
        self.data = torch.randn(length, size)

    def __getitem__(self, index):
        return self.data[index]

    def __len__(self):
        return self.len


class BoringModel(LightningModule):
    def __init__(self):
        super().__init__()
        self.layer = torch.nn.Linear(32, 2)

    def training_step(self, batch, batch_idx):
        return self.layer(batch).sum()

    def configure_optimizers(self):
        return torch.optim.SGD(self.layer.parameters(), lr=0.1)

    def train_dataloader(self):
        return DataLoader(RandomDataset(32, 64), batch_size=2)


if __name__ == "__main__":
    LightningCLI(BoringModel, trainer_defaults={
        "max_steps": 1,
        "logger": WandbLogger(project="my-project")
    })

Error messages and logs

Inspect the config.yaml file in the current working directory. It contains the logger dumped as an object. It's unclear what the resolution to this could be. Ideally we would want

logger:
    - class_path: lightning.pytorch.loggers.wandb.WandbLogger
       init_args:
           ....

in the config file, but since we can't know the init args that were provided at the time of instantiation, it's hard to make this work. For the time being, it might be better to just drop the entry with a warning so that loading the config doesn't fail.

Environment

Current environment

#- Lightning Component (e.g. Trainer, LightningModule, LightningApp, LightningWork, LightningFlow):
#- PyTorch Lightning Version (e.g., 1.5.0): 2.1.0dev
#- Lightning App Version (e.g., 0.5.2): -
#- PyTorch Version (e.g., 2.0): 2.1
#- Python version (e.g., 3.9): 3.11
#- OS (e.g., Linux): MacOS
#- CUDA/cuDNN version: -
#- GPU models and configuration: -
#- How you installed Lightning(`conda`, `pip`, source): source
#- Running environment of LightningApp (e.g. local, cloud): local

More info

jsonargparse==4.25.0

cc @carmocca @mauvilsa

[carmocca]

I think this is meant to use lazy_instance: https://lightning.ai/docs/pytorch/latest/cli/lightning_cli_expert.html#class-type-defaults

        "logger": lazy_instance(WandbLogger, project="my-project")

[awaelchli]

I forgot about it, I think that makes the most sense. In this case we could raise an error for all non-pimitive types and suggest to use lazy_instance?

[carmocca]

@mauvilsa Could that break valid use-cases of serializing non-primitive types as strings?

If it's fine, should it be done in jsonargparse?

[mauvilsa]

There is already a warning for this. Maybe for some reason it is not showing?
https://github.com/omni-us/jsonargparse/blob/ba9c330f4c4cc3959e2412a60900b3d32215919e/jsonargparse/_typehints.py#L1205-L1211

[carmocca]

It does appear to me with --print_config. I just hadn't scrolled to the top of the logs to see it.

But for somebody who inspects their config.yaml after training, it's very likely they will miss it in the training logs.

Still, if there's no valid use case for serializing as a string, it would help to be strict and make it an error

[mauvilsa]

Making it an error is problematic. Types and defaults might come from signatures in third party packages. These most of the time are not possible to change by a developer.

A more viable option is that the key+value is not included in the serialization. Though, I am not sure if this would be easy to do.

[carmocca]

I don't think that's better because then people will wonder where it is.

What if we keep the current behaviour but jsonargparse adds a YAML comment on top of the fields that were serialized with the warning information? It won't work with JSON config files though.

[awaelchli]

Making it an error is problematic.

Would it still be problematic if it's done in Lightning rather than jsonargparse? All the user needs to do is to change to lazy_instance when passing the trainer_defaults to the CLI object.

[mauvilsa]

What if we keep the current behaviour but jsonargparse adds a YAML comment on top of the fields that were serialized with the warning information? It won't work with JSON config files though.

pyyaml doesn't have support for comments. There is a header comment because this can be printed before the yaml content. ruyaml does have comment support, but making that a requirement is not good.

Since anyway there is no valid value to set, the message could be in the value.

Would it still be problematic if it's done in Lightning rather than jsonargparse?

If it is done in Lightning and is only limited to what is in trainer_defaults, then there wouldn't be an issue to give an error and not allow this.

[carmocca]

It should be for any parser.set_defaults() call to cover the case of overriding add_arguments_to_parser

[mauvilsa]

You are right, it could be parser.set_defaults() taking care of this. Though now that I thought about it a bit more, it is not so easy to implement. But I can look into it.