You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "wjones127 (via GitHub)" <gi...@apache.org> on 2023/12/27 01:09:52 UTC

[PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

wjones127 opened a new pull request, #39380:
URL: https://github.com/apache/arrow/pull/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.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1446381623


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));

Review Comment:
   > It seems we are already retrieving the error indicator and saving the traceback in PythonErrorDetail class, but can be reading the code wrong.
   
   Yes, the `traceback` module isn't being used to retrieve any errors. They are definitely already retrieved.
   
   I am using `traceback.format_exception()` so that our error format is consistent with what Python prints normally, which should be easiest for users to understand. I could instead manually try to reproduce this format, but there's a lot of special logic in https://github.com/python/cpython/blob/3.12/Lib/traceback.py



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1448497227


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PyList_GET_SIZE(formatted.obj());

Review Comment:
   > I assume the function calls will do the type checking internally, correct?
   
   If you're the concrete `PyList` APIs, they will generally assume that it is a list object. Also, fast access macros such as `PyList_GET_ITEM` don't do any error checking.
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39380:
URL: https://github.com/apache/arrow/pull/39380#issuecomment-1890189969

   After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 6fe7480125b7fdb3a000a27fcc9cf464697b8a60.
   
   There were no benchmark performance regressions. 🎉
   
   The [full Conbench report](https://github.com/apache/arrow/runs/20445656734) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1448243639


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),

Review Comment:
   Thanks. I've added error checks to each of the calls into the Python API.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1448245024


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PyList_GET_SIZE(formatted.obj());

Review Comment:
   The [API docs](https://docs.python.org/3/library/traceback.html#traceback.format_exception) 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?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1448245226


##########
python/pyarrow/tests/test_cffi.py:
##########
@@ -414,6 +414,37 @@ def test_export_import_batch_reader(reader_factory):
         pa.RecordBatchReader._import_from_c(ptr_stream)
 
 
+@needs_cffi
+def test_export_import_exception_reader():
+    c_stream = ffi.new("struct ArrowArrayStream*")

Review Comment:
   Done.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1446427840


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),

Review Comment:
   You need to check for errors here.



##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PyList_GET_SIZE(formatted.obj());

Review 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).



##########
python/pyarrow/tests/test_cffi.py:
##########
@@ -414,6 +414,37 @@ def test_export_import_batch_reader(reader_factory):
         pa.RecordBatchReader._import_from_c(ptr_stream)
 
 
+@needs_cffi
+def test_export_import_exception_reader():
+    c_stream = ffi.new("struct ArrowArrayStream*")

Review Comment:
   Can you add a reference to the GH issue?



##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PyList_GET_SIZE(formatted.obj());
+    for (Py_ssize_t i = 0; i < num_lines; ++i) {
+      Py_ssize_t line_size;
+      const char* data =
+          PyUnicode_AsUTF8AndSize(PyList_GET_ITEM(formatted.obj(), i), &line_size);

Review Comment:
   Again, should either type-check and check for errors, or call the more generic `PyObject_StdStringStr`.



##########
python/pyarrow/tests/test_cffi.py:
##########
@@ -414,6 +414,37 @@ def test_export_import_batch_reader(reader_factory):
         pa.RecordBatchReader._import_from_c(ptr_stream)
 
 
+@needs_cffi
+def test_export_import_exception_reader():
+    c_stream = ffi.new("struct ArrowArrayStream*")
+    ptr_stream = int(ffi.cast("uintptr_t", c_stream))
+
+    gc.collect()  # Make sure no Arrow data dangles in a ref cycle
+    old_allocated = pa.total_allocated_bytes()
+
+    def gen():
+        if True:
+            try:
+                raise ValueError('foo')
+            except ValueError as e:
+                raise NotImplementedError('bar') from e
+        else:
+            yield from make_batches()
+
+    original = pa.RecordBatchReader.from_batches(make_schema(), gen())
+    original._export_to_c(ptr_stream)
+
+    reader = pa.RecordBatchReader._import_from_c(ptr_stream)
+    with pytest.raises(OSError) as exc_info:
+        reader.read_next_batch()
+
+    # inner *and* outer exception should be present
+    assert 'ValueError: foo' in str(exc_info.value)
+    assert 'NotImplementedError: bar' in str(exc_info.value)

Review Comment:
   Can you also test that the traceback is present? For example:
   ```suggestion
       assert "raise ValueError('foo')" in str(exc_info.value)
   ```



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1449148925


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PyList_GET_SIZE(formatted.obj());

Review Comment:
   Ah right. I see the distinction now between the error checking and non-error-checking variants. 👍 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1448245330


##########
python/pyarrow/tests/test_cffi.py:
##########
@@ -414,6 +414,37 @@ def test_export_import_batch_reader(reader_factory):
         pa.RecordBatchReader._import_from_c(ptr_stream)
 
 
+@needs_cffi
+def test_export_import_exception_reader():
+    c_stream = ffi.new("struct ArrowArrayStream*")
+    ptr_stream = int(ffi.cast("uintptr_t", c_stream))
+
+    gc.collect()  # Make sure no Arrow data dangles in a ref cycle
+    old_allocated = pa.total_allocated_bytes()
+
+    def gen():
+        if True:
+            try:
+                raise ValueError('foo')
+            except ValueError as e:
+                raise NotImplementedError('bar') from e
+        else:
+            yield from make_batches()
+
+    original = pa.RecordBatchReader.from_batches(make_schema(), gen())
+    original._export_to_c(ptr_stream)
+
+    reader = pa.RecordBatchReader._import_from_c(ptr_stream)
+    with pytest.raises(OSError) as exc_info:
+        reader.read_next_batch()
+
+    # inner *and* outer exception should be present
+    assert 'ValueError: foo' in str(exc_info.value)
+    assert 'NotImplementedError: bar' in str(exc_info.value)

Review Comment:
   Thanks, that is a good idea.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou merged PR #39380:
URL: https://github.com/apache/arrow/pull/39380


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1445819815


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));

Review Comment:
   It seems we are already retrieving the error indicator and saving the traceback in `PythonErrorDetail` class, but can be reading the code wrong.
   Would it be possible to use `exc_traceback_.obj()` similar to `exc_type_.obj()` so there would be no need to use the traceback module?
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1448598588


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,40 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+    RETURN_IF_PYERROR();
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PySequence_Length(formatted.obj());

Review Comment:
   You should also check for failure here (-1 is returned on error).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "AlenkaF (via GitHub)" <gi...@apache.org>.
AlenkaF commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1446962425


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));

Review Comment:
   Ah, I see. Thank you for explaining!



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "wjones127 (via GitHub)" <gi...@apache.org>.
wjones127 commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1449147291


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,40 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+    RETURN_IF_PYERROR();
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PySequence_Length(formatted.obj());

Review Comment:
   Thanks, I should have seen that 🤦 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] GH-37164: [Python] Attach Python stacktrace to errors in `ConvertPyError` [arrow]

Posted by "pitrou (via GitHub)" <gi...@apache.org>.
pitrou commented on code in PR #39380:
URL: https://github.com/apache/arrow/pull/39380#discussion_r1448497227


##########
python/pyarrow/src/arrow/python/common.cc:
##########
@@ -131,6 +138,34 @@ class PythonErrorDetail : public StatusDetail {
   }
 
  protected:
+  Result<std::string> FormatImpl() const {
+    PyAcquireGIL lock;
+
+    // Use traceback.format_exception()
+    OwnedRef traceback_module;
+    RETURN_NOT_OK(internal::ImportModule("traceback", &traceback_module));
+
+    OwnedRef fmt_exception;
+    RETURN_NOT_OK(internal::ImportFromModule(traceback_module.obj(), "format_exception",
+                                             &fmt_exception));
+
+    OwnedRef formatted;
+    formatted.reset(PyObject_CallFunctionObjArgs(fmt_exception.obj(), exc_type_.obj(),
+                                                 exc_value_.obj(), exc_traceback_.obj(),
+                                                 NULL));
+
+    std::stringstream ss;
+    ss << "Python exception: ";
+    Py_ssize_t num_lines = PyList_GET_SIZE(formatted.obj());

Review 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.
   
   



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org