new PythonThrowable java class to address deficiency in fix for issue #294 #303
Conversation
will parse a string that is the output of Python call to traceback.format_exc. It will then produce either a 'python style' output (via PythonThrowable.printStackTracePython) or 'java style' output (via PythonThrowable.printStackTraceJava). By default the Throwable.printStackTrace() call will use Python format, but can default to java via the PythonThrowable(String,boolean) flag (2nd argument).
|
The circleci build seems to have failed because of some findbugs problem, but when I run gradlew findbugsMain locally it completes successfully. I can't tell by the output what is going wrong with the circleci run. |
from <stdin>. Also added test cases for
|
Again the build apparently failed in findbugs...here is the output from the page at 'Details' above
I can't reproduce this locally...if I run gradlew findbugsMain it finishes successfully, so don't know why it's failing. |
|
Hi Scott, the FindBugs report is here: I'll try to look at the pull request over the weekend. Thanks! |
describing the expected Python stack trace format.
Codecov Report
@@ Coverage Diff @@
## master #303 +/- ##
============================================
+ Coverage 81.49% 81.95% +0.45%
- Complexity 704 725 +21
============================================
Files 84 83 -1
Lines 8625 8571 -54
Branches 383 398 +15
============================================
- Hits 7029 7024 -5
+ Misses 1456 1399 -57
- Partials 140 148 +8
Continue to review full report at Codecov.
|
|
Hi Barthelemy. I've fixed the findbugs issues and the formatting things...although the py4j formatter messed up the example Python stack traces in the PythonThrowable class java docs. The code coverage going down apparently has to do with not testing the getters in PythonThrowable and PythonStackTraceElement. These are both immutable classes and the getters are trivial, so I don't think testing the getters directly are actually needed. |
|
One more thing: I did not change the code in Protocol.getReturnValue() that combines the python output from traceback.format_exc with a String message to create and throw a Py4JException. What I would suggest is that this be changed to something like: new Py4JException(new PythonThrowable(pythonFormatExc)); or perhaps: new Py4JException("Java side message",new PythonThrowable(pythonFormatExc)); |
|
@scottslewis apologies for the long delay. I looked at the code and before I dive into specific questions or code review comments I have some high-level questions:
|
|
Hi Barthelemy,
Although one could raise a PythonThrowable directly, my suggestion would be to still raise a Py4jException and provide a PythonThrowble as a cause of the Py4jException, so as not to break existing clients.
I would say it's more than 'play nicer'...it's more like it would 'play at all' with the large number of java logging and tooling in the java world...making integration much easier and more likely.
It's cleaner and more extensible by design. It also makes it quite easy and natural to inherit from PythonThrowable....for creating java-side exceptions that correspond to Python-side exceptions...if desired...for Py4j directly...or py4j consumers (like me wrt OSGi remote services).
It's meant to represent python-side thrown exceptions, not py4j exceptions. With java's nested/caused-by structure (as opposed to only inheritance) this seems simpler to me. And there's nothing in Py4jException that adds to PythonThrowable. And I do think that nesting PythonThrowable as the cause of Py4jException makes more sense than throwing Py4jException xor PythonThrowable.
WRT the parsing of the Python stack trace string: I haven't been personally involved in the pepping, but by usage and observation it seems quite stable to me...i.e. 2 and 3 seem to have the same output. I haven't looked at chained exception pep but will do so. I would have no problem with sending a structured variable, but that would mean enhancing the py4j protocol. If you are willing to do this it's fine with me. I don't think it would be very possible (for backward compatibility reasons) for Python to change the stack trace string significantly at this point, but if they did so IMHO it's likely to be a Python 4 or Python 5 level change and that's likely going to require a new release of Py4j as well. So although separating on Python side would potentially be more robust, I don't suspect it's going to be an issue in the short term. FWIW one thing that could be done is to wrap the PythonThrowable constructor in a try/catch block, and if it fails for any reason then simply include the String unparsed in Py4jException to be thrown as is done now. This would provide some insurance that the communication wouldn't be broken by having the parsing fail. My suggestion is that this be done. If PythonThrowable is introduced now, it could be changed in the future (new constructor) to take a String[] or some such when the structured stack trace is introduced...or if the parsing fails due to some other change in the stack trace String format.
I don't have strong feelings about this. I could see it as an advantage to do so, but the Python world is far less mature about exception handling in API design, and tooling for same, and nesting exceptions... so it doesn't seem like a driving need to me. I would be more inclined to see if it's needed/desired by consumers before doing the necessary work. |
https://www.python.org/dev/peps/pep-3134/ Also added tests for chained exceptions.
|
Hi Barthelemy. Added support for Python chained exceptions. to PythonThrowable, and added test case. I am going to add another chained exception test case but it's going to take a little doing to generate the python test case so it may be a day or so before it appears. |
|
Hi Scott, thank you very much for your detailed answers. I think I really like using PythonThrowable as a cause of Py4jException. If you could post an example of what the whole stack trace would look like (Java user code calls Python, Python raises an exception, Java prints the stack trace and we see the chained exception Py4JException -> PythonThrowable), I would invite two other contributors to comment, but I am mostly convinced now this is a good addition. The example stack trace may also end up in the documentation considering that this is a common question. |
|
Hi Barthelemy. I will produce additional tests and examples as you describe, but it's going to take a couple of days due to other commitments. Please lmk if that holds things up. One thought: If PythonThrowable is created and set as a cause of Py4jException then there should probably be a new option on the JavaGateway instance...that determines whether the PythonThrowable stack trace prints out as 'python style'...i.e. top exception last or 'java style'...top exception first. The current default...i.e. PythonThrowable(String) is 'python style' but it would probably be good to allow 'java style' by JavaGateway configuration (and then using PythonThrowable(String,boolean) constructor at the appropriate code location. Anyway, I'll create example output both ways . |
|
BTW...I did try to put an example in the javadocs for PythonThrowable and one of the ci checks (findbugs?) complained about it, so I removed it from javadocs. We can put in other docs, though. |
Improved robustness of exception string parsing. Added toString() implementation that presents both java and python stack traces for PythonThrowable in <wrapping exception>.printStackTrace()
|
Hi Barthelemy. I've just committed some more improvements...i.e. added test code for chained exceptions, made the PythonThrowable string parsing more robust (with new/additional tests) and added toString() implementation that allows .printStackTrace() to produce output like this: org.osgi.framework.ServiceException: Service exception on remote service proxy rsid=org.eclipse.ecf.remoteservice.RemoteServiceID[containerID=URIID [uri=py4j://127.0.0.1:25334/python];containerRelativeID=4]
at org.eclipse.ecf.remoteservice.client.AbstractRSAClientService.invoke(AbstractRSAClientService.java:102)
at com.sun.proxy.$Proxy6.sayHello(Unknown Source)
at org.eclipse.ecf.examples.protobuf.hello.consumer.HelloConsumer$1.run(HelloConsumer.java:42)
at java.lang.Thread.run(Unknown Source)
Caused by: org.eclipse.ecf.core.util.ECFException: Could not execute remote call=RemoteCall[method=org.eclipse.ecf.examples.protobuf.hello.IHello.sayHello, parameters=[h: "some other message"
f: "java"
to: "python"
hellomsg: "Hello from java"
x: 1.1
x: 1.2
], timeout=30000]
at org.eclipse.ecf.provider.direct.protobuf.ProtobufClientContainer$ProtoBufDirectRemoteService.invokeSync(ProtobufClientContainer.java:146)
at org.eclipse.ecf.remoteservice.client.AbstractRSAClientService.invoke(AbstractRSAClientService.java:97)
... 3 more
Caused by: py4j.Py4JException: An exception was raised by the Python Proxy
at py4j.Protocol.getReturnValue(Protocol.java:459)
at py4j.reflection.PythonProxyHandler.invoke(PythonProxyHandler.java:108)
at com.sun.proxy.$Proxy1._call_endpoint(Unknown Source)
at org.eclipse.ecf.provider.direct.protobuf.ProtobufCallableEndpointImpl.call_endpoint(ProtobufCallableEndpointImpl.java:76)
at org.eclipse.ecf.provider.direct.protobuf.ProtobufCallableEndpointImpl.call_endpoint(ProtobufCallableEndpointImpl.java:92)
at org.eclipse.ecf.provider.py4j.protobuf.ProtobufPy4jProviderImpl$2$1.call_endpoint(ProtobufPy4jProviderImpl.java:85)
at org.eclipse.ecf.provider.direct.protobuf.ProtobufClientContainer$ProtoBufDirectRemoteService.invokeSync(ProtobufClientContainer.java:144)
... 4 more
Caused by: py4j.PythonThrowable: exeception in class A
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py:16 run
raise Exception('exeception in class A') from exc
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\run.py:45 sayHello
aa.run()
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py:334 wrapper
respb = func(args[0],argInst)
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py:337 wrapper
raise e
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py:155 _raw_bytes_from_java
return getattr(self,methodName)(serializedArgs)
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\bridge.py:523 _call_endpoint
return endpoint._raw_bytes_from_java(methodName,serializedArgs)
at C:\Users\slewis\git\scottslewis.py4j\py4j-python\src\py4j\java_gateway.py:2270 _call_proxy
return_value = getattr(self.pool[obj_id], method)(*params)
---Python Stack---
Traceback (most recent call last):
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 14, in run
b.run()
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py", line 9, in run
1 / 0
ZeroDivisionError: division by zero
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "C:\Users\slewis\git\scottslewis.py4j\py4j-python\src\py4j\java_gateway.py", line 2270, in _call_proxy
return_value = getattr(self.pool[obj_id], method)(*params)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\bridge.py", line 523, in _call_endpoint
return endpoint._raw_bytes_from_java(methodName,serializedArgs)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 155, in _raw_bytes_from_java
return getattr(self,methodName)(serializedArgs)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 337, in wrapper
raise e
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 334, in wrapper
respb = func(args[0],argInst)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\run.py", line 45, in sayHello
aa.run()
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 16, in run
raise Exception('exeception in class A') from exc
Exception: exeception in class A
Caused by: py4j.PythonThrowable: division by zero
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py:9 run
1 / 0
at C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py:14 run
b.run()
---Python Stack---
Traceback (most recent call last):
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 14, in run
b.run()
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py", line 9, in run
1 / 0
ZeroDivisionError: division by zero
Explanation: The top of the stack is this exception: org.osgi.framework.ServiceException. OSGi Remote Services spec requires this runtime exception be thrown if an exception occurs in remote service. The cause of the ServiceException is the underlying exception thrown by the distribution provider. The next level down exception is: Caused by: org.eclipse.ecf.core.util.ECFException. This wrapping exception is thrown if any ECF distribution provider (ECF Remote Services has a pluggable distribution provider approach) throws a runtime exception. The next level down is Caused by: py4j.Py4JException: An exception was raised by the Python Proxy. This is a result of changing line 458 from this (current): to this: Note this passes the python stack trace (in String result) to new PythonThrowable instance for parsing. The cause of the Py4JException is set to the PythonThrowable as discussed in previous comments. The next level down in the stack trace is the output of the top-level PythonThrowable.toString()...i.e. line starting with: Caused by: py4j.PythonThrowable: exeception in class A ... This includes the java (top to bottom) of the Python stack and after ---Python Stack--- the python version. In Eclipse, both the java and python File lines are automatically parsed and a link is created to the appropriate Python line (assuming .py file is accessible in workspace). I decided for testing to put both the java (1st) and python (2nd) stack trace in output. It's possible to use only one or the other. In this case the python exception had set as it's cause another exception (chained) and so the underlying python exception is given as Caused by: py4j.PythonThrowable: division by zero ... Note if this had been caused by another chained python exception (or raised as a result of an exception as per pep 3134) it would have an additional Caused By. It this case the division by zero is the root exception. This example output was generated with this code: try {
HelloMsgContent result = helloService.sayHello(createRequest());
} catch (Exception e) {
e.printStackTrace();
}
if the code had this: try {
HelloMsgContent result = helloService.sayHello(createRequest());
} catch (Exception e) {
PythonThrowable pt = (PythonThrowable) e.getCause().getCause().getCause();
pt.printStackTrace();
}
The output is: Traceback (most recent call last):
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 14, in run
b.run()
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\b.py", line 9, in run
1 / 0
ZeroDivisionError: division by zero
The above exception was the direct cause of the following exception:
Traceback (most recent call last):
File "C:\Users\slewis\git\scottslewis.py4j\py4j-python\src\py4j\java_gateway.py", line 2270, in _call_proxy
return_value = getattr(self.pool[obj_id], method)(*params)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\bridge.py", line 523, in _call_endpoint
return endpoint._raw_bytes_from_java(methodName,serializedArgs)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 155, in _raw_bytes_from_java
return getattr(self,methodName)(serializedArgs)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 337, in wrapper
raise e
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\python\osgiservicebridge\src\osgiservicebridge\protobuf.py", line 334, in wrapper
respb = func(args[0],argInst)
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\run.py", line 45, in sayHello
aa.run()
File "C:\Users\slewis\git\Py4j-RemoteServicesProvider\examples\org.eclipse.ecf.examples.protobuf.hello\python-src\a.py", line 16, in run
raise Exception('exeception in class A') from exc
|
Py4JException is the cause of some other exception (e.g. ServiceException)
|
Hi Scott, thanks a lot for the explanation and the examples. I'll invite other contributors to comment, but I believe this is a nice addition. @JoshRosen @batterseapower @jonahkichwacoders : if you have a few minutes to spare, could you review the example stack trace in the last comment of this issue. You have all been involved in error handling and I would like to have your input on this proposed addition. Essentially, @batterseapower recently added the ability to ship python stack trace to the Java side when an exception occurs in a Python callback. The python stack trace is included in the exception message which is already a major improvement! @scottslewis took that exception trace string and generated a proper stack trace that can be used by IDEs or logging systems. There were two concerns: (1) complexity and general "brittleness" of such implementation and (2) impedance mismatch between Java and Python exception traces. Before diving into the code, I wanted to have your opinion on this feature addition. Thanks! |
|
Hi Barthelemy,
WRT brittleness: Although I don't think PythonThrowable is likely to be brittle, I would suggest that it be deployed with a try/catch (Exception) wrapper (for the PythonThrowable constructor) for an initial release, in order to improve robustness over time 'in the stack trace wild'. Also, people have 'unusual' Python stack trace output, these can be easily added to the test cases.
A word about this: There are basically two main structural differences between Java and Python when it comes to handling stack traces: 1) Java has classes only while Python has functions and classes; 2) by default, java presents the stack trace in 'last exception first' order (Caused By) and Python 'first exception first' order. As with the example in the comment above, the current impl of PythonThrowable.printStackTrace() presents the stack first in Java order, but for PythonThrowable also includes the stack trace (and for caused by) in Python-style/order after ---Python Stack--- To include both with java first was a design choice on my part. It could be presented other ways (e.g. just the java or just the python stack trace rather than both) with this same implementation. Note that PythonThrowable has API to get one, the other, or both programmatically. |
|
Are there plans to merge this? If so, as mentioned in earlier comment, I would suggest the following...so as to prevent a possible PythonThrowable parsing failure (e.g. due to changes in Python format) from breaking things: FWIW one thing that could be done is to wrap the PythonThrowable constructor in a try/catch block, and if it fails for any reason then simply include the String unparsed in Py4jException to be thrown as is done now. This would provide some insurance that the communication wouldn't be broken by having the parsing fail. |
A new class PythonThrowable to take the String output from Python traceback.format_exc(), parse it, and represent the python Exception message and stack trace as a PythonThrowable instance.
The constructor for PythonThrowable will parse a string that is the output of Python call to traceback.format_exc. It will then produce either a 'python style'
stack output (via PythonThrowable.printStackTracePython) or 'java style' output (via PythonThrowable.printStackTraceJava). By default the Throwable.printStackTrace() call will use Python format, but can default to java via the PythonThrowable(String,boolean) flag (2nd argument).
Also includes java test code (PythonThrowableTest class) and small addition to Py4JException class (additional constructor) to allow the easy usage of PythonThrowable.