[RFC] Clean up Logger saving of Profiler output

[daniellepintz]

Background

Currently we write our checkpoint and profiler output to the Logger, in the case that the user does not specify a path.
(Note: this is for the case of one logger, the case of multiple loggers to be discussed in a different issue).

The exact priority followed is:
Profiler:

1. if a dirpath is provided write to it
2. if not, write to the Logger
3. if there is no Logger, write to default_root_dir

Checkpointing:

1. if a dirpath is provided write to it
2. if not, and a weights_save_path is provided in the Trainer, write to it (Deprecated in [RFC] Deprecate weights_save_path from the Trainer constructor #11768)
3. if not, write to the Logger
4. if there is no Logger, write to default_root_dir

Motivation

There are several issues and inconsistencies with how we currently write to the Logger.

1. One issue is that for the checkpoint output we put it in a nice directory called "checkpoints", but for the profiler output it is not in a nice directory called "profiler" but instead the individual output files are just sitting in one of the logger's directories.

2. There are inconsistencies in terms of where the loggers store the profiler output (in the case where no dirpath is provided). Most of the loggers store the output in their save_dir, but the TensorBoardLogger stores the profiler output in the same place where it stores the checkpoints - save_dir/name/version.
   https://github.com/PyTorchLightning/pytorch-lightning/blob/e15a66412cc220fa241ec7cbb64b339a2f124761/pytorch_lightning/trainer/trainer.py#L2095-L2107

Why do we have different behavior for the TensorBoardLogger? Also, why is the profiler output not in the same place as the checkpointing output?

Pitch

1. When logging to the logger, put the profiler output in a directory called "profiler".

2. All loggers should store their output in save_dir/name/version. Introduce a new property on the Logger Base API called log_dir (TBLogger and CSVLogger already have it), which returns this path, and is the place to store profiler and checkpointing output.

This will allow us to clean up this code here:
https://github.com/PyTorchLightning/pytorch-lightning/blob/e15a66412cc220fa241ec7cbb64b339a2f124761/pytorch_lightning/trainer/trainer.py#L2095-L2107

And also massively simplify this code:
https://github.com/PyTorchLightning/pytorch-lightning/blob/e15a66412cc220fa241ec7cbb64b339a2f124761/pytorch_lightning/callbacks/model_checkpoint.py#L582-L596

replacing it with simply ckpt_path = os.path.join(trainer.log_dir, "checkpoints") after #11768 is also complete.

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

[awaelchli]

I tried to do this once too, standardizing the log dir, without success #7543. We can try again :)

[tshu-w]

I've been struggling with the situation where trainer.log_dir doesn't make sense when using other loggers and can only use TensorBoardLogger. For instance, SaveConfigCallback saves the configuration under trainer.log_dir, but for different runs, default_root_dir and logger.save_dir are usually the same, which cause saving conflict, as I raised in the previous issue #7542 which @awaelchli failed to fix in #7543.

https://github.com/PyTorchLightning/pytorch-lightning/blob/7d1075d2116f8ceda9f7bf12cc083fbc1eb0f32e/pytorch_lightning/trainer/trainer.py#L2202-L2213

I like the idea that use save_dir/name/version as log_dir on the Logger Base, but I found a lot of difficulties when I tried to implement it in practice. The following are some of the troubles I encountered for reference. I have only looked at wandb and neptune, which may be more complicated when considering other loggers.

1. The name and version of NeptuneLogger are None at setup when ModelCheckpoint __resolve_ckpt_dir because the actual neptune run object is initialize later than setup.
2. save_dir, name, and version are not always can be found in third-party loggers, or cannot be modified by the user.
   1. User cannot customize Neptune version currently, see Feature Request: use custom_run_id to change the run id neptune-ai/neptune-client#876
   2. Wandb hardcodes its run_dir Allow wandb.init to set wandb.run.dir. wandb/wandb#1620 https://github.com/wandb/client/blob/5dc9a15a3f666005a1f726e9430a178da2151463/wandb/sdk/wandb_settings.py#L618 and neptune hardcodes its save_dir, which causes the use of these loggers to store multiple directories, one for the PL and one for the Loggers themselves.
3. If save_dir, name, and version are not required, there should be a uniform policy for generating them, but each logger may be different, see 2

[tshu-w]

@awaelchli @daniellepintz Considering the above complex situation, I have a proposal which solve 1 2 3 as I mentioned in previous comment #12028 (comment):

1. Every logger should have save_dir, name, version as init_args with default values for PL own uses (e.g., saving checkpoints, profiler outputs, and cli config). The default value of them are : str = './', : str = 'lightning_logs' and : Optional[str] = None, respectively.
2. We can use the incremented number as the generation strategy for version, as TensorBoardLogger does, if users don't specified, or timestamp.
3. We just need to make sure that the third party logger stores its own logs in default_root_dir/save_dir (in case the script directory is not available after the experiment), because they all have their own directory format, and it is impossible to make them follow save_dir/name/version see Several issues with WandbLogger output directory structure in offline mode #12036 https://github.com/wandb/client/blob/5dc9a15a3f666005a1f726e9430a178da2151463/wandb/sdk/wandb_settings.py#L618.
4. If possible, we should map save_dir, name and version to the logger initialization parameters (for example, dir, name and id in wandb init), and if not (like netpune), we should also log these three values to the loggers.
5. These three properties of loggers should return their own values, not third-party values, especially if they cannot be mapped in 4.
6. In this case, it is natural to make the dir, name, and version of multiple loggers consistent (through class variable?) and there is no question of which folder to store them.
7. For none logger case? Using DummyLogger? Should we decouple storage from logger? default_root_dir is not a good place to save all things as there will be conflicts

BTW, should I raise a new RFC? If so, it would me much appreciated you can help me to write in the format like this issue as my English is not well.

[daniellepintz]

Hi @tshu-w thanks for your analysis here! Yes, these are all things I am aware of and planning to address as part of this issue. No need to raise a new RFC since the proposal here covers it :)

[tshu-w]

Cool, looking forward to seeing the finalized plan, so I can help modify the Loggers.