search

ioam / topographica

A general-purpose neural simulator focusing on topographic maps.
topographica.org
BSD 3-Clause "New" or "Revised" License
53 stars 32 forks source link

Make IPython extension optional. #598

Closed ceball closed 9 years ago

ceball commented 9 years ago

Part of #594.

I wondered about catching all errors rather than just SkipTest - what do you think? (Also, feels odd to be importing unittest here and elsewhere, but I guess lots of nose importing has been happening so it's not too bad...)

Not sure about the message, too. (Could maybe print out the caught exception instead?)

jlstevens commented 9 years ago

I think you can catch all exceptions, not just SkipTest. Otherwise, the message looks fine to me and I am ok with this change.

philippjfr commented 9 years ago

Hmm, if the IPython extension is failing for some reason other than a failure to import IPython we want to know about it. Catch 'em all exceptions are really bad practice in general.

jlstevens commented 9 years ago

I agree that catching all exceptions can be a bad thing. Here, I think it makes sense as the warning message is very general and the IPython extension is a completely optional component.

Do we really want to stop Topographica working just because some things won't display nicely in the IPython Notebook?

jbednar commented 9 years ago

Catching all exceptions is indeed a bad thing, but in this case I think it's appropriate, because we're not hiding it. I'd rather have a warning message in this case than to find that Topographica won't load, and as long as we do have a warning, I think it's safe to catch all exceptions here. It would be nice if the warning showed what the exception was, though, e.g. printing one message for the expected exception (as it has now) and a different one for unexpected ones listing the actual exception.

jlstevens commented 9 years ago

Care to update this PR soon so we can merge? Thanks!

ceball commented 9 years ago

Sorry, "soon" means something different to you than to me ;) I am happy to catch all exceptions and do what Jim suggests with the warning message...soon.

ceball commented 9 years ago

If importing the ipython extension generates a SkipTest error, the user will see something like:

WARNING: Could not load ipython extension 'topo.misc.ipython': IPython extension requires IPython >= 0.12

If the ipython extension generates a different error, the user will instead just see the text of that error (i.e. for the extension doing raise ValueError('something bad') they'd see WARNING: Could not load ipython extension 'topo.misc.ipython': something bad).

Maybe it would be better to print more than the text of the error message, but I don't know how to do that any more (it was enough for me to figure out that exception as is right for the versions of python we support...). E.g. I thought it would be nice to report the error type, but that seemed to require doing something like e.__class__.__name__. I recall using another module in the past (traceback?) to display exception information, but I'm not sure what's normal now. Anyone familiar with it want to tell me what to do?

jbednar commented 9 years ago

The current behavior sounds ok, but showing the traceback would indeed be more helpful. I don't specifically remember any reason not to use traceback, but I do have some very vague notion that we might have traced back some problem to a call to traceback. Sorry I can't be more helpful than that!

jbednar commented 9 years ago

Looks like we're still using traceback.print_exc() in multiple places, so it must be ok: https://github.com/ioam/topographica/search?q=traceback