Skip to content

Comments

Add try/catch around python function calls.#5678

Closed
hgaiser wants to merge 2 commits intoBVLC:masterfrom
fizyr-forks:python-exception
Closed

Add try/catch around python function calls.#5678
hgaiser wants to merge 2 commits intoBVLC:masterfrom
fizyr-forks:python-exception

Conversation

@hgaiser
Copy link
Contributor

@hgaiser hgaiser commented Jun 9, 2017

Every Python call from c++ should be guarded with a try/catch to print its Python error. Previously a Python exception would only print :

terminate called after throwing an instance of 'boost::python::error_already_set'

With this PR, caffe will print the entire stack trace where the exception occurred, making debugging 100x easier ;)

@hgaiser
Copy link
Contributor Author

hgaiser commented Jun 26, 2017

Kindly requesting a review.

@shelhamer
Copy link
Member

Hey @hgaiser, thanks for trying to make pycaffe friendlier but it seems the change is failing the exception test for continuous integration. Please have a look at that first.

@hgaiser
Copy link
Contributor Author

hgaiser commented Jun 29, 2017

@shelhamer Apparently PyErr_Print clears the error indicator, so I changed it to fetch and restore the error indicator. It's a bit ugly if you ask me (PyErr_Print shouldn't clear the error indicator IMO) but apparently this is how it is intended by the Python people.

That was the cause of the RuntimeError not being thrown in Python, so the test case works now. We'll see what Travis has to say about it :)

@hgaiser
Copy link
Contributor Author

hgaiser commented Jun 29, 2017

Seems two of the Travis jobs failed, but they appear unrelated to my changes. @shelhamer can you verify if these are something for me to be concerned about ?

@shelhamer
Copy link
Member

Sorry, on second look I'm confused by the need for this PR. When I raise an exception from a Python layer it is properly reported. Under what conditions do you encounter the error in this issue? What version of Python and boost::python are you using?

Seems two of the Travis jobs failed, but they appear unrelated to my changes.

Those assertion failures are very strange. I have not seen them elsewhere, but they do not seem related to this PR either. I'll try to have a closer look soon.

@hgaiser
Copy link
Contributor Author

hgaiser commented Jun 29, 2017

Try with the following prototxt:

name: 'ExceptionNet'

input: "x"
input_dim: 1
input_dim: 1
input_dim: 1
input_dim: 1

layer {
  type: 'Python'
  name: 'exception'
  top: 'exception'
  bottom: 'x'
  python_param {
    module: 'exception_layer'
    layer: 'ExceptionLayer'
  }
}

And corresponding python layer:

import caffe

class ExceptionLayer(caffe.Layer):
        def setup(self, bottom, top):
                pass
        def reshape(self, bottom, top):
                pass
        def forward(self, bottom, top):
                raise RuntimeError
        def backward(self, top, propagate_down, bottom):
                pass

Place them in the same dir and run something like this:

PYTHONPATH=. caffe time --model net.prototxt

The error I'm seeing with master is:

I0630 00:22:24.016942  3414 caffe.cpp:360] Performing Forward
terminate called after throwing an instance of 'boost::python::error_already_set'

The error I am seeing with this PR is:

caffe.cpp:364] Performing Forward
Traceback (most recent call last):
  File "/home/hgaiser/dev/caffe-exception/exception_layer.py", line 9, in forward
    raise RuntimeError
RuntimeError
terminate called after throwing an instance of 'boost::python::error_already_set'

For the record, I'm using python 3.6 and boost 1.64.

@hgaiser
Copy link
Contributor Author

hgaiser commented Jul 25, 2017

@shelhamer any update?

@hgaiser
Copy link
Contributor Author

hgaiser commented Sep 27, 2017

Ping?

@Noiredd
Copy link
Member

Noiredd commented Nov 2, 2017

I have restarted the tests that failed prior to #5973, they failed for unrelated reasons but pass now. We could resume review now, @shelhamer?

@MikalaiDrabovich
Copy link

This could be very helpful for debugging - any updates on merging?

@hgaiser
Copy link
Contributor Author

hgaiser commented Apr 29, 2019

Closing this PR due to inactivity.

@hgaiser hgaiser closed this Apr 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants