-
Notifications
You must be signed in to change notification settings - Fork 287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Catch silent subproc_execute failures #2404
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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')}"]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the exitcode
Notice how the first command returns There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL, thank you! |
||
with pytest.raises(Exception) as e: | ||
subproc_execute(cmd, shell=True) | ||
assert "silent failure" in str(e.value) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sometimes commands write debug information to stderr, so we should have a more resilient way of checking if a subprocess ran to completion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wish they didn't but you're right.. unfortunately, subprocess.run doesn't capture PIPESTATUS it seems, only showing the return code for the last command. Do you have any thoughts? The only thing I can think of would be to check stderr for "command not found" or "error", but that's pretty awkward.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't have a great solution.
pipestatus is a bash feature, so this complicates things a little since we use
/bin/sh
as the default shell inShellTask
and depending on the OS that's not bash (e.g. on MacOS/bin/sh
is symlinked to bash:).
A low-lift solution is to write a warning to the flytekit logs that we detected the use of a pipe in the command and that means we might end up not capturing the output of the command. That's not a great solution either, but at least we let the users know that they are in pipeland, which means they will have to handle it (maybe mention pipestatus?). wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I think that's the best solution given the current constraints. It is a bit of a corner case and I don't think we should be expected to handle every failure state. How does the latest commit look?