pytorch-lightning

pytorch-lightning copied to clipboard

Bug description

Currently, lightning.pytorch.loggers.csv_logs.CSVLogger(name: str, ...) the parameter name of the CSVLogger ist typed as str. https://github.com/Lightning-AI/pytorch-lightning/blob/9c8cd4ce68d4477e5f1dcfd4e64c2701693b08fd/src/lightning/pytorch/loggers/csv_logs.py#L97

This contrasts other loggers, e.g. TensorBoardLogger or WandbLogger.

I suggest typing CSVLogger(name: Optional[str], ...) as it is ineed optional: https://github.com/Lightning-AI/pytorch-lightning/blob/9c8cd4ce68d4477e5f1dcfd4e64c2701693b08fd/src/lightning/fabric/loggers/csv_logs.py#L69

What version are you seeing the problem on?

master

How to reproduce the bug

import lightning

lightning.pytorch.loggers.csv_logs.CSVLogger("save_dir", name=None)

$ mypy test.py

Error messages and logs

test.py:3: error: Argument "name" to "CSVLogger" has incompatible type "None"; expected "str"  [arg-type]

cc @borda

@mwip Can you explain why the Optional annotation is needed? Even if it was made optional, the logic would anyway require to set it to "lightning_logs":

if name is None:
    name = "lightning_logs"

So I would prefer to clearly indicate the default value and leave it as is.

@awaelchli Forgive my confusion, but I'm not able to find the line you are referring to. Maybe you'd be able to link it?

Also, this seems to differ from the behavior of this minimal example:

import lightning

csv_logger1 = lightning.pytorch.loggers.csv_logs.CSVLogger(save_dir="save_dir")
csv_logger2 = lightning.pytorch.loggers.csv_logs.CSVLogger(save_dir="save_dir", name=None)
csv_logger3 = lightning.pytorch.loggers.csv_logs.CSVLogger(save_dir="save_dir", name="some_name")

print(f"{csv_logger1.name=}")
# csv_logger1.name='lightning_logs'
print(f"{csv_logger2.name=}")
# csv_logger2.name=''
print(f"{csv_logger3.name=}")
# csv_logger3.name='some_name'

print(f"{csv_logger1.log_dir=}")
# csv_logger1.log_dir='save_dir/lightning_logs/version_0'
print(f"{csv_logger2.log_dir=}")
# csv_logger2.log_dir='save_dir/version_0'

Or am I missing something?

Ok, I initially thought you wanted to make the argument optional in the literal sense (i.e. making the default None). But I should have read more carefully. I am ok with your proposed change if it's just about adding Optional[] around the type. Feel free to submit a PR if you're interested.