KeyboardInterrupt Signal does not raise KeyboardInterrupt Error

[LarsHill]

🐛 Bug

If a KeyboardInterrupt signal is send during training via e.g. ctrl-c the trainer.fit() method finishes immediately but no KeyboardInterrupt error is raised. Hence, the remaining model training script (everything after the trainer.fit() call) continues as if nothing has happened. Since my training script saves a status file "successfull" to disc if no error occured, raising no KeyboardInterrupt error during training is quite problematic.

To Reproduce

The problem comes from the design of the _call_and_handle_interrupt() method in the lightning trainer class.
This method wraps the entire training and has the following structure:

import time


def main():
    # _call_and_handle_interrupt method which wraps entire training has this structure
    #  However, the BaseException is never reached if a KeyboardInterrupt is caught and thus no error is raised
    try:
        # training here
        for i in range(100):
            time.sleep(1)
    except KeyboardInterrupt as e:
        print('handle KeyBoardInterrupt.')
        # why not raining here as well?
    except BaseException as e:
        # never reached when a KeyboardInterrupt is caught
        print('handle BaseException.')
        raise


if __name__ == '__main__':
    main()

Since the KeyboardInterrupt exception handling has no raise statement no error is raised and the trainer.fit() method exits with no error. It is said that the KeyboardInterrupt exception block will be removed in lightning version 1.7 but I think the current behavior is not expected. At least one should raise the caught KeyboardInterrupt after handling it.

Expected behavior

If I send a KeyboardInterrupt signal while training, I expect the trainer.fit() method to raise that error immediately so that my script finishes without executing further code which relys on the fact that no error occured.

cc @Borda @tchaton @rohitgr7 @akihironitta @justusschock @awaelchli @ninginthecloud

[justusschock]

Hi @LarsHill ,

Thanks for opening this issue. We certainly know about these flaws (which is why it will be removed in 1.7), but I am not sure if we can change the current behaviour (even if it doesn't seem to be intuitive) due to backwards compatibility reasons.

cc @tchaton for this :)

[awaelchli]

We introduced this as a feature. This is "graceful shutdown" and will not re-raise the KeyboardInterrupt on purpose, by design. But alternatively one could make a SystemExit() instead and just terminate the script. If you are blocked by this behavior, you could use the Callback and implement on_exception() to act on the KeyboardInterrupt and re-raise it.

[LarsHill]

Thanks for the replys. @awaelchli I don't really see how this behaviour is a wanted feature but understand that due to backward compatibility it cannot be removed with a simple patch.

The custom KeyboardInterruptCallback, which will raise the KeyboardInterrupt if caught, is an option I'll try out. It would be a workaround until version 1.7 is released :)

[awaelchli]

I don't really see how this behaviour is a wanted feature but understand that due to backward compatibility it cannot be removed with a simple patch.

So there are two aspects to this I think. One is the "graceful" error handling, which means when we press Ctrl+C or send SIGINT, we simply want to avoid that the terminal gets filled with a stack trace to the KeyBoardInterrupt exception. This is the main motivation why we want this "feature". However, I understand that this is not what you are reporting about.

The second aspect is that the script continues to execute and this is what you point out in the code sample. I believe there is not much motivation behind this but it's rather just a random design choice that someone once made and nobody complained about it so far. Don't quote me on that ^^.

So, to respect the first requirement, my proposal was to exit() instead of re-raising keyboard interrupt. I'm not sure if I'm missing a weird use case but this is something we could explore.

try:
       ...
    except KeyboardInterrupt as e:
        print('handle KeyBoardInterrupt.')
        exit()  # instead of re-raising e
    except BaseException as e:
        ...

[carmocca]

Since on_keyboard_interrupt will be removed in 1.7 and the change requested in this issue is not going to be part of the 1.6 release, we can close it because this could only be part of the 1.7 release (as it's a feature) but it's where we drop the functionality.

[awaelchli]

@carmocca Could you point me to issue that proposes to remove the Keyboard interrupt handling? I have objections.

[awaelchli]

I disagree. Regardless of whether the deprecated on_keyboard_interrupt gets removed, we will still have to handle KeyBoardInterrupt and call into the new on_exception hook. My proposal above stands, and the issue is valid to be kept open for discussion.