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

Changed exception class to be more descriptive for graphviz installation errors #152

Closed
wants to merge 2 commits into from

Conversation

pnb
Copy link
Contributor

@pnb pnb commented Apr 26, 2017

pydot raises a generic Exception when it can't find graphviz installation binaries, which is not ideal. This pull request changes that to a more descriptive and specific class of exception so that it can be handled properly by other code (see keras-team/keras#6398 for the specific motivation).

@johnyf
Copy link
Contributor

johnyf commented Apr 26, 2017

Thank you for the suggestion. An issue with the selected exception FileNotFoundError is it appears to be absent from Python 2.7. It is present in Python 3.6. pydot still declares compatibility with Python 2.7, so it cannot use an exception that is present only in Python 3.

@pnb
Copy link
Contributor Author

pnb commented Apr 26, 2017

Good point! It's been a while since I used Python 2.7. Do you think ImportError would be appropriate in this case then?

@johnyf
Copy link
Contributor

johnyf commented Apr 26, 2017

ImportError is defined as "Raised when the import statement has troubles trying to load a module. ...", but the file not found is a binary executable, not a python module.

@pnb
Copy link
Contributor Author

pnb commented Apr 26, 2017

It's not an exact match for sure. Perhaps raising another OSError with the specific message would be appropriate then?

@johnyf
Copy link
Contributor

johnyf commented Apr 26, 2017

Raising OSError with additional information seems to be a good solution, because it is minimally invasive. It refines the current approach, because OSError is more specific than Exception. In intercepting OSError and replacing it with Exception, the current approach destroys information that can be preserved.

You may want to look at this answer, so that the exact same error be raised, except for a modification to its message. This would allow higher levels (e.g., keras) to handle a very specific exception type as if pydot was transparent, by checking themselves whether e.errno == os.errno.ENOENT.

@pnb
Copy link
Contributor Author

pnb commented Apr 27, 2017

I think that will work great! The approach in that stack overflow answer didn't seem to work across both python 2 and 3, as e.args was not re-interpreted when the exception was thrown for some reason. However it was easy to raise a new OSError with all the old arguments plus a new descriptive argument.

In python 3 apparently this is automatically turned into FileNotFoundError, still with the custom message, which seems like a pretty ideal outcome.

@johnyf johnyf self-assigned this Apr 27, 2017
@johnyf
Copy link
Contributor

johnyf commented Oct 24, 2017

Merged as edb0720.

Thanks for adjusting this change. Regarding the observation that in Python 3 instantiating OSError returns the subclass FileNotFoundError:

The constructor often actually returns a subclass of OSError, as described in OS exceptions below. The particular subclass depends on the final errno value. This behaviour only occurs when constructing OSError directly or via an alias, and is not inherited when subclassing.

In particular, FileNotFoundError:

Corresponds to errno ENOENT.

Also, appending the message at the end of OSError.args didn't match the signature of its constructor. Instead, modifies only strerror and filename. Tested with both Python 2 and 3.

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