reraise exception back to user #1715
Conversation
facebook-github-bot
added
the
CLA Signed
# Conflicts: # submitit/core/submission.py
gwenzek
force-pushed
the
traceback
branch
2 times, most recently
from
cdf8874 to
8e58878
Compare
|
well apparently it's not working correctly for python 3.6 ^^' @Mortimerp9 you can still try it on Python >= 3.8 |
| if outcome == "error": | ||
| if isinstance(original_err, str): | ||
| # Normally original_err is an exception, unless we failed to pickle it. | ||
| trace = original_err | ||
| return utils.FailedJobError( | ||
| f"Job (task={self.task_id}) failed during processing with trace:\n" | ||
| f"----------------------\n{trace}\n" |
There was a problem hiding this comment.
why not just use original_err here? For readability?
| elif self.paths.stdout.exists(): | ||
| log = subprocess.check_output(["tail", "-40", str(self.paths.stdout)], encoding="utf-8") | ||
| stderr = subprocess.check_output(["tail", "-40", str(self.paths.stdout)], encoding="utf-8") |
There was a problem hiding this comment.
you call it stderr but it's reading stdout
| message.extend( | ||
| [f"No error stream produced. Look at stdout: {self.paths.stdout}", "-" * 40, log] | ||
| [f"No error stream produced. Look at stdout: {self.paths.stdout}", "-" * 40, stderr] |
| except Exception as error: # TODO: check pickle methods for capturing traceback; pickling and raising | ||
| except Exception as pickle_error: | ||
| logger.error(f"Could not pickle job result because of {pickle_error}") | ||
| save_error(pickle_error, tmp_path) |
There was a problem hiding this comment.
you don't want to raise the exception higher her like in the other cases?
|
this looks good to me, but I am no expert of the core of submitit, I'll try it out with stopes |
|
Hi @gwenzek! Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours needs attention. You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
|
@jrapin could we merge this one or should we just abandon? |
|
Hi Pierre, the reason I didn't push for it getting merged is that the design is a bit hacky. |
|
Thanks, we ended up not using it until now as the string was often enough. |
@Mortimerp9 was complaining about the "UncompletedJobError" not being very precise.
There can be several cases:
The idea of this PR is to pickle the full exception and reraise it on the executor side when the user is calling
job.result().This will change '1' into the original exception breaking change
This require pickling an exception which isn't possible by default.
I've included tblib in our code base. The amount of code is relatively low.
Basically we are converting the error and the stack frame into a Python object and dropping some fields that are can't be pickled.
Then during unpickling we do the reverse operation so that we can create a convincing exception.
While adding tests I realize that submitit would NOT write a result pickle file if the pickling of
resultwould fail.Because the temp file created in
submitit/submitit/core/submission.py
Line 58 in 4cf1462
submitit/submitit/core/submission.py
Line 64 in 4cf1462
Mortimer may have saw this issue. So I've rewritten a bit the pickling logic to leave a pickle behind even if we can't pickle the result or the exception. (some 2)
I also realized that catching "Exception" might not be enough and maybe we should also handle "BaseException" since otherwise we might exit without leaving a result.pkl behind us. But I left this for future investigations.