From 1e6777502ba534bd3aec833356fec5c728eb6f39 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Tue, 16 Jul 2024 17:35:44 -0300 Subject: [PATCH 01/12] Tentative fix for the hook issue Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index 87bd5c4505..ffd179861a 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -5,6 +5,7 @@ from __future__ import annotations import importlib +import logging import sys import traceback from collections import defaultdict @@ -38,6 +39,8 @@ v{version} """ +logger = logging.getLogger(__name__) + @click.group(context_settings=CONTEXT_SETTINGS, name="Kedro") @click.version_option(version, "--version", "-V", help="Show version and exit") @@ -199,6 +202,13 @@ def main( click.echo(message) click.echo(hint) sys.exit(exc.code) + except Exception as error: + logger.error(f"An error has occurred: {error}") + pass + finally: + self._cli_hook_manager.hook.after_command_run( + project_metadata=self._metadata, command_args=args, exit_code=0 + ) @property def global_groups(self) -> Sequence[click.MultiCommand]: From 22977d5281ba7304b9ad45b98a5e31265663e280 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Thu, 18 Jul 2024 11:33:57 -0300 Subject: [PATCH 02/12] Add variable to track if hook was called already Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index ffd179861a..9f8ff4c82e 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -157,6 +157,9 @@ def main( self._cli_hook_manager.hook.before_command_run( project_metadata=self._metadata, command_args=args ) + + hook_called = False + try: super().main( args=args, @@ -172,6 +175,8 @@ def main( self._cli_hook_manager.hook.after_command_run( project_metadata=self._metadata, command_args=args, exit_code=exc.code ) + hook_called = True + # When CLI is run outside of a project, project_groups are not registered catch_exception = "click.exceptions.UsageError: No such command" # click convert exception handles to error message @@ -206,9 +211,10 @@ def main( logger.error(f"An error has occurred: {error}") pass finally: - self._cli_hook_manager.hook.after_command_run( - project_metadata=self._metadata, command_args=args, exit_code=0 - ) + if not hook_called: + self._cli_hook_manager.hook.after_command_run( + project_metadata=self._metadata, command_args=args, exit_code=0 + ) @property def global_groups(self) -> Sequence[click.MultiCommand]: From 6ee73b8e9024f28b7ba67bad2eb4031b50c6e6bb Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Fri, 19 Jul 2024 17:23:18 -0300 Subject: [PATCH 03/12] Properly set exit code when there is an exception Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index 9f8ff4c82e..a71d2a885f 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -40,6 +40,11 @@ """ logger = logging.getLogger(__name__) +logger.setLevel(logging.ERROR) +handler = logging.StreamHandler(sys.stderr) +formatter = logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s") +handler.setFormatter(formatter) +logger.addHandler(handler) @click.group(context_settings=CONTEXT_SETTINGS, name="Kedro") @@ -159,6 +164,7 @@ def main( ) hook_called = False + exit_code = 0 try: super().main( @@ -209,12 +215,17 @@ def main( sys.exit(exc.code) except Exception as error: logger.error(f"An error has occurred: {error}") - pass + exit_code = 1 + self._cli_hook_manager.hook.after_command_run( + project_metadata=self._metadata, command_args=args, exit_code=1 + ) + hook_called = True finally: if not hook_called: self._cli_hook_manager.hook.after_command_run( project_metadata=self._metadata, command_args=args, exit_code=0 ) + sys.exit(exit_code) @property def global_groups(self) -> Sequence[click.MultiCommand]: From 45013cffeabcf44147f0c6a013328c3ea6240c87 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Thu, 25 Jul 2024 02:50:52 -0300 Subject: [PATCH 04/12] Add test coverage for exception on after_command_hook Signed-off-by: Laura Couto --- tests/framework/cli/test_cli.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/framework/cli/test_cli.py b/tests/framework/cli/test_cli.py index 317a596924..672fc97502 100644 --- a/tests/framework/cli/test_cli.py +++ b/tests/framework/cli/test_cli.py @@ -2,6 +2,7 @@ from itertools import cycle from os import rename from pathlib import Path +from unittest.mock import MagicMock, patch import click from click.testing import CliRunner @@ -432,6 +433,23 @@ def test_kedro_cli_with_project(self, mocker, fake_metadata): assert "Global commands from Kedro" in result.output assert "Project specific commands from Kedro" in result.output + @patch("sys.exit") + def test_main_hook_exception_handling(self, mock_sys_exit, fake_metadata): + kedro_cli = KedroCLI(fake_metadata.project_path) + kedro_cli._cli_hook_manager.hook.after_command_run = MagicMock() + + with patch.object( + click.CommandCollection, "main", side_effect=Exception("Test Exception") + ): + result = CliRunner().invoke(kedro_cli, []) + + kedro_cli._cli_hook_manager.hook.after_command_run.assert_called_once_with( + project_metadata=kedro_cli._metadata, command_args=[], exit_code=1 + ) + + mock_sys_exit.assert_called_once_with(1) + assert "An error has occurred: Test Exception" in result.output + @mark.usefixtures("chdir_to_dummy_project") class TestRunCommand: From 66136bd564a1b48c36a66d019214b0b0dde0212e Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Thu, 25 Jul 2024 03:04:28 -0300 Subject: [PATCH 05/12] Add test for the finally block Signed-off-by: Laura Couto --- tests/framework/cli/test_cli.py | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/tests/framework/cli/test_cli.py b/tests/framework/cli/test_cli.py index 672fc97502..9a2644b2dd 100644 --- a/tests/framework/cli/test_cli.py +++ b/tests/framework/cli/test_cli.py @@ -450,6 +450,22 @@ def test_main_hook_exception_handling(self, mock_sys_exit, fake_metadata): mock_sys_exit.assert_called_once_with(1) assert "An error has occurred: Test Exception" in result.output + @patch("sys.exit") + def test_main_hook_finally_block(self, mock_sys_exit, fake_metadata): + kedro_cli = KedroCLI(fake_metadata.project_path) + kedro_cli._cli_hook_manager.hook.after_command_run = MagicMock() + + # No exception is raised, so it should go to the finally block and call the hook + with patch.object(click.CommandCollection, "main"): + result = CliRunner().invoke(kedro_cli, []) + + kedro_cli._cli_hook_manager.hook.after_command_run.assert_called_once_with( + project_metadata=kedro_cli._metadata, command_args=[], exit_code=0 + ) + + mock_sys_exit.assert_called_once_with(0) + assert "An error has occurred:" not in result.output + @mark.usefixtures("chdir_to_dummy_project") class TestRunCommand: From 7b592fea05989bed3a4cad7eb93eb7d0bd95f0fe Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Wed, 31 Jul 2024 15:28:07 -0300 Subject: [PATCH 06/12] Remove redundant logger configuration on cli.py Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 5 ----- 1 file changed, 5 deletions(-) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index a71d2a885f..fe792b1afe 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -40,11 +40,6 @@ """ logger = logging.getLogger(__name__) -logger.setLevel(logging.ERROR) -handler = logging.StreamHandler(sys.stderr) -formatter = logging.Formatter("%(asctime)s - %(name)s - %(levelname)s - %(message)s") -handler.setFormatter(formatter) -logger.addHandler(handler) @click.group(context_settings=CONTEXT_SETTINGS, name="Kedro") From c5c22ebf370030ef4aac5af35806a0a067f93475 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Wed, 31 Jul 2024 15:48:40 -0300 Subject: [PATCH 07/12] Add minimal required logger config to sned messages to stderr Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 1 + 1 file changed, 1 insertion(+) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index fe792b1afe..5fdebdea73 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -40,6 +40,7 @@ """ logger = logging.getLogger(__name__) +logger.addHandler(logging.StreamHandler(sys.stderr)) @click.group(context_settings=CONTEXT_SETTINGS, name="Kedro") From 155a30db85e5928e845ec8d1e742a91d3b207a47 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Wed, 31 Jul 2024 18:55:22 -0300 Subject: [PATCH 08/12] Call sys.exit only once Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index 5fdebdea73..2207cc4157 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -178,6 +178,7 @@ def main( project_metadata=self._metadata, command_args=args, exit_code=exc.code ) hook_called = True + exit_code = exc.code # When CLI is run outside of a project, project_groups are not registered catch_exception = "click.exceptions.UsageError: No such command" @@ -208,20 +209,20 @@ def main( ) click.echo(message) click.echo(hint) - sys.exit(exc.code) + # sys.exit(exc.code) except Exception as error: logger.error(f"An error has occurred: {error}") - exit_code = 1 self._cli_hook_manager.hook.after_command_run( project_metadata=self._metadata, command_args=args, exit_code=1 ) hook_called = True + exit_code = 1 finally: if not hook_called: self._cli_hook_manager.hook.after_command_run( project_metadata=self._metadata, command_args=args, exit_code=0 ) - sys.exit(exit_code) + sys.exit(exit_code) @property def global_groups(self) -> Sequence[click.MultiCommand]: From de64ef76f0dc86447b75412261001d71979a2530 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Wed, 31 Jul 2024 19:09:15 -0300 Subject: [PATCH 09/12] Lint Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index 2207cc4157..3bd40e5125 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -178,7 +178,9 @@ def main( project_metadata=self._metadata, command_args=args, exit_code=exc.code ) hook_called = True - exit_code = exc.code + exit_code = ( + exc.code if isinstance(exc.code, int) else 1 + ) # exc.code can be str, int or None. # When CLI is run outside of a project, project_groups are not registered catch_exception = "click.exceptions.UsageError: No such command" From f6f657b13f4f240090dde95c4d068a18424fae19 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Wed, 31 Jul 2024 21:06:09 -0300 Subject: [PATCH 10/12] Remove comment Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 1 - 1 file changed, 1 deletion(-) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index 3bd40e5125..a5177bf4d8 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -211,7 +211,6 @@ def main( ) click.echo(message) click.echo(hint) - # sys.exit(exc.code) except Exception as error: logger.error(f"An error has occurred: {error}") self._cli_hook_manager.hook.after_command_run( From b66c0fd64e4288dfdbd63f880d5e24e777865179 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Thu, 1 Aug 2024 00:38:54 -0300 Subject: [PATCH 11/12] Move exit into exception block Signed-off-by: Laura Couto --- kedro/framework/cli/cli.py | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/kedro/framework/cli/cli.py b/kedro/framework/cli/cli.py index a5177bf4d8..0892deed35 100644 --- a/kedro/framework/cli/cli.py +++ b/kedro/framework/cli/cli.py @@ -160,7 +160,6 @@ def main( ) hook_called = False - exit_code = 0 try: super().main( @@ -178,9 +177,6 @@ def main( project_metadata=self._metadata, command_args=args, exit_code=exc.code ) hook_called = True - exit_code = ( - exc.code if isinstance(exc.code, int) else 1 - ) # exc.code can be str, int or None. # When CLI is run outside of a project, project_groups are not registered catch_exception = "click.exceptions.UsageError: No such command" @@ -211,19 +207,19 @@ def main( ) click.echo(message) click.echo(hint) + sys.exit(exc.code) except Exception as error: logger.error(f"An error has occurred: {error}") self._cli_hook_manager.hook.after_command_run( project_metadata=self._metadata, command_args=args, exit_code=1 ) hook_called = True - exit_code = 1 + sys.exit(1) finally: if not hook_called: self._cli_hook_manager.hook.after_command_run( project_metadata=self._metadata, command_args=args, exit_code=0 ) - sys.exit(exit_code) @property def global_groups(self) -> Sequence[click.MultiCommand]: From 43054c5664050d58246a2e881dab18a6ffd5cbe4 Mon Sep 17 00:00:00 2001 From: Laura Couto Date: Thu, 1 Aug 2024 00:46:57 -0300 Subject: [PATCH 12/12] Change test Signed-off-by: Laura Couto --- tests/framework/cli/test_cli.py | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/tests/framework/cli/test_cli.py b/tests/framework/cli/test_cli.py index 9a2644b2dd..b9144ebc1b 100644 --- a/tests/framework/cli/test_cli.py +++ b/tests/framework/cli/test_cli.py @@ -434,7 +434,7 @@ def test_kedro_cli_with_project(self, mocker, fake_metadata): assert "Project specific commands from Kedro" in result.output @patch("sys.exit") - def test_main_hook_exception_handling(self, mock_sys_exit, fake_metadata): + def test_main_hook_exception_handling(self, fake_metadata): kedro_cli = KedroCLI(fake_metadata.project_path) kedro_cli._cli_hook_manager.hook.after_command_run = MagicMock() @@ -447,11 +447,10 @@ def test_main_hook_exception_handling(self, mock_sys_exit, fake_metadata): project_metadata=kedro_cli._metadata, command_args=[], exit_code=1 ) - mock_sys_exit.assert_called_once_with(1) assert "An error has occurred: Test Exception" in result.output @patch("sys.exit") - def test_main_hook_finally_block(self, mock_sys_exit, fake_metadata): + def test_main_hook_finally_block(self, fake_metadata): kedro_cli = KedroCLI(fake_metadata.project_path) kedro_cli._cli_hook_manager.hook.after_command_run = MagicMock() @@ -463,7 +462,6 @@ def test_main_hook_finally_block(self, mock_sys_exit, fake_metadata): project_metadata=kedro_cli._metadata, command_args=[], exit_code=0 ) - mock_sys_exit.assert_called_once_with(0) assert "An error has occurred:" not in result.output