Opened 10 years ago

Closed 10 years ago

Last modified 7 years ago

#11812 closed defect (fixed)

traceback with load and attach of .sage files

Reported by: mstreng Owned by: jason
Priority: major Milestone: sage-4.8
Component: misc Keywords: load attach traceback
Cc: johanbosman Merged in: sage-4.8.alpha0
Authors: Marco Streng Reviewers: Johan Bosman
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by mstreng)

When debugging code that is loaded into or attached to a Sage session from a .sage file, the tracebacks are not very informative: they refer to <string> instead of to the file name, and give no line numbers or code snippets. This makes it hard to find out where the error is.

The patch fixes this for attach, but not for load (because of efficiency reasons).

Apply 11812.3.patch


Example:

Suppose I have some files loaded with "load" or "attach", and one of them is a .sage file with

class Bar():
     def function_with_common_name(self):
         # lots of lines of code
         1/0
         # lots of lines of code

# other classes that also have functions named function_with_common_name

def foo():
     # lots of lines of code
     a = Bar()
     a.function_with_common_name()
     # lots of lines of code

Now I do

sage: foo()
---------------------------------------------------------------------------
ZeroDivisionError                         Traceback (most recent call last)

/home/marco/<ipython console> in <module>()

/home/marco/<string> in foo()

/home/marco/<string> in function_with_common_name(self)

/usr/local/sage/sage-4.6.2/local/lib/python2.6/site-packages/sage/structure/element.so
in sage.structure.element.RingElement.__div__
(sage/structure/element.c:11981)()

/usr/local/sage/sage-4.6.2/local/lib/python2.6/site-packages/sage/rings/integer.so
in sage.rings.integer.Integer._div_ (sage/rings/integer.c:11944)()

/usr/local/sage/sage-4.6.2/local/lib/python2.6/site-packages/sage/rings/integer_ring.so
in sage.rings.integer_ring.IntegerRing_class._div
(sage/rings/integer_ring.c:5142)()

ZeroDivisionError: Rational division by zero

Then to find the bug using this traceback, I would need to search my files to find all functions named "function_with_common_name" that are called from functions named "foo" for a line that could cause a division by zero...

More details: http://groups.google.com/group/sage-devel/browse_thread/thread/761f69944e21efd4

Attachments (4)

trac_11812-traceback_attach.patch (4.2 KB) - added by mstreng 10 years ago.
11812.patch (3.0 KB) - added by mstreng 10 years ago.
simpler patch: fixes the problem without adding complicated new doctests
11812.2.patch (6.0 KB) - added by mstreng 10 years ago.
11812.3.patch (5.3 KB) - added by mstreng 10 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 10 years ago by mstreng

  • Description modified (diff)

comment:2 Changed 10 years ago by mstreng

  • Description modified (diff)

Changed 10 years ago by mstreng

comment:3 Changed 10 years ago by mstreng

  • Description modified (diff)

comment:4 Changed 10 years ago by mstreng

  • Status changed from new to needs_review

All tests passed on sage 4.7

comment:5 Changed 10 years ago by mstreng

  • Status changed from needs_review to needs_info

Judging from the patchbot results, it doesn't seem to work after all.

What exactly does spawn('sage') do? It always started a Sage session of the same Sage version and the same Sage installation when I tried it out (it was not in the PATH, but it was in the working directory). Maybe it doesn't work on all machines or it depends on the working directory?

Any idea of how to do these tests so that they succeed with patch and fail without?

comment:6 Changed 10 years ago by mstreng

  • Description modified (diff)
  • Status changed from needs_info to needs_review

only apply 11812.patch

comment:7 follow-up: Changed 10 years ago by leif

A few minor issues:

  • if preparse_to_file == None: should be if preparse_to_file is None:.
  • There's some mark-up missing in the docstrings (also in parts you didn't add).

  • In docstrings, Python identifiers (constants, function and variable names etc.) and the like should be enclosed in double backticks (``name``, which means Courier / typewriter font); `name` in contrast yields typesetting in math mode.


I'm not sure what your problem with doctests involving tracebacks exactly was; we have a couple of such tests (not needing to call sage or use the PExpect interface); they just look slightly different to what you get in a real Sage session.

You can also use continuation lines in examples and doctests by the way, but again they're (currently still) a bit different such that you cannot directly paste them from a Sage session, i.e., you have to use the Python form instead of Sage's (cf. #10458).

Changed 10 years ago by mstreng

simpler patch: fixes the problem without adding complicated new doctests

comment:8 in reply to: ↑ 7 Changed 10 years ago by mstreng

Replying to leif:

  • if preparse_to_file == None: should be if preparse_to_file is None:.
  • There's some mark-up missing in the docstrings (also in parts you didn't add).

Thanks, all corrected (I think).

  • In docstrings, Python identifiers (constants, function and variable names etc.) and the like should be enclosed in double backticks (``name``, which means Courier / typewriter font); `name` in contrast yields typesetting in math mode.

Not sure what you mean: I don't see where in this docstring single backticks are or should be used. I did add some double backticks though.

I'm not sure what your problem with doctests involving tracebacks exactly was; we have a couple of such tests (not needing to call sage or use the PExpect interface); they just look slightly different to what you get in a real Sage session.

The problem with the doctest is that I couldn't create a test that really checks whether #11812 was fixed:

  • The test with pexpect in that attachment seemed to work on my computer, but doesn't really, according to patchbot. The problem is that I don't quite understand which sage installation is used in this test.

So I had to remove the test. As the example isn't tested anyway, I decided to keep things simple by removing that as well and adding a few explanatory comments to the code.

comment:9 Changed 10 years ago by mstreng

I would like to remind the patchbot to apply 11812.patch only.

comment:10 Changed 10 years ago by mstreng

  • Status changed from needs_review to needs_work

Oops, this patch works only from the moment you attach the file until the moment it changes: if I change my file in another terminal, and then do sage: foo() again, it gets reloaded without good tracebacks.

comment:11 Changed 10 years ago by mstreng

  • Cc johanbosman added

I'm writing a new patch.

Unrelated to that, see also #11925.

Changed 10 years ago by mstreng

comment:12 Changed 10 years ago by mstreng

  • Authors set to Marco Streng
  • Description modified (diff)

New patch that does work. Apply 11812.3.patch only. (testing now)

Changed 10 years ago by mstreng

comment:13 Changed 10 years ago by mstreng

  • Status changed from needs_work to needs_review

All tests pass!!

comment:14 follow-up: Changed 10 years ago by johanbosman

  • Reviewers set to Johan Bosman
  • Status changed from needs_review to positive_review

Looks good. ;).

comment:15 Changed 10 years ago by jdemeyer

  • Milestone changed from sage-4.7.2 to sage-4.7.3

comment:16 in reply to: ↑ 14 Changed 10 years ago by mstreng

  • Keywords load attach traceback added

Replying to johanbosman:

Thanks!

comment:17 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.3.alpha0
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:18 Changed 10 years ago by jdemeyer

  • Milestone sage-4.7.3 deleted

Milestone sage-4.7.3 deleted

comment:19 Changed 10 years ago by jdemeyer

  • Merged in changed from sage-4.7.3.alpha0 to sage-4.8.alpha0
  • Milestone set to sage-4.8

comment:20 in reply to: ↑ description ; follow-up: Changed 7 years ago by jdemeyer

Replying to mstreng:

The patch fixes this for attach, but not for load (because of efficiency reasons).

What exactly are these "efficiency reasons"? Are people really executing load or attach in tight loops? I think that one should always have good tracebacks, so I would always use temporary files.

comment:21 in reply to: ↑ 20 ; follow-up: Changed 7 years ago by mstreng

Replying to jdemeyer:

Replying to mstreng:

The patch fixes this for attach, but not for load (because of efficiency reasons).

What exactly are these "efficiency reasons"?

With a temporary file, there is a good traceback, but working with files can be slow. When precompiling in memory, it is faster, but that is not implemented in a way that yields good tracebacks.

Are people really executing load or attach in tight loops?

I can't imagine that anyone would do that.

I think that one should always have good tracebacks, so I would always use temporary files.

I completely agree. However, it seemed during the original discussion that I was the only one who always wanted good tracebacks. And someone made the switch to memory on purpose, for efficiency reasons.

comment:22 in reply to: ↑ 21 ; follow-up: Changed 7 years ago by jdemeyer

Replying to mstreng:

With a temporary file, there is a good traceback, but working with files can be slow. When precompiling in memory, it is faster, but that is not implemented in a way that yields good tracebacks.

I am thinking about #71 and it might be possible to have a traceback for a preparsed-in-memory file by re-preparsing the file when showing the traceback. I still have to check if that actually works.

And someone made the switch to memory on purpose

Indeed.

for efficiency reasons.

That's not clear to me, maybe it was just to simplify the code?

comment:23 in reply to: ↑ 22 Changed 7 years ago by mstreng

Replying to jdemeyer:

I am thinking about #71 and it might be possible to have a traceback for a preparsed-in-memory file by re-preparsing the file when showing the traceback. I still have to check if that actually works.

Sounds good.

Anyway, to get good tracebacks by default also for load, all you need to do is change

load_debug_mode = False 
attach_debug_mode = True 

both to True in sage/misc/preparser.py, except that the name debug_mode is a bit confusing when it is the default.

maybe it was just to simplify the code?

Simplifying and improving the code was indeed the purpose of #7514, where the switch to memory was made. The efficiency reason for the switch to memory was suggested to me by various people, not including the author of #7514.

Note: See TracTickets for help on using tickets.