Hang when using Lightning CLI from config file and DDP

[gau-nernst]

🐛 Bug

When I use Lightning CLI to instantiate training from a YAML config file with DDP, I got the error below and the process will hang.

RuntimeError: SaveConfigCallback expected xxxx/lightning_logs/version_5/config.yaml to NOT exist. Aborting to avoid overwriting results of a previous run. You can delete the previous config file, set `LightningCLI(save_config_callback=None)` to disable config saving, or set `LightningCLI(save_config_overwrite=True)` to overwrite the config file.

Ctrl + C will not work. I have to kill the process manually.

I suspect this is because each DDP process tries to write the same config file to disk. Passing either save_config_callback=None or save_config_overwrite=True like suggested in the error message solves the issue.

To Reproduce

# bug.py
import torch
from torch.utils.data import Dataset, DataLoader

from pytorch_lightning import LightningModule
from pytorch_lightning.utilities.cli import LightningCLI

class RandomDataset(Dataset):
    def __init__(self, size, num_samples):
        self.len = num_samples
        self.data = torch.randn(num_samples, 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 forward(self, x):
        return self.layer(x)

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

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

    def training_step(self, batch, batch_idx):
        loss = self(batch).sum()
        self.log("train_loss", loss)
        return {"loss": loss}

    def validation_step(self, batch, batch_idx):
        loss = self(batch).sum()
        self.log("valid_loss", loss)

    def test_step(self, batch, batch_idx):
        loss = self(batch).sum()
        self.log("test_loss", loss)

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

if __name__ == "__main__":
    LightningCLI(BoringModel)

# bug.yaml
trainer:
  gpus: 2
  strategy: ddp
  max_epochs: 10

Run Lightning CLI

python bug.py fit --config bug.yaml

Expected behavior

There should be no error by default, without explicitly passing save_config_callback=None or save_config_overwrite=True to Lightning CLI.

Environment

• CUDA:
  - GPU:
  - GeForce RTX 3090
  - GeForce RTX 3090
  - GeForce RTX 3090
  - GeForce RTX 3090
  - available: True
  - version: 11.3
• Packages:
  - numpy: 1.21.2
  - pyTorch_debug: False
  - pyTorch_version: 1.10.0
  - pytorch-lightning: 1.5.5
  - tqdm: 4.62.3
• System:
  - OS: Linux
  - architecture:
  - 64bit
  - ELF
  - processor: x86_64
  - python: 3.8.12
  - version: Dataset only available when the trainer is instantiated #86~18.04.1-Ubuntu SMP Fri Jun 18 01:23:22 UTC 2021

Additional context

cc @carmocca @mauvilsa

[carmocca]

Is your problem the error message or that you had to kill the process manually?

If it's the former, it's just warning you that the previous config file will be overwritten, and telling you what options you have.

If it's the latter, it's a general problem with DDP where processes can become unresponsive.

[gau-nernst]

Once I got the error message, the process is stuck there. I don't see any training taking place, no Lightning training process bar whatsoever. The GPUs seem to be occupied when viewed in nvidia-smi, so I'm not sure if training is still going on?

My problem is more about the training script fails to run when I use Lightning CLI + DDP with default settings (without passing save_config_callback=None or save_config_overwrite=True). The error message is not just a warning, but the training in fact hangs (or at least I don't have any indicator it is still running). Having to kill the process manually is just an observation, not really the main problem.

I think using Lightning CLI + DDP is a popular combination? It will be great if it works without passing extra arguments by the users.

[carmocca]

The first time it will work seamlessly. The CLI automatically saves the configuration file when it starts running for reproducibility.

Then, the second time it runs, if you are setup to use the same config file you'll get this error because we try to not mess with the user configuration unless we are explicitly told to (hence the flag).

In your error message you can see:

expected xxxx/lightning_logs/version_5/config.yaml to NOT exist

This means your experiment is reusing the same directory as a previous experiment.

[carmocca]

@mauvilsa This questions comes up a lot. Do you think we should read the existing configuration file and check if they will be equal? Avoiding the error if they are.

If yes, should this be a byte check or a dictionary check? The former has the benefit that it would respect any user comments.

[gau-nernst]

The first time it will work seamlessly. The CLI automatically saves the configuration file when it starts running for reproducibility.

Then, the second time it runs, if you are setup to use the same config file you'll get this error because we try to not mess with the user configuration unless we are explicitly told to (hence the flag).

In your error message you can see:

expected xxxx/lightning_logs/version_5/config.yaml to NOT exist

This means your experiment is reusing the same directory as a previous experiment.

It was a new, separate experiment. It was not writing to an old experiment. I didn't set the experiment folder, so Lightning created a new folder under lightning_logs/. I suspect the error was because the 2nd DDP process also tried to save the config file, of the same experiment, to the same folder. Perhaps to fix this, Lightning can tell only 1 DDP process to save the config file?

[carmocca]

Lightning can tell only 1 DDP process to save the config file?

We do this already: https://github.com/PyTorchLightning/pytorch-lightning/blob/0ee78e96efa71f962f4cfafa73588e4f6a0b8bce/pytorch_lightning/utilities/cli.py#L417-L424

[gau-nernst]

I think I found the issue (and the solution?)

https://github.com/PyTorchLightning/pytorch-lightning/blob/e8169bbd467467383588ba5ac18f55e7d8e1d84f/pytorch_lightning/utilities/cli.py#L410-L424

The save config callback checks for config file existence BEFORE checking if the process is the 1st process. A simple fix will be moving the config file existence check inside the 1st process check

    def setup(self, trainer: Trainer, pl_module: LightningModule, stage: Optional[str] = None) -> None:
        # save the config in `setup` because (1) we want it to save regardless of the trainer function run
        # and we want to save before processes are spawned
        log_dir = trainer.log_dir
        assert log_dir is not None

        if trainer.is_global_zero:
            config_path = os.path.join(log_dir, self.config_filename)
            if not self.overwrite and os.path.isfile(config_path):
                raise RuntimeError(
                    f"{self.__class__.__name__} expected {config_path} to NOT exist. Aborting to avoid overwriting"
                    " results of a previous run. You can delete the previous config file,"
                    " set `LightningCLI(save_config_callback=None)` to disable config saving,"
                    " or set `LightningCLI(save_config_overwrite=True)` to overwrite the config file."
                )
                
            # save only on rank zero to avoid race conditions on DDP.
            # the `log_dir` needs to be created as we rely on the logger to do it usually
            # but it hasn't logged anything at this point
            get_filesystem(log_dir).makedirs(log_dir, exist_ok=True)
            self.parser.save(
                self.config, config_path, skip_none=False, overwrite=self.overwrite, multifile=self.multifile
            )

I tested the above in my local Lightning installation and the problem goes away. Perhaps I can make a PR for this if it's okay?

[carmocca]

trainer.log_dir will broadcast the resulting directory to all processes. So all should have the same path.

https://github.com/PyTorchLightning/pytorch-lightning/blob/08e661ff7266f298a6e904fa04931698e20b2b89/pytorch_lightning/trainer/trainer.py#L1836

This is why we don't need to check the rank for the exception.

Can you validate if that's your case?

[gau-nernst]

I'm not quite sure I understand your point, and what I should validate? Isn't it because all processes have the same path, the 2nd process will see the existence of a config file, which was saved by the 1st process, and throw the runtime error?

I can reproduce my problem with the Boring model as in the original post, at least with Lightning 1.5.5. I deleted the lightning_logs/ directory to have a clean logging folder, to ensure no duplicates or overwriting of experiments. Still, the problem happened.

[carmocca]

Yes, it's possible. Opened #11199 with the fix.

[guitangl]

We are still seeing this issue today, with multi-node training.

Why not just move the code block of file exists check under the " if trainer.is_global_zero:" ?