Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#16242 closed defect (fixed)

assert_have_dot2tex should reraise exception not replace it

Reported by: slabbe Owned by: slabbe
Priority: major Milestone: sage-6.3
Component: packages: optional Keywords:
Cc: Merged in:
Authors: Sébastien Labbé Reviewers: Nathann Cohen
Report Upstream: N/A Work issues:
Branch: ecc5f70 (Commits) Commit:
Dependencies: Stopgaps:

Description (last modified by slabbe)

I installed sage -i dot2tex and then I get an error saying that dot2tex not available:

sage: G = DiGraph()
sage: G.add_edge(3333, 88, 'my_label')
sage: G.set_latex_options(format='dot2tex')
sage: view(G)
Traceback (most recent call last):
...
RuntimeError: dot2tex not available.

The reason is that the function assert_have_dot2tex replaces the ImportError (in my case a pyparsing importerror) by a RuntimeError with the dot2tex not available message. This hides the real error.

BEFORE fix:

sage: from sage.graphs.dot2tex_utils import assert_have_dot2tex
sage: assert_have_dot2tex()
---------------------------------------------------------------------------
RuntimeError                              Traceback (most recent call last)
<ipython-input-2-9f40acb4631c> in <module>()
----> 1 assert_have_dot2tex()

/Users/slabbe/Applications/sage-review/local/lib/python2.7/site-packages/sage/graphs/dot2tex_utils.pyc in assert_have_dot2tex()
     60             raise RuntimeError(check_error_string)
     61     except ImportError:
---> 62         raise RuntimeError(missing_error_string)
     63 
     64 def quoted_latex(x):

RuntimeError: 
dot2tex not available.

Please see :meth:`sage.graphs.generic_graph.GenericGraph.layout_graphviz`
for installation instructions.

AFTER fix:

sage: from sage.graphs.dot2tex_utils import assert_have_dot2tex
sage: assert_have_dot2tex()

An error occured when importing dot2tex.

Please see :meth:`sage.graphs.generic_graph.GenericGraph.layout_graphviz`
for installation instructions.

---------------------------------------------------------------------------
ImportError                               Traceback (most recent call last)
<ipython-input-2-9f40acb4631c> in <module>()
----> 1 assert_have_dot2tex()

/Users/slabbe/Applications/sage-git/local/lib/python2.7/site-packages/sage/graphs/dot2tex_utils.pyc in assert_have_dot2tex()
     56 """
     57     try:
---> 58         import dot2tex
     59     except ImportError as e:
     60         print import_error_string

/Users/slabbe/Applications/sage-git/local/lib/python2.7/site-packages/dot2tex/__init__.py in <module>()
     34 
     35 
---> 36 import dot2tex as d2t
     37 
     38 def get_logstream():

/Users/slabbe/Applications/sage-git/local/lib/python2.7/site-packages/dot2tex/dot2tex.py in <module>()
     45 import warnings
     46 
---> 47 import dotparsing
     48 
     49 # Silence DeprecationWarnings about os.popen3 in Python 2.6

/Users/slabbe/Applications/sage-git/local/lib/python2.7/site-packages/dot2tex/dotparsing.py in <module>()
     24     import matplotlib.pyparsing_py2 as pyparsing
     25 except ImportError:
---> 26     import matplotlib.pyparsing as pyparsing
     27     # just raise if that failed, too
     28 

ImportError: No module named pyparsing

Change History (21)

comment:1 Changed 5 years ago by slabbe

  • Branch set to u/slabbe/16242
  • Commit set to ecc5f70423cf25c566fd8f30364a3ffe295d6ad8
  • Status changed from new to needs_review

New commits:

ecc5f70assert_have_dot2tex should reraise exception

comment:2 Changed 5 years ago by leif

  • Authors set to Sébastien Labbé

comment:3 Changed 5 years ago by slabbe

  • Description modified (diff)

comment:4 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:5 follow-up: Changed 5 years ago by ncohen

I don't get what this patch does. Why do you remove the whole file ? If you only want to intercept a specific import error, why don't you do something like that instead ?

sage: def raise_an_error(msg):
....:     raise ImportError(msg)
sage: try:
....:     raise_an_error("Hello !")
....: except ImportError as E:
....:     if E.message == "Hello !":
....:         print "got it !"
....:         
got it !
sage: try:
....:     raise_an_error("Hey !")
....: except ImportError as E:
....:     if E.message == "Hello !":
....:         print "got it !"

Nathann

comment:6 Changed 5 years ago by ncohen

  • Status changed from needs_review to needs_info

comment:7 in reply to: ↑ 5 Changed 5 years ago by slabbe

  • Status changed from needs_info to needs_review

Replying to ncohen:

I don't get what this patch does.

The patch fixes a problem. The problem is that an exception is catched and replaced by a message error which is misleading. The solution is simply to reraise the error if an error occurs. If no error occurs, then more tests are done as before.

Why do you remove the whole file ?

The patch does not remove any file. What do you mean?

If you only want to intercept a specific import error, why don't you do something like that instead ?

Because an "ImportError" is already enough. I do not need to be more specific.

Sébastien

comment:8 Changed 5 years ago by ncohen

Weird. I am pretty sure that when I last clicked to see the branch's code, all it did was to empty a file. O_o

I am writing some code for Jeroen at the moment, I will review yours when it is done. One hour from now at most I hope.

Nathann

comment:9 follow-up: Changed 5 years ago by ncohen

I don't get it again : why do you both print a message THEN raise the exception ? If you need to change the message why don't you just raise your exception with a different message ?

Nathann

comment:10 in reply to: ↑ 9 ; follow-up: Changed 5 years ago by slabbe

If you need to change the message why don't you just raise your exception with a different message ?

I want to keep the message. This is the purpose of this ticket. Changing the error message just hide the real problem.

I don't get it again : why do you both print a message THEN raise the exception ?

Because it was like that before and because that printed message is unrelated to the purpose of the ticket. I kept the message because it just gives more information.

BEFORE:

  • print error info about dot2tex (unrelated to the goal of the ticket)
  • then raise RunTimeError with error info about dot2tex (related to the actual ticket)

NOW:

  • print error info about dot2tex
  • then reraise the error

Ok maybe print statement is not clean I agree. Do you want me to fix that part in this ticket? But how should it be then? How to add cleanly info before raising an exception?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 5 years ago by ncohen

I want to keep the message. This is the purpose of this ticket. Changing the error message just hide the real problem.

What I understand is that formerly ths try/except contained too much, while with your patch it only contains the import statement.

Sooooooooo there is no risk that catching this ImportError and raising another importerror with a more useful information (i.e. how to install the spkg) would hide anything, is there ?

Because it was like that before and because that printed message is unrelated to the purpose of the ticket. I kept the message because it just gives more information.

The message is useful indeed, but you print it before raising an exception.

BEFORE:

  • print error info about dot2tex (unrelated to the goal of the ticket)
  • then raise RunTimeError with error info about dot2tex (related to the actual ticket)

NOW:

  • print error info about dot2tex
  • then reraise the error

Ok maybe print statement is not clean I agree. Do you want me to fix that part in this ticket? But how should it be then? How to add cleanly info before raising an exception?

I still do not understand. What could possibly go wrong with that :

try:
    import dot2tex
except ImportError:
    raise ImportError(the_more_meaningful_message_that_you_currently_print)

Nathann

comment:12 in reply to: ↑ 11 Changed 5 years ago by slabbe

Sooooooooo there is no risk that catching this ImportError? and raising another importerror with a more useful information (i.e. how to install the spkg) would hide anything, is there ?

Yes there is.

I still do not understand. What could possibly go wrong with that :

try:
    import dot2tex
except ImportError:
    raise ImportError(the_more_meaningful_message_that_you_currently_print)

Suppose import dot2tex raises an ImportError which is caused by the importation of another module like pyparsing (see the description of the ticket). Then the more meaningful message that you can think of will be confusing because it will not give you the error traceback for the pyparsing import error.

This is what happen to me for some version of Sage with some version of dot2tex spkg two months ago and motivated me to create this ticket. The import error of pyparsing was not appearing on the screen and I had to fix the code and reraise the real error to see what was the problem.

comment:13 follow-up: Changed 5 years ago by ncohen

HMmmmmmm.. I see.

Then indeed I don't know how to do it any better than what you have in your branch. Too bad :-/

Nathann

comment:14 Changed 5 years ago by ncohen

  • Reviewers set to Nathann Cohen
  • Status changed from needs_review to positive_review

comment:15 in reply to: ↑ 13 Changed 5 years ago by slabbe

Thanks Nathann.

comment:16 follow-up: Changed 5 years ago by jhpalmieri

Instead of

except ImportError as e:
    print import_error_string
    raise # re-raise current exception

(by the way, I don't think you need the "as e" part), you could do

except ImportError:
    raise ImportError(import_error_string)

Would that be better?

comment:17 in reply to: ↑ 16 Changed 5 years ago by slabbe

except ImportError:
    raise ImportError(import_error_string)

Would that be better?

No. It is basically what it was before. It loses what creates the error.

(by the way, I don't think you need the "as e" part), you could do

You are right.

I forgot to remove it after I tried to add info in e unsuccessfully:

except ImportError as e:
    e.add_info_method(import_error_string) #  <--- how to do this?
    raise e # re-raise current exception

comment:18 follow-up: Changed 5 years ago by jhpalmieri

How about something like:

except ImportError as e:
    raise ImportError(e.message + import_error_string)
Version 0, edited 5 years ago by jhpalmieri (next)

comment:19 Changed 5 years ago by vbraun

  • Branch changed from u/slabbe/16242 to ecc5f70423cf25c566fd8f30364a3ffe295d6ad8
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:20 in reply to: ↑ 18 Changed 5 years ago by slabbe

  • Commit ecc5f70423cf25c566fd8f30364a3ffe295d6ad8 deleted

Replying to jhpalmieri:

How about something like:

except ImportError as e:
    raise ImportError(e.message + import_error_string)

With this we lose the traceback of the original error... Same thing for the following :

except ImportError as e:
    e.args = (e.message + import_error_string,)
    raise e

comment:21 Changed 5 years ago by vbraun

Swallowing the traceback is good if you can give a better diagnosis. But I'd avoid randomly changing the error message as it'll be actually harmful if you do a web search for it.

In any case, this ticket is closed.

Note: See TracTickets for help on using tickets.