Catch silent subproc_execute failures #2404
Conversation
Signed-off-by: pryce-turner <[email protected]>
pryce-turner
requested review from
wild-endeavor,
kumare3,
eapolinario,
pingsutw,
cosmicBboy and
samhita-alla
as code owners
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2404 +/- ##
==========================================
- Coverage 75.81% 72.97% -2.84%
==========================================
Files 181 215 +34
Lines 18327 19541 +1214
Branches 2580 3602 +1022
==========================================
+ Hits 13895 14261 +366
- Misses 3830 4660 +830
- Partials 602 620 +18 ☔ View full report in Codecov by Sentry. |
pryce-turner
changed the title
| # 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.
the exitcode 0 only happens because the second command (i.e. gzip) returns a 0. Bash implements a feature called PIPESTATUS, which captures the exit codes of all commands in a pipe. It's just an array that you can verify after the pipe is run, for example:
bash-5.2$ inexistet command --flag | echo bla | gzip -c > out
bash: inexistet: command not found
bash-5.2$ echo "${PIPESTATUS[@]}"
127 0 0
Notice how the first command returns 127 and the other two return 0.
There was a problem hiding this comment.
TIL, thank you!
flytekit/extras/tasks/shell.py
Outdated
| @@ -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 == "" | |||
There was a problem hiding this comment.
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.
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.
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 in ShellTask and depending on the OS that's not bash (e.g. on MacOS /bin/sh is symlinked to bash:
❯ uname -a
Darwin gondor 23.3.0 Darwin Kernel Version 23.3.0: Wed Dec 20 21:31:00 PST 2023; root:xnu-10002.81.5~7/RELEASE_ARM64_T6020 arm64
❯ readlink /private/var/select/sh
/bin/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.
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?
Signed-off-by: pryce-turner <[email protected]>
Why are the changes needed?
Under some circumstances, subproc_execute will exit with 0 but have a non-null stderr.
What changes were proposed in this pull request?
Adds an assert statement to handle 0 exit with stderr.
How was this patch tested?
Unit test to capture AssertionError and ensure the message contains the right content.
Setup process
make setupmake unit_test