Improve error handling in ShellTask

[pradithya]

TL;DR

Improve the error returned by ShellTask. As of now, ShellTask error message is not helpful since it doesn't capture the stderr from the subprocess. Additionally, the error thrown by ShellTask is currently classified as system error instead of user error.

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added

Complete description

• Raise FlyteRecoverableError from ShellTask so that it's classified as user's error and the retry policy specified in the task is respected.
• Improve the error message captured by ShellTask. As of now the error message generated by ShellTask in flyteconsole looks as follows. Notice that it doesn't contain any details of the failed command.

[3/3] currentAttempt done. Last Error: SYSTEM::Traceback (most recent call last):

      File "/usr/local/lib/python3.8/site-packages/flytekit/exceptions/scopes.py", line 165, in system_entry_point
        return wrapped(*args, **kwargs)
      File "/usr/local/lib/python3.8/site-packages/flytekit/core/base_task.py", line 527, in dispatch_execute
        raise e
      File "/usr/local/lib/python3.8/site-packages/flytekit/core/base_task.py", line 524, in dispatch_execute
        native_outputs = self.execute(**native_inputs)
      File "/usr/local/lib/python3.8/site-packages/flytekit/extras/tasks/shell.py", line 220, in execute
        subprocess.check_call(gen_script, shell=True)
      File "/usr/local/lib/python3.8/subprocess.py", line 364, in check_call
        raise CalledProcessError(retcode, cmd)

Message:

    Command '
        export RUN_DATE=2023-07-04 &&
        make run
    ' returned non-zero exit status 2.

SYSTEM ERROR! Contact platform administrators.

With this change, users would be able to see stdout and stderr of the subprocess as part of the error message.

Tracking Issue

flyteorg/flyte#3559

Follow-up issue

N.A.

[welcome]

Thank you for opening this pull request! 🙌

These tips will help get your PR across the finish line:

• Most of the repos have a PR template; if not, fill it out to the best of your knowledge.
• Sign off your commits (Reference: DCO Guide).

[fg91]

I think not every user error should be a recoverable one 🤔

Take a look at how PythonFunctionTask separates user vs system error scopes:

return exception_scopes.user_entry_point(self._task_function)(**kwargs)

[pradithya]

I think not every user error should be a recoverable one

I agree, however, since it's a shell execution I am not sure how can we differentiate recoverable vs non-recoverable user error. I am defaulting to recoverable error so that the users of ShellTask can still leverage the task retry feature.

[fg91]

Right, of course, makes sense 👍
This change is not really backwards compatible though, I wonder whether this is a dealbreaker 🤔

[pradithya]

Yes, there is undoubtedly a different behavior change when a shell task throws an error.

Before: Error in shell task is considered system error thus retried N number of times, where N is 3 by default.

After: Error in shell task is considered user recoverable error. By default, it's not retried and will only be retried if users specify retry in the task's metadata.

However, the shell task didn't respect the retry strategy set by users, and I think it can be considered as bug.

[pradithya]

@wild-endeavor your opinion will be very helpful here.

[pingsutw]

+1, I think shell task should throw user recoverable error

[pingsutw]

qq: Currently, the pod's log didn't show any stdout and stderr?

[pradithya]

Currently it does. However this PR capture the stdout thus need to emit it manually.

[welcome]

Congrats on merging your first pull request! 🎉