From 9132cf3e2cb05375bb44abc1c6930c25a24643f5 Mon Sep 17 00:00:00 2001 From: pryce-turner Date: Thu, 9 May 2024 15:12:00 -0700 Subject: [PATCH 1/2] Made outfile ephemeral Signed-off-by: pryce-turner --- flytekit/extras/tasks/shell.py | 5 ++++ .../flytekit/unit/extras/tasks/test_shell.py | 24 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/flytekit/extras/tasks/shell.py b/flytekit/extras/tasks/shell.py index 03f6b61ebc..1fbbc58b7f 100644 --- a/flytekit/extras/tasks/shell.py +++ b/flytekit/extras/tasks/shell.py @@ -80,6 +80,8 @@ def subproc_execute(command: typing.Union[List[str], str], **kwargs) -> ProcessR # Execute the command and capture stdout and stderr result = subprocess.run(command, **kwargs) + assert result.stderr == "" + # Access the stdout and stderr output return ProcessResult(result.returncode, result.stdout, result.stderr) @@ -93,6 +95,9 @@ def subproc_execute(command: typing.Union[List[str], str], **kwargs) -> ProcessR custom dependencies?\n{e}""" ) + except AssertionError: + raise Exception(f"Command: {command}\nexperienced a silent failure, likely due to shell=True:\n{result.stderr}") + def _dummy_task_func(): """ diff --git a/tests/flytekit/unit/extras/tasks/test_shell.py b/tests/flytekit/unit/extras/tasks/test_shell.py index bb06f1a5dc..57f6336652 100644 --- a/tests/flytekit/unit/extras/tasks/test_shell.py +++ b/tests/flytekit/unit/extras/tasks/test_shell.py @@ -370,3 +370,27 @@ def test_subproc_execute_with_shell(): subproc_execute(cmd, shell=True) cont = open(opth).read() assert "hello" in cont + + +def test_subproc_execute_missing_dep(): + cmd = ["non-existent", "blah"] + with pytest.raises(Exception) as e: + subproc_execute(cmd) + assert "executable could not be found" in str(e.value) + + +def test_subproc_execute_error(): + cmd = ["ls", "--banana"] + with pytest.raises(Exception) as e: + subproc_execute(cmd) + assert "Failed with return code" in str(e.value) + + +def test_subproc_execute_shell_error(tmp_path): + # This is a corner case I ran into that really shouldn't + # ever happen. The assert catches anything in stderr despite + # a 0 exit. + cmd = " ".join(["bcftools", "isec", "|", "gzip", "-c", ">", f"{tmp_path.joinpath('out')}"]) + with pytest.raises(Exception) as e: + subproc_execute(cmd, shell=True) + assert "silent failure" in str(e.value) From e2f41dde61cbaeefad738f4601c2d84fdb610b98 Mon Sep 17 00:00:00 2001 From: pryce-turner Date: Mon, 17 Jun 2024 14:41:07 -0700 Subject: [PATCH 2/2] Changed error handling to warn log for pipe with shell commands Signed-off-by: pryce-turner --- flytekit/extras/tasks/shell.py | 11 +++++++---- tests/flytekit/unit/extras/tasks/test_shell.py | 10 ---------- 2 files changed, 7 insertions(+), 14 deletions(-) diff --git a/flytekit/extras/tasks/shell.py b/flytekit/extras/tasks/shell.py index 1fbbc58b7f..ef9cd0c0e1 100644 --- a/flytekit/extras/tasks/shell.py +++ b/flytekit/extras/tasks/shell.py @@ -79,8 +79,14 @@ def subproc_execute(command: typing.Union[List[str], str], **kwargs) -> ProcessR try: # Execute the command and capture stdout and stderr result = subprocess.run(command, **kwargs) + print(result.check_returncode()) - assert result.stderr == "" + if "|" in command and kwargs.get("shell"): + logger.warning( + """Found a pipe in the command and shell=True. + This can lead to silent failures if subsequent commands + succeed despite previous failures.""" + ) # Access the stdout and stderr output return ProcessResult(result.returncode, result.stdout, result.stderr) @@ -95,9 +101,6 @@ def subproc_execute(command: typing.Union[List[str], str], **kwargs) -> ProcessR custom dependencies?\n{e}""" ) - except AssertionError: - raise Exception(f"Command: {command}\nexperienced a silent failure, likely due to shell=True:\n{result.stderr}") - def _dummy_task_func(): """ diff --git a/tests/flytekit/unit/extras/tasks/test_shell.py b/tests/flytekit/unit/extras/tasks/test_shell.py index 57f6336652..65a7a50e39 100644 --- a/tests/flytekit/unit/extras/tasks/test_shell.py +++ b/tests/flytekit/unit/extras/tasks/test_shell.py @@ -384,13 +384,3 @@ def test_subproc_execute_error(): with pytest.raises(Exception) as e: subproc_execute(cmd) assert "Failed with return code" in str(e.value) - - -def test_subproc_execute_shell_error(tmp_path): - # This is a corner case I ran into that really shouldn't - # ever happen. The assert catches anything in stderr despite - # a 0 exit. - cmd = " ".join(["bcftools", "isec", "|", "gzip", "-c", ">", f"{tmp_path.joinpath('out')}"]) - with pytest.raises(Exception) as e: - subproc_execute(cmd, shell=True) - assert "silent failure" in str(e.value)