GH-37164: [Python] Attach Python stacktrace to errors in ConvertPyError#39380
GH-37164: [Python] Attach Python stacktrace to errors in ConvertPyError#39380pitrou merged 6 commits intoapache:mainfrom
ConvertPyError#39380Conversation
| &fmt_exception)); | ||
|
|
||
| OwnedRef formatted; | ||
| formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(), |
There was a problem hiding this comment.
You need to check for errors here.
There was a problem hiding this comment.
Thanks. I've added error checks to each of the calls into the Python API.
|
|
||
| std::stringstream ss; | ||
| ss << "Python exception: "; | ||
| Py_ssize_t num_lines = PyList_GET_SIZE(formatted.obj()); |
There was a problem hiding this comment.
You should probably check that we do have a list here. Or you can simply use PySequence_Length and PySequence_GetItem (this code is not performance-critical).
There was a problem hiding this comment.
The API docs claim this should return a list, but it's perhaps possible that they might change to some other sequence. I changed to be generic sequences.
I assume the function calls will do the type checking internally, correct?
There was a problem hiding this comment.
I assume the function calls will do the type checking internally, correct?
If you're using the concrete PyList APIs, they will generally assume that it is a list object (there is no hard rule unfortunately). Also, fast access macros such as PyList_GET_ITEM don't do any error checking.
There was a problem hiding this comment.
Ah right. I see the distinction now between the error checking and non-error-checking variants. 👍
|
|
||
| std::stringstream ss; | ||
| ss << "Python exception: "; | ||
| Py_ssize_t num_lines = PySequence_Length(formatted.obj()); |
There was a problem hiding this comment.
You should also check for failure here (-1 is returned on error).
There was a problem hiding this comment.
Thanks, I should have seen that 🤦
|
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 6fe7480. There were no benchmark performance regressions. 🎉 The full Conbench report has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them. |
…rtPyError` (apache#39380) ### Rationale for this change Users might define Python generators that are used in RecordBatchReaders and then exported through the C Data Interface. However, if an error occurs in their generator, the stacktrace and message are currently swallowed in the current `ConvertPyError` implementation, which only provides the type of error. This makes debugging code that passed RBRs difficult. ### What changes are included in this PR? Changes `ConvertPyError` to provide the fully formatted traceback in the error message. ### Are these changes tested? Yes, added one test to validate the errors messages are propagated. ### Are there any user-facing changes? This is a minor change in the error reporting behavior, which will provide more information. * Closes: apache#37164 Authored-by: Will Jones <willjones127@gmail.com> Signed-off-by: Antoine Pitrou <antoine@python.org>
Rationale for this change
Users might define Python generators that are used in RecordBatchReaders and then exported through the C Data Interface. However, if an error occurs in their generator, the stacktrace and message are currently swallowed in the current
ConvertPyErrorimplementation, which only provides the type of error. This makes debugging code that passed RBRs difficult.What changes are included in this PR?
Changes
ConvertPyErrorto provide the fully formatted traceback in the error message.Are these changes tested?
Yes, added one test to validate the errors messages are propagated.
Are there any user-facing changes?
This is a minor change in the error reporting behavior, which will provide more information.
ConvertPyError#37164