From 08d6ac78bf6a746b78fe93bc30f172b52d84f831 2010-01-08 09:39:21 From: Fernando Perez Date: 2010-01-08 09:39:21 Subject: [PATCH] Move crash handling to the application level and simplify class structure. Starting to try to take real advantage of the refactoring, to have generic crash handling. This also lets us initialize the app without needing all the self.attempt() wrappers, since now there's a good system-wide crash handler at the app level (not inside the shell instance). I didn't yet remove the attempt() method because we may have occasional uses for it (we still do, but in one place only). I also removed some extra class layers that weren't quite needed. Creating classes solely for the purpose of passing parameters makes the code (IMO) harder to understand, I kept getting lost in parts of the class hierarchy. I think these changes provide the same flexibility but with easier to follow code (less things to remember, basically). What I tried to do was to use argument passing instead of inheritance for all cases I saw where the inheritance wasn't really adding new functionality. In some cases, this actually allowed me to remove methods that were effectively duplicated in the subclasses. --- diff --git a/IPython/config/loader.py b/IPython/config/loader.py index 5b01462..328cd76 100644 --- a/IPython/config/loader.py +++ b/IPython/config/loader.py @@ -298,14 +298,12 @@ NoConfigDefault = NoConfigDefault() class ArgParseConfigLoader(CommandLineConfigLoader): - # arguments = [(('-f','--file'),dict(type=str,dest='file'))] - arguments = () - - def __init__(self, argv=None, *args, **kw): + def __init__(self, argv=None, arguments=(), *args, **kw): """Create a config loader for use with argparse. - With the exception of argv, other args and kwargs arguments here are - passed onto the constructor of :class:`argparse.ArgumentParser`. + With the exception of ``argv`` and ``arguments``, other args and kwargs + arguments here are passed onto the constructor of + :class:`argparse.ArgumentParser`. Parameters ---------- @@ -313,11 +311,16 @@ class ArgParseConfigLoader(CommandLineConfigLoader): argv : optional, list If given, used to read command-line arguments from, otherwise sys.argv[1:] is used. + + arguments : optional, tuple + Description of valid command-line arguments, to be called in sequence + with parser.add_argument() to configure the parser. """ super(CommandLineConfigLoader, self).__init__() if argv == None: argv = sys.argv[1:] self.argv = argv + self.arguments = arguments self.args = args self.kw = kw diff --git a/IPython/config/tests/test_loader.py b/IPython/config/tests/test_loader.py index ea06dab..487db8b 100755 --- a/IPython/config/tests/test_loader.py +++ b/IPython/config/tests/test_loader.py @@ -66,15 +66,13 @@ class TestArgParseCL(TestCase): def test_basic(self): - class MyLoader(ArgParseConfigLoader): - arguments = ( + arguments = ( (('-f','--foo'), dict(dest='Global.foo', type=str)), (('-b',), dict(dest='MyClass.bar', type=int)), (('-n',), dict(dest='n', action='store_true')), (('Global.bam',), dict(type=str)) ) - - cl = MyLoader() + cl = ArgParseConfigLoader(arguments=arguments) config = cl.load_config('-f hi -b 10 -n wow'.split()) self.assertEquals(config.Global.foo, 'hi') self.assertEquals(config.MyClass.bar, 10) diff --git a/IPython/core/application.py b/IPython/core/application.py index c3dddb7..ad6472b 100755 --- a/IPython/core/application.py +++ b/IPython/core/application.py @@ -33,7 +33,7 @@ import logging import os import sys -from IPython.core import release +from IPython.core import release, crashhandler from IPython.utils.genutils import get_ipython_dir, get_ipython_package_dir from IPython.config.loader import ( PyFileConfigLoader, @@ -77,6 +77,29 @@ class ApplicationError(Exception): pass +app_cl_args = ( + (('--ipython-dir', ), dict( + dest='Global.ipython_dir',type=unicode, + help='Set to override default location of Global.ipython_dir.', + default=NoConfigDefault, + metavar='Global.ipython_dir') ), + (('-p', '--profile',), dict( + dest='Global.profile',type=unicode, + help='The string name of the ipython profile to be used.', + default=NoConfigDefault, + metavar='Global.profile') ), + (('--log-level',), dict( + dest="Global.log_level",type=int, + help='Set the log level (0,10,20,30,40,50). Default is 30.', + default=NoConfigDefault, + metavar='Global.log_level')), + (('--config-file',), dict( + dest='Global.config_file',type=unicode, + help='Set the config file name to override default.', + default=NoConfigDefault, + metavar='Global.config_file')), + ) + class Application(object): """Load a config, construct components and set them running.""" @@ -94,11 +117,17 @@ class Application(object): ipython_dir = None #: A reference to the argv to be used (typically ends up being sys.argv[1:]) argv = None + #: Default command line arguments. Subclasses should create a new tuple + #: that *includes* these. + cl_arguments = app_cl_args # Private attributes _exiting = False _initialized = False + # Class choices for things that will be instantiated at runtime. + _CrashHandler = crashhandler.CrashHandler + def __init__(self, argv=None): self.argv = sys.argv[1:] if argv is None else argv self.init_logger() @@ -125,39 +154,49 @@ class Application(object): if self._initialized: return - - self.attempt(self.create_default_config) + + # The first part is protected with an 'attempt' wrapper, that will log + # failures with the basic system traceback machinery. Once our crash + # handler is in place, we can let any subsequent exception propagate, + # as our handler will log it with much better detail than the default. + self.attempt(self.create_crash_handler) + self.create_default_config() self.log_default_config() self.set_default_config_log_level() - self.attempt(self.pre_load_command_line_config) - self.attempt(self.load_command_line_config, action='abort') + self.pre_load_command_line_config() + self.load_command_line_config() self.set_command_line_config_log_level() - self.attempt(self.post_load_command_line_config) + self.post_load_command_line_config() self.log_command_line_config() - self.attempt(self.find_ipython_dir) - self.attempt(self.find_resources) - self.attempt(self.find_config_file_name) - self.attempt(self.find_config_file_paths) - self.attempt(self.pre_load_file_config) - self.attempt(self.load_file_config) + self.find_ipython_dir() + self.find_resources() + self.find_config_file_name() + self.find_config_file_paths() + self.pre_load_file_config() + self.load_file_config() self.set_file_config_log_level() - self.attempt(self.post_load_file_config) + self.post_load_file_config() self.log_file_config() - self.attempt(self.merge_configs) + self.merge_configs() self.log_master_config() - self.attempt(self.pre_construct) - self.attempt(self.construct) - self.attempt(self.post_construct) + self.pre_construct() + self.construct() + self.post_construct() self._initialized = True def start(self): self.initialize() - self.attempt(self.start_app) + self.start_app() #------------------------------------------------------------------------- # Various stages of Application creation #------------------------------------------------------------------------- + def create_crash_handler(self): + """Create a crash handler, typically setting sys.excepthook to it.""" + self.crash_handler = self._CrashHandler(self, self.name) + sys.excepthook = self.crash_handler + def create_default_config(self): """Create defaults that can't be set elsewhere. @@ -185,10 +224,9 @@ class Application(object): def create_command_line_config(self): """Create and return a command line config loader.""" - return BaseAppArgParseConfigLoader(self.argv, - description=self.description, - version=release.version - ) + return ArgParseConfigLoader(self.argv, self.cl_arguments, + description=self.description, + version=release.version) def pre_load_command_line_config(self): """Do actions just before loading the command line config.""" @@ -384,7 +422,10 @@ class Application(object): raise except: if action == 'abort': + self.log.critical("Aborting application: %s" % self.name, + exc_info=True) self.abort() + raise elif action == 'exit': self.exit(0) diff --git a/IPython/core/crashhandler.py b/IPython/core/crashhandler.py index ad8d340..d0de855 100644 --- a/IPython/core/crashhandler.py +++ b/IPython/core/crashhandler.py @@ -28,10 +28,8 @@ from IPython.core import release from IPython.core import ultratb from IPython.external.Itpl import itpl -from IPython.utils.genutils import * - #**************************************************************************** -class CrashHandler: +class CrashHandler(object): """Customizable crash handlers for IPython-based systems. Instances of this class provide a __call__ method which can be used as a @@ -41,15 +39,15 @@ class CrashHandler: """ - def __init__(self,IP,app_name,contact_name,contact_email, - bug_tracker,crash_report_fname, + def __init__(self,app, app_name, contact_name=None, contact_email=None, + bug_tracker=None, crash_report_fname='CrashReport.txt', show_crash_traceback=True): """New crash handler. Inputs: - - IP: a running IPython instance, which will be queried at crash time - for internal information. + - app: a running application instance, which will be queried at crash + time for internal information. - app_name: a string containing the name of your application. @@ -77,13 +75,14 @@ class CrashHandler: """ # apply args into instance - self.IP = IP # IPython instance + self.app = app self.app_name = app_name self.contact_name = contact_name self.contact_email = contact_email self.bug_tracker = bug_tracker self.crash_report_fname = crash_report_fname self.show_crash_traceback = show_crash_traceback + self.section_sep = '\n\n'+'*'*75+'\n\n' # Hardcoded defaults, which can be overridden either by subclasses or # at runtime for the instance. @@ -124,7 +123,7 @@ $self.bug_tracker #color_scheme = 'Linux' # dbg try: - rptdir = self.IP.ipython_dir + rptdir = self.app.ipython_dir except: rptdir = os.getcwd() if not os.path.isdir(rptdir): @@ -134,7 +133,7 @@ $self.bug_tracker # properly expanded out in the user message template self.crash_report_fname = report_name TBhandler = ultratb.VerboseTB(color_scheme=color_scheme, - long_header=1) + long_header=1) traceback = TBhandler.text(etype,evalue,etb,context=31) # print traceback to screen @@ -159,70 +158,62 @@ $self.bug_tracker def make_report(self,traceback): """Return a string containing a crash report.""" - - sec_sep = '\n\n'+'*'*75+'\n\n' - + import platform + + sec_sep = self.section_sep + report = [] rpt_add = report.append rpt_add('*'*75+'\n\n'+'IPython post-mortem report\n\n') - rpt_add('IPython version: %s \n\n' % release.version) - rpt_add('BZR revision : %s \n\n' % release.revision) - rpt_add('Platform info : os.name -> %s, sys.platform -> %s' % + rpt_add('IPython version: %s \n' % release.version) + rpt_add('BZR revision : %s \n' % release.revision) + rpt_add('Platform info : os.name -> %s, sys.platform -> %s\n' % (os.name,sys.platform) ) - rpt_add(sec_sep+'Current user configuration structure:\n\n') - rpt_add(pformat(self.IP.dict())) - rpt_add(sec_sep+'Crash traceback:\n\n' + traceback) + rpt_add(' : %s\n' % platform.platform()) + rpt_add('Python info : %s\n' % sys.version) + try: - rpt_add(sec_sep+"History of session input:") - for line in self.IP.user_ns['_ih']: - rpt_add(line) - rpt_add('\n*** Last line of input (may not be in above history):\n') - rpt_add(self.IP._last_input_line+'\n') + config = pformat(self.app.config) + rpt_add(sec_sep+'Current user configuration structure:\n\n') + rpt_add(config) except: pass + rpt_add(sec_sep+'Crash traceback:\n\n' + traceback) return ''.join(report) + class IPythonCrashHandler(CrashHandler): """sys.excepthook for IPython itself, leaves a detailed report on disk.""" - def __init__(self,IP): + def __init__(self, app, app_name='IPython'): # Set here which of the IPython authors should be listed as contact AUTHOR_CONTACT = 'Fernando' # Set argument defaults - app_name = 'IPython' bug_tracker = 'https://bugs.launchpad.net/ipython/+filebug' contact_name,contact_email = release.authors[AUTHOR_CONTACT][:2] crash_report_fname = 'IPython_crash_report.txt' # Call parent constructor - CrashHandler.__init__(self,IP,app_name,contact_name,contact_email, + CrashHandler.__init__(self,app,app_name,contact_name,contact_email, bug_tracker,crash_report_fname) def make_report(self,traceback): """Return a string containing a crash report.""" - sec_sep = '\n\n'+'*'*75+'\n\n' - - report = [] + sec_sep = self.section_sep + # Start with parent report + report = [super(IPythonCrashHandler, self).make_report(traceback)] + # Add interactive-specific info we may have rpt_add = report.append - - rpt_add('*'*75+'\n\n'+'IPython post-mortem report\n\n') - rpt_add('IPython version: %s \n\n' % release.version) - rpt_add('BZR revision : %s \n\n' % release.revision) - rpt_add('Platform info : os.name -> %s, sys.platform -> %s' % - (os.name,sys.platform) ) - rpt_add(sec_sep+'Current user configuration structure:\n\n') - # rpt_add(pformat(self.IP.dict())) - rpt_add(sec_sep+'Crash traceback:\n\n' + traceback) try: rpt_add(sec_sep+"History of session input:") - for line in self.IP.user_ns['_ih']: + for line in self.app.shell.user_ns['_ih']: rpt_add(line) rpt_add('\n*** Last line of input (may not be in above history):\n') - rpt_add(self.IP._last_input_line+'\n') + rpt_add(self.app.shell._last_input_line+'\n') except: pass diff --git a/IPython/core/ipapp.py b/IPython/core/ipapp.py index af76e6e..8384838 100755 --- a/IPython/core/ipapp.py +++ b/IPython/core/ipapp.py @@ -28,6 +28,7 @@ import logging import os import sys +from IPython.core import crashhandler from IPython.core import release from IPython.core.application import Application, BaseAppArgParseConfigLoader from IPython.core.error import UsageError @@ -280,19 +281,18 @@ cl_args = ( ) -class IPythonAppCLConfigLoader(BaseAppArgParseConfigLoader): - - arguments = cl_args - - default_config_file_name = u'ipython_config.py' - class IPythonApp(Application): name = u'ipython' description = 'IPython: an enhanced interactive Python shell.' config_file_name = default_config_file_name + cl_arguments = Application.cl_arguments + cl_args + + # Private and configuration attributes + _CrashHandler = crashhandler.IPythonCrashHandler + def __init__(self, argv=None, **shell_params): """Create a new IPythonApp. @@ -309,6 +309,7 @@ class IPythonApp(Application): super(IPythonApp, self).__init__(argv) self.shell_params = shell_params + def create_default_config(self): super(IPythonApp, self).create_default_config() # Eliminate multiple lookups @@ -340,13 +341,6 @@ class IPythonApp(Application): Global.wthread = False Global.gthread = False - def create_command_line_config(self): - """Create and return a command line config loader.""" - return IPythonAppCLConfigLoader(self.argv, - description=self.description, - version=release.version - ) - def load_file_config(self): if hasattr(self.command_line_config.Global, 'quick'): if self.command_line_config.Global.quick: diff --git a/IPython/core/iplib.py b/IPython/core/iplib.py index eea73bf..5965b19 100644 --- a/IPython/core/iplib.py +++ b/IPython/core/iplib.py @@ -1167,37 +1167,14 @@ class InteractiveShell(Component, Magic): color_scheme='NoColor', tb_offset = 1) - # IPython itself shouldn't crash. This will produce a detailed - # post-mortem if it does. But we only install the crash handler for - # non-threaded shells, the threaded ones use a normal verbose reporter - # and lose the crash handler. This is because exceptions in the main - # thread (such as in GUI code) propagate directly to sys.excepthook, - # and there's no point in printing crash dumps for every user exception. - if self.isthreaded: - ipCrashHandler = ultratb.FormattedTB() - else: - from IPython.core import crashhandler - ipCrashHandler = crashhandler.IPythonCrashHandler(self) - self.set_crash_handler(ipCrashHandler) + # The instance will store a pointer to the system-wide exception hook, + # so that runtime code (such as magics) can access it. This is because + # during the read-eval loop, it may get temporarily overwritten. + self.sys_excepthook = sys.excepthook # and add any custom exception handlers the user may have specified self.set_custom_exc(*custom_exceptions) - def set_crash_handler(self, crashHandler): - """Set the IPython crash handler. - - This must be a callable with a signature suitable for use as - sys.excepthook.""" - - # Install the given crash handler as the Python exception hook - sys.excepthook = crashHandler - - # The instance will store a pointer to this, so that runtime code - # (such as magics) can access it. This is because during the - # read-eval loop, it gets temporarily overwritten (to deal with GUI - # frameworks). - self.sys_excepthook = sys.excepthook - def set_custom_exc(self,exc_tuple,handler): """set_custom_exc(exc_tuple,handler) @@ -1880,7 +1857,7 @@ class InteractiveShell(Component, Magic): if (self.SyntaxTB.last_syntax_error and self.autoedit_syntax): self.edit_syntax_error() - + # We are off again... __builtin__.__dict__['__IPYTHON__active'] -= 1 diff --git a/IPython/frontend/prefilterfrontend.py b/IPython/frontend/prefilterfrontend.py index 239435c..6cb2997 100644 --- a/IPython/frontend/prefilterfrontend.py +++ b/IPython/frontend/prefilterfrontend.py @@ -77,12 +77,6 @@ class PrefilterFrontEnd(LineFrontEndBase): """ if argv is None: argv = ['--no-banner'] - # This is a hack to avoid the IPython exception hook to trigger - # on exceptions (https://bugs.launchpad.net/bugs/337105) - # XXX: This is horrible: module-leve monkey patching -> side - # effects. - from IPython.core import iplib - iplib.InteractiveShell.isthreaded = True LineFrontEndBase.__init__(self, *args, **kwargs) self.shell.output_trap = RedirectorOutputTrap( diff --git a/IPython/frontend/tests/test_prefilterfrontend.py b/IPython/frontend/tests/test_prefilterfrontend.py index ebe100c..5e7504b 100644 --- a/IPython/frontend/tests/test_prefilterfrontend.py +++ b/IPython/frontend/tests/test_prefilterfrontend.py @@ -88,9 +88,6 @@ def isolate_ipython0(func): del user_ns[k] for k in new_globals: del user_global_ns[k] - # Undo the hack at creation of PrefilterFrontEnd - from IPython.core import iplib - iplib.InteractiveShell.isthreaded = False return out my_func.__name__ = func.__name__