Don't enable pylab mode, when matplotlib is not importable #2681
Conversation
| except ImportError: | ||
| print("pylab mode doesn't work as matplotlib could not be found." + \ | ||
| "\nIs it installed on the system?", file=sys.stderr) | ||
| return | ||
| shell = self.shell |
There was a problem hiding this comment.
Below, another error is catch and log a message to the console as well as showing the traceback.
Maybe we could conform to that and do the same, it would allow for better debugging guide the user when they have an error like that.
What do you think ?
Same for other file.
There was a problem hiding this comment.
Yes, makes sense. Then the user directly sees that matplotlib could not be imported in the file IPython/core/pylabtools.py.
Changed locally. I have just some problems in the other file with logging...
|
Looks fine to me, Just a small comment that have to be discussed. |
| @@ -20,7 +20,8 @@ | |||
| # Imports | |||
| #----------------------------------------------------------------------------- | |||
|
|
|||
| from __future__ import absolute_import | |||
| from __future__ import absolute_import, print_function | |||
|
|
|||
There was a problem hiding this comment.
we shouldn't use the print function here, there's a logger attached to instances of InteractiveShellApp, please use that
There was a problem hiding this comment.
Does log.warn write to the terminal that wrote the process and print to the actual frontend ?
There was a problem hiding this comment.
log.warn only writes to the terminal where the process was launched from, but has the advantaged that it can be easily redirected to a file, or also selectively disabled based on the loglevel that is set (which can also be set from the command line)
There was a problem hiding this comment.
Unfortunately 'TerminalInteractiveShell' object has no attribute 'log', so I'm unsure, how to get access to a logger in this file.
Changed in the other file through.
How is it possible to log from a TerminalInteractiveShell object?
|
Pinging the status of this one... |
|
The remaining problem is, that I don't know how to log properly from a 'TerminalInteractiveShell' object. See the comments above for the file: IPython/core/shellapp.py |
|
TerminalInteractiveShell inherit from Configurable, and might maybe inherit from logging configurable. |
|
TerminalInteractiveShell inherits from InteractiveShell, which inherits from SingletonConfigurable. I have still no clue, how to properly log/warn the user, except directly printing to the console... |
|
Typically in the Shell we use |
| #shell.logger.log_write("pylab mode doesn't work as matplotlib could not be found." + \ | ||
| # "\nIs it installed on the system?") | ||
| shell.showtraceback() | ||
| return |
There was a problem hiding this comment.
Are all callers of this function able to handle a None return value?
There was a problem hiding this comment.
Thanks for the comment!
When verifying this, I realized that enable_pylab is the only function that calls this function (modulo some tests).
I just undid this change again and catched the ImportError in the enable_pylab function instead.
On a headless server, matplotlib is a unnecessary dependency and the ipython console will crash badly, when tring to enable %pylab there. In that case, just don't enable %pylab and ask the user to install matplotlib. Original bug report: https://bugzilla.redhat.com/show_bug.cgi?id=872176
|
As This looks now much nicer than the solution before. |
| except ImportError: | ||
| self.log.warn("pylab mode doesn't work as matplotlib could not be found." + \ | ||
| "\nIs it installed on the system?") | ||
| self.shell.showtraceback() |
There was a problem hiding this comment.
Do we really want to call showtraceback here but not in interactiveshell?
There was a problem hiding this comment.
The log could be suppressed so the traceback makes sense here (See the pull request and the explanation by @minrk).
Furthermore there is no IPython.utils.warn.showtraceback() to be called in the InteractiveShell and not showing a traceback there is conform with the rest of the code there.
|
This is much cleaner, I left one inline comment, but this is almost ready to go. |
|
OK, thanks for the explanation. I am going to merge. |
Don't enable pylab mode, when matplotlib is not importable
Don't enable pylab mode, when matplotlib is not importable
On a headless server, matplotlib is a unnecessary dependency and the ipython
console will crash badly, when tring to enable %pylab there.
In that case, just don't enable %pylab and ask the user to install matplotlib.
Original bug report: https://bugzilla.redhat.com/show_bug.cgi?id=872176