Raise Python exceptions as Lisp (pyerror) errors and guard numpy import

[plandes]

The big change with this pull request is to raise a Lisp pyerror when a Python error is raised. It does this by wrapping all evaluated code in a try catch that looks something like:

try:
  _ = <inserted code>
except Exception as e:
  _ = ExecutionFailure(e)

Both the stack trace and the error are retained and available in the (modified) pyerror. Example:

(handler-case
    (raw-pyexec "raise ValueError('Some error')")
  (pyerror (e)
    (format t "Caught: <~a>~%Message: <~a>~%Exception class: <~a>~%"
        e
        (slot-value e 'py4cl2-cffi::exception-message)
        (slot-value e 'py4cl2-cffi::exception-type))))

produces:

Caught: <Python raised an exception:
Traceback (most recent call last):
  File "<string>", line 3, in <module>
ValueError: Some error
>
Message: <Some error>
Exception class: <ValueError>

For some unknown reason, I was not able to merge in the changes you made last night so I recreated my fork for this pull request (in case you were wondering or if there's any other git weirdness).

[digikar99]

Let me think if there's a better way to do this.

raw-py anyways exists only for backward compatibility; for anything non-trivial, the rest of the interface through pycall, pyvalue, etc is recommended which does the error handling appropriately.

[plandes]

@digikar99 Sounds good. Maybe using a monitor on the Python side is a consideration, or even adding C code to catch Python syntax issues.

Also, since I created the pull request I have noticed other methods silently discard Python side issues such as pymethod. For this reason, in my own project I've added the following Python side wrapper:

class Invoker(object):
    def __init__(self, class_name: str, *args, **kwargs):
        try:
            ci = ClassImporter(class_name, False)
            self._instance = ci.instance(class_name, *args, **kwargs)
            self._args = (args, kwargs)
            self._error = None
        except Exception as e:
            self._instance = None
            self._args = None
            self._error = e

    def invoke(self, method_name: str, *args, **kwargs):
        try:
            meth: Callable = getattr(self._instance, method_name)
            return meth(*args, **kwargs)
        except Exception as e:
            return ExecutionFailure(e)

    def __str__(self):
        return f'{self._instance.__class__}({self._args}), err: {self._error}'

Then check the type and raise as a Lisp error:

(defun python-maybe-raise-error (val)
  (if (and (eq (type-of val) 'python-object)
       (equal (slot-value val 'type)
          "<class '__main__.ExecutionFailure'>"))
      (error 'pyerror
         :format-control (format nil "Python raised an exception:~%~a"
                     (pyslot-value val "stack"))
         :exception-type (pyslot-value val "exception_type")
         :exception-message (pyslot-value val "exception_message"))
      val))

[digikar99]

other methods silently discard Python side issues such as pymethod

I can confirm that there definitely seems to be some bug. I will try to get it fixed soon. Thanks for reporting!

[digikar99]

9c65f0eea1389796bc94ef126ce023ddf578caec should fix the pymethod not raising exceptions issue.

[digikar99]

ed6f6f8dccb224220f0145fa5495acb235f5c50d should add support for capturing the python error even when raised in raw-py.

I have incorporated your commit about importing sys and traceback before numpy, but I felt it better to capture the error output rather than modify how the output is sent to pyrun_simplestring. Modifying how the output is sent to pyrun_simplestring also requires guessing the indentation of the supplied code.

[plandes]

@digikar99 OK I will test it soon. Regarding the indentation in Python: I do not believe it matters as long as it is consistent for the respective block. I also tested with different indentations. That said, I respect your decision in the change.

Then I would argue for breaking out the handling of low level Python communication in a separate function, or even class, so it can be overridden. I might be missing something, but it appears these changes do not address error handling and recovery from raised Python exceptions.

[digikar99]

I do not believe it matters as long as it is consistent for the respective block.

As long as we can detect it correctly, it should indeed be okay; but detection/guess-work would indeed be necessary.

I would argue for breaking out the handling of low level Python communication in a separate function, or even class, so it can be overridden.

Here I'm restricting the low level to whatever is provided by the Python's C-API. We can certainly subclass pyerror if that is required!

I might be missing something, but it appears these changes do not address error handling and recovery from raised Python exceptions.

Ah, okay! So, a big part of the most recent changes was restructuring how the output is read asynchronously from python, so that with-python-output can work correctly. This involved getting the synchronization primitives working in the correct pattern. But in doing so, I also got a equivalent with-python-error-output. The trouble with raw-py and PyRun_SimpleString is that the only official way (as per the C-API) to detect whether or not PyRun_SimpleString resulted in a python error is to check its return value; beyond that, there is no way to find out what the error is through C-API. However, fortunately, even if there is no C-API to detect an error occuring through PyRun_SimpleString, the python interpreter still outputs the error on to the standard error stream. So, I wrapped the call to PyRun_SimpleString inside with-python-error-output. Now, whenever there is an error, hopefully, the error string will be caught which is then passed on to the pyerror.

As for the errors that should be raised through pymethod etc, there was a bug which did not let the control pass to (python-may-be-error) appropriately - this is the function which is responsible for using the C-API to detect and signal python errors.