Errors within try/except of train(self) are misrepresented as checkpointing MisconfigurationException

[l-salewski]

🐛 Bug

I think I found a bug, where errors probably caused by users are misrepresented as checkpointing MisconfigurationException even though the checkpointing is configured correctly.

This happens when errors are raised within training (such as RuntimeErrors or CUDA-OOM errors) and bubble up to the try/except command in def train(self) function, which can be found here: https://github.com/PyTorchLightning/pytorch-lightning/blob/65247ec47cbe8f0254f444f409025585806f9113/pytorch_lightning/trainer/trainer.py#L550
As these errors are not excepted here, the code continues to execute the code in finally. This calls self.train_loop.on_train_end() which proceeds to save a checkpoint. If one monitors on a validation metric, such as val/accuracy, no value has been saved as the error occured during the training. Thus, in turn a MisconfigurationException is raised by the checkpointing code, stating that the monitored metric is not found in the returned metrics.

To Reproduce

• configure checkpointing to monitor a validation metric, which (by definition) must not be logged during training.
• raise any error during training, such as RuntimeError but not KeybordInterrupt (that is the only one which is caught).

Expected behavior

The error should not be glossed over by the finally statement and be raised all the way to the top, so the user can see it and fix the underlying bug.

Environment

• OS: Linux
• env-type: conda
• pytorch-lightning 1.1.4
• pytorch 1.7.0
• python 3.8.6
• CUDA/cuDNN version: 11.0
• GPU models and configuration: One v100, 32GB vRAM

Lazy patch:

A very hacky way of at least letting the user know about their error, is to modify train such that after except KeyboardInterrupt all other errors are caught with except Exception as e and then immediately printing the error with print(e). Unfortunately raising the error with raise e does not work because the finally code is executed first, raising its own MisconfigurationException.

The full code of train would look like this (changes are below the excepted KeyboardInterrupt):

    def train(self):
        self.run_sanity_check(self.get_model())

        # set stage for logging
        self.logger_connector.set_stage("train")

        self.checkpoint_connector.has_trained = False

        # enable train mode
        model = self.get_model()
        model.train()
        torch.set_grad_enabled(True)

        # reload data when needed
        self.train_loop.reset_train_val_dataloaders(model)

        # hook
        self.train_loop.on_train_start()

        try:
            if self.train_loop.should_skip_training():
                return
            # run all epochs
            for epoch in range(self.current_epoch, self.max_epochs):

                # hook
                self.train_loop.on_train_epoch_start(epoch)

                with self.profiler.profile("run_training_epoch"):
                    # run train epoch
                    self.train_loop.run_training_epoch()

                if self.max_steps and self.max_steps <= self.global_step:
                    return

                # update LR schedulers
                self.optimizer_connector.update_learning_rates(interval='epoch')

                # early stopping
                met_min_epochs = epoch >= self.min_epochs - 1
                met_min_steps = self.global_step >= self.min_steps if self.min_steps else True

                if self.should_stop:
                    if met_min_epochs and met_min_steps:
                        return
                    log.info(
                        'Trainer was signaled to stop but required minimum epochs'
                        f' ({self.min_epochs}) or minimum steps ({self.min_steps}) has'
                        ' not been met. Training will continue...'
                    )

        except KeyboardInterrupt:
            rank_zero_warn('Detected KeyboardInterrupt, attempting graceful shutdown...')

            # user could press ctrl+c many times... only shutdown once
            if not self.interrupted:
                self.interrupted = True
                self._state = TrainerState.INTERRUPTED
                self.on_keyboard_interrupt()
        except Exception as e:
            print(e)  # better would be raising it, but that would be executed after the finally
        finally:
            # hook
            self.train_loop.on_train_end()

Test

A test to catch this problem in the future would be to checkpoint on a validation metric, raise any error in the training (except KeyboardInterrupt which is excepted) and assert that it bubbles up all the way.

[justusschock]

Hi @l-salewski ,

Thanks for the heads up. However, I don't believe this to be correct, all Exceptions that are not excepted, are raise automatically after the finally Statement as shown in the Python Docs:

If the exception is not handled by an except clause, the exception is re-raised after the finally clause has been executed.

[l-salewski]

Hi @justusschock ,

thanks for getting back to me. Actually you are correct and I should have gotten that from the beginning, as I actually looked at the Python doc, whilst trying to understanding why reraising the original except did "not work".
For me, VS Code stops at the first exception raised, which is is the MisconfigurationException from within checkpointing and this caused me not to see the original exception, as I was not looking at the full stack trace.

So I guess one could see this as less of a clear bug, but as a usability issue due to the unintuive order of exceptions. The main downside of this unintuive order of exceptions is, that debugging is harder. Thus, I would still argue, that the MisconfigurationException from within checkpointing should not have been raised at all.

[Borda]

@l-salewski do you have any specific suggestions on how to work on the order?

[stale]

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

[l-salewski]

@Borda sorry for not getting back earlier to you.

Unfortunately I am not that familiar with lightnings inner workings to understand why the finally, whose code eventually raises the MisconfigurationException, is needed. Like I said in my earlier comment, I think it would be the best if the MisconfigurationException would not be raised at all if any error is raised during training.
But then I guess there are users who prefer to have at least a checkpoint saved if their code crashes.

[hadim]

I am hitting the same issue here.

When doing hparams tuning, some of the trained models might fail (memory error or other things). The tuning lib I use (optuna) catches any error and saves it in a database for later re-use and analysis while the tuning can still continue in the worker. Since PL 1.2.5 (it wasn't the case a few months ago but can't remember the exact version I was using), if trainer.fit fails the exact cause is "hidden" by MisconfigurationException in ModelCheckpoint (since val_loss does not exist) and it makes it very hard to debug failed models since they all show the same error.

I don't know how to fix that, to be honest, I just wanted to raise that this behaviour makes debugging harder when training a large list of models.

[ananthsub]

@justusschock @shuyingsunshine21 for checkpointing at the end, we should try to save a last.ckpt checkpoint - anything else that relies on the monitor name / metrics is likely to fail for the reason laid out here

[hadim]

Maybe the code after finally such as self.train_loop.on_train_end() should just silently fails?