From 72835b21b930f56f94803376a29cff2c0d58b9f8 Mon Sep 17 00:00:00 2001 From: Mauricio Villegas Date: Thu, 11 Nov 2021 14:00:00 +0100 Subject: [PATCH 1/9] Fix SignalConnector._has_already_handler check for callable type --- CHANGELOG.md | 3 +++ pytorch_lightning/trainer/connectors/signal_connector.py | 4 ++-- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0082201aa1cf9..da42754cf0f62 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -121,6 +121,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/). - Fixed `CombinedLoader` and `max_size_cycle` didn't receive a `DistributedSampler` ([#10374](https://github.com/PyTorchLightning/pytorch-lightning/issues/10374)) +- Fixed `SignalConnector._has_already_handler` check for callable type ([#10483](https://github.com/PyTorchLightning/pytorch-lightning/pull/10483)) + + - diff --git a/pytorch_lightning/trainer/connectors/signal_connector.py b/pytorch_lightning/trainer/connectors/signal_connector.py index dc33d1244441f..4e7d691605f9a 100644 --- a/pytorch_lightning/trainer/connectors/signal_connector.py +++ b/pytorch_lightning/trainer/connectors/signal_connector.py @@ -4,7 +4,7 @@ import sys from signal import Signals from subprocess import call -from types import FrameType, FunctionType +from types import FrameType from typing import Callable, List, Union import pytorch_lightning as pl @@ -104,6 +104,6 @@ def _is_on_windows(self) -> bool: def _has_already_handler(self, signum: Signals) -> bool: try: - return isinstance(signal.getsignal(signum), FunctionType) + return callable(signal.getsignal(signum)) except AttributeError: return False From 05e6cf0df9cbcd48dcb8d700c0638b3ae8473648 Mon Sep 17 00:00:00 2001 From: Mauricio Villegas Date: Fri, 12 Nov 2021 07:15:06 +0100 Subject: [PATCH 2/9] Add unit tests for SignalConnector._has_already_handler --- .../connectors/test_signal_connector.py | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/tests/trainer/connectors/test_signal_connector.py b/tests/trainer/connectors/test_signal_connector.py index 3da8c100fe40c..c82d94aa932be 100644 --- a/tests/trainer/connectors/test_signal_connector.py +++ b/tests/trainer/connectors/test_signal_connector.py @@ -19,6 +19,7 @@ import pytest from pytorch_lightning import Trainer +from pytorch_lightning.trainer.connectors.signal_connector import SignalConnector from pytorch_lightning.utilities.exceptions import ExitGracefullyException from tests.helpers import BoringModel from tests.helpers.runif import RunIf @@ -57,3 +58,25 @@ def training_step(self, batch, batch_idx): else: trainer.fit(model) assert trainer._terminate_gracefully == (False if register_handler else terminate_gracefully) + + +def signal_handler(): + pass + +class C: + def signal_handler(self): + pass + +@pytest.mark.parametrize( + ["handler", "expected_return"], + [ + (signal.Handlers.SIG_DFL, False), + (signal_handler, True), + (C().signal_handler, True), + ], +) +def test_has_already_handler(handler, expected_return): + trainer = Trainer() + connector = SignalConnector(trainer) + with mock.patch("signal.getsignal", return_value=handler): + assert connector._has_already_handler(signal.SIGTERM) is expected_return From 22e5264d4ddb0a6c9f5c17c43c82a982f6cb96d4 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Fri, 12 Nov 2021 06:16:50 +0000 Subject: [PATCH 3/9] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- tests/trainer/connectors/test_signal_connector.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/trainer/connectors/test_signal_connector.py b/tests/trainer/connectors/test_signal_connector.py index c82d94aa932be..609605d0b3496 100644 --- a/tests/trainer/connectors/test_signal_connector.py +++ b/tests/trainer/connectors/test_signal_connector.py @@ -63,10 +63,12 @@ def training_step(self, batch, batch_idx): def signal_handler(): pass + class C: def signal_handler(self): pass + @pytest.mark.parametrize( ["handler", "expected_return"], [ From 392061fcb4464ca220dbe3f09cecb63532014e2b Mon Sep 17 00:00:00 2001 From: Mauricio Villegas Date: Fri, 12 Nov 2021 13:39:52 +0100 Subject: [PATCH 4/9] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Adrian Wälchli --- tests/trainer/connectors/test_signal_connector.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/trainer/connectors/test_signal_connector.py b/tests/trainer/connectors/test_signal_connector.py index 609605d0b3496..5004097dec984 100644 --- a/tests/trainer/connectors/test_signal_connector.py +++ b/tests/trainer/connectors/test_signal_connector.py @@ -64,7 +64,7 @@ def signal_handler(): pass -class C: +class SignalHandlers: def signal_handler(self): pass @@ -78,6 +78,7 @@ def signal_handler(self): ], ) def test_has_already_handler(handler, expected_return): + """Test that the SignalConnector detects whether a signal handler is already attached.""" trainer = Trainer() connector = SignalConnector(trainer) with mock.patch("signal.getsignal", return_value=handler): From 6800f02e95b4218d28dc6e91669856f1df7964e7 Mon Sep 17 00:00:00 2001 From: Mauricio Villegas Date: Fri, 12 Nov 2021 13:40:22 +0100 Subject: [PATCH 5/9] Update test_signal_connector.py --- tests/trainer/connectors/test_signal_connector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/trainer/connectors/test_signal_connector.py b/tests/trainer/connectors/test_signal_connector.py index 5004097dec984..a3bfbbbd570ab 100644 --- a/tests/trainer/connectors/test_signal_connector.py +++ b/tests/trainer/connectors/test_signal_connector.py @@ -74,7 +74,7 @@ def signal_handler(self): [ (signal.Handlers.SIG_DFL, False), (signal_handler, True), - (C().signal_handler, True), + (SignalHandlers().signal_handler, True), ], ) def test_has_already_handler(handler, expected_return): From 3f38bf3bf757b6bb798a2f587edb190f8db53b52 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 25 Nov 2021 02:59:03 +0100 Subject: [PATCH 6/9] patch where getsignal is used --- tests/trainer/connectors/test_signal_connector.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/trainer/connectors/test_signal_connector.py b/tests/trainer/connectors/test_signal_connector.py index 863e06de3e450..e7cff2b9a633f 100644 --- a/tests/trainer/connectors/test_signal_connector.py +++ b/tests/trainer/connectors/test_signal_connector.py @@ -124,5 +124,5 @@ def test_has_already_handler(handler, expected_return): """Test that the SignalConnector detects whether a signal handler is already attached.""" trainer = Trainer() connector = SignalConnector(trainer) - with mock.patch("signal.getsignal", return_value=handler): + with mock.patch("pytorch_lightning.trainer.connectors.signal_connector.signal.getsignal", return_value=handler): assert connector._has_already_handler(signal.SIGTERM) is expected_return From 525aa7ee583a52759e6b592adfc7495d4f96ec26 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 25 Nov 2021 03:06:34 +0100 Subject: [PATCH 7/9] check agains DFL --- pytorch_lightning/trainer/connectors/signal_connector.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/pytorch_lightning/trainer/connectors/signal_connector.py b/pytorch_lightning/trainer/connectors/signal_connector.py index c5561a15720ba..571d547c21ac6 100644 --- a/pytorch_lightning/trainer/connectors/signal_connector.py +++ b/pytorch_lightning/trainer/connectors/signal_connector.py @@ -93,10 +93,7 @@ def _is_on_windows(self) -> bool: return sys.platform == "win32" def _has_already_handler(self, signum: Signals) -> bool: - try: - return callable(signal.getsignal(signum)) - except AttributeError: - return False + return signal.getsignal(signum) is not signal.SIG_DFL @staticmethod def _register_signal(signum: Signals, handlers: HandlersCompose) -> None: From f40ce61ae34588f7b3a7c9af875b1470a261b0fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adrian=20W=C3=A4lchli?= Date: Thu, 25 Nov 2021 03:06:48 +0100 Subject: [PATCH 8/9] include test case for IGN --- tests/trainer/connectors/test_signal_connector.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/trainer/connectors/test_signal_connector.py b/tests/trainer/connectors/test_signal_connector.py index e7cff2b9a633f..909b20bfd89cc 100644 --- a/tests/trainer/connectors/test_signal_connector.py +++ b/tests/trainer/connectors/test_signal_connector.py @@ -115,6 +115,7 @@ def signal_handler(self): @pytest.mark.parametrize( ["handler", "expected_return"], [ + (signal.Handlers.SIG_IGN, True), (signal.Handlers.SIG_DFL, False), (signal_handler, True), (SignalHandlers().signal_handler, True), From a94cb9db1bc9ceec3754120aca15ebec79e78eb9 Mon Sep 17 00:00:00 2001 From: Carlos Mocholi Date: Tue, 30 Nov 2021 23:16:46 +0100 Subject: [PATCH 9/9] Use staticmethod --- tests/trainer/connectors/test_signal_connector.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/tests/trainer/connectors/test_signal_connector.py b/tests/trainer/connectors/test_signal_connector.py index e0b288f12cd60..c27806a2b9a88 100644 --- a/tests/trainer/connectors/test_signal_connector.py +++ b/tests/trainer/connectors/test_signal_connector.py @@ -131,7 +131,5 @@ def signal_handler(self): ) def test_has_already_handler(handler, expected_return): """Test that the SignalConnector detects whether a signal handler is already attached.""" - trainer = Trainer() - connector = SignalConnector(trainer) with mock.patch("pytorch_lightning.trainer.connectors.signal_connector.signal.getsignal", return_value=handler): - assert connector._has_already_handler(signal.SIGTERM) is expected_return + assert SignalConnector._has_already_handler(signal.SIGTERM) is expected_return