Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle Python layer exceptions correctly #2462

Merged
merged 3 commits into from
Aug 6, 2015

Conversation

longjon
Copy link
Contributor

@longjon longjon commented May 15, 2015

(This is a simplified alternative implementation of part of the first commit of #2001, plus a test.)

As noted by @tnarihi, Python layer exceptions are not handled correctly in pycaffe. PyErr_Print is used to display exception text on stderr, but this has the side effect of clearing the exception, which results in an additional SystemError: NULL return without exception set.

The exceptions get passed through correctly if we simply don't try to catch and print them; that's the first commit. (Note that the Python interpreter will print these exceptions, if uncaught; it's only the caffe tool that won't.) Since we'd still like to see the exception text when using the caffe tool, the second commit adds a single, ifdefed, catch/PyErr_Print block to the tool code. (There's no harm in calling PyErr_Print since we know we're not in the interpreter.)

I prefer this approach to the one in #2001, for these reasons:

  • it simplifies the Python wrapper code, removing the duplicate error catching blocks
  • it removes the need to think about where exceptions might occur in future wrapper code
  • it avoids the business of embedding Python calls to read (and restore) the exception
  • calling code (from C++) has the option of avoiding the exception printout (e.g., and dealing with the exception)

The disadvantage of this approach is that:

  • Python exceptions won't be automatically printed when invoking nets with Python layers from C++ code (outside the Python interpreter) other than the caffe tool

Let me know what you think @tnarihi and others, and thanks @tnarihi for pointing out this issue. (Note also that this does not include all of the exception-fixing functionality from #2001, which I'll plan to check out separately.)

Previously, PyErr_Print was used to print Python exceptions. This has
the side effect of clearing the exception, which results in (an
additional) SystemError in the Python interpreter. Exception printing
from the caffe binary tool is re-added in a future commit.
@shelhamer
Copy link
Member

Merging as I agree that this is simpler than the solution in #2001. Thanks Jon and thanks Takuya for raising the issue!

shelhamer added a commit that referenced this pull request Aug 6, 2015
Handle Python layer exceptions correctly
@shelhamer shelhamer merged commit ac6d4b6 into BVLC:master Aug 6, 2015
ctrevino added a commit to Robotertechnik/caffe that referenced this pull request Aug 6, 2015
 Merge pull request BVLC#2462 from longjon/correct-python-exceptions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants