Added support for new pydot versions to fix find_graphviz error #6398
Conversation
keras/utils/vis_utils.py
Outdated
| try: | ||
| if hasattr(pydot, 'find_graphviz'): | ||
| if not pydot.find_graphviz(): | ||
| raise ImportError() |
There was a problem hiding this comment.
This needs an error message explaining what went wrong and how to fix it (like below).
There was a problem hiding this comment.
After experimenting more it seems like this isn't even needed anymore, so I removed it.
keras/utils/vis_utils.py
Outdated
| if not pydot.find_graphviz(): | ||
| raise ImportError() | ||
| else: | ||
| pydot.Dot.create(pydot.Dot()) |
There was a problem hiding this comment.
What is this statement for? A comment would be helpful.
keras/utils/vis_utils.py
Outdated
| raise ImportError() | ||
| else: | ||
| pydot.Dot.create(pydot.Dot()) | ||
| except: |
There was a problem hiding this comment.
No catch-all exceptions. You should catch a specific class.
There was a problem hiding this comment.
The problem with this is that pydot raises a generic exception if it can't find dot, so there is no specific class. One alternative might be to not checking the dot installation at all, but that produces a less helpful error message that doesn't mention graphviz specifically.
keras/utils/vis_utils.py
Outdated
| try: | ||
| # Attempt to create an image of a blank graph to check the pydot/graphviz installation. | ||
| pydot.Dot.create(pydot.Dot()) | ||
| except: |
There was a problem hiding this comment.
Again: you should only except a specific Exception class.
There was a problem hiding this comment.
There is not a specific Exception class in this situation. pydot raises a generic Exception (pydot source).
There was a problem hiding this comment.
Oh wow... In this case let's catch the exception, add a comment on why we use Exception, and check that the exception message is what we expect. raise otherwise.
There was a problem hiding this comment.
Actually, never mind what I said, it will be safer to just catch Exception. Do add a comment on why we do this.
There was a problem hiding this comment.
For what it's worth I will probably also try submit a pull request to pyplot to raise a more appropriate exception.
|
Please see #5313 (comment). |
I was having difficulties with the
keras.utils.plot_modelfunction, specifically that pydot no longer supports thefind_graphvizfunction, and installingpydot-nginstead does not seem to fix it. This pull request should solve issue #3210.