Should we support logger=None in the Trainer?

[akashkw]

Background

Currently, the logger parameter in Trainer has the type Union[LightningLoggerBase, Iterable[LightningLoggerBase], bool]. The correct way to specify no logger is to say logger=False, however previously logger=None would work too (if ignoring type-checking errors).

A recent refactor #11920 changed the behavior such that logger=None results in errors, however this was not seen as an issue at the time because it was undefined behavior. We have since discovered that logger=None was being used in some places, so a fix has been made #12249.

Originally posted by @akashkw in #12249 (comment)

Discussion Questions

What approach should we take here? Should we:

• Change the type of logger to be Optional[Union[LightningLoggerBase, Iterable[LightningLoggerBase], bool]]
• Insist that users pass in the correct types

cc @awaelchli @edward-io @Borda @ananthsub @rohitgr7 @kamil-kaczmarek @Raalsky @Blaizzy @justusschock @kaushikb11

[akashkw]

My opinion is that since we allow users to create their own LightningLoggerBase object and pass it into trainer (for example logger=TensorBoardLogger(), it might give users the impression that logger=None should work as well. That would be my intuition, and I would be surprised to discover that it's not supported.

Of course, if type checking warnings pop up then it is an easy fix, but I'm not sure exactly how obvious those warnings are. There are also large parts of the codebase that are not properly typed, so maybe typing alone shouldn't be the only safeguard in this case.

[awaelchli]

Yes let's fix it