#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: |
Description (last modified by )
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)
Change History (27)
comment:1 Changed 9 years ago by
- Description modified (diff)
comment:2 Changed 9 years ago by
- Description modified (diff)
Changed 9 years ago by
comment:3 Changed 9 years ago by
- Description modified (diff)
comment:4 Changed 9 years ago by
- Status changed from new to needs_review
comment:5 Changed 9 years ago by
- 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 9 years ago by
- Description modified (diff)
- Status changed from needs_info to needs_review
only apply 11812.patch
comment:7 follow-up: ↓ 8 Changed 9 years ago by
A few minor issues:
if preparse_to_file == None:
should beif 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).
comment:8 in reply to: ↑ 7 Changed 9 years ago by
Replying to leif:
if preparse_to_file == None:
should beif 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 thePExpect
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 content of the traceback in the example
sage: attach('myfile.sage')
on line 1573 of trac_11812-traceback_attach.patch is ignored in doctests according to this link. And indeed, that example passes the doctests even with #11812 not applied.
- 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 9 years ago by
I would like to remind the patchbot to apply 11812.patch only.
comment:10 Changed 9 years ago by
- 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 9 years ago by
- Cc johanbosman added
I'm writing a new patch.
Unrelated to that, see also #11925.
Changed 9 years ago by
comment:12 Changed 9 years ago by
- Description modified (diff)
New patch that does work. Apply 11812.3.patch only. (testing now)
Changed 9 years ago by
comment:14 follow-up: ↓ 16 Changed 9 years ago by
- Reviewers set to Johan Bosman
- Status changed from needs_review to positive_review
Looks good. ;).
comment:15 Changed 9 years ago by
- Milestone changed from sage-4.7.2 to sage-4.7.3
comment:16 in reply to: ↑ 14 Changed 9 years ago by
- Keywords load attach traceback added
Replying to johanbosman:
Thanks!
comment:17 Changed 9 years ago by
- Merged in set to sage-4.7.3.alpha0
- Resolution set to fixed
- Status changed from positive_review to closed
comment:19 Changed 9 years ago by
- 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: ↓ 21 Changed 6 years ago by
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: ↓ 22 Changed 6 years ago by
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
orattach
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: ↓ 23 Changed 6 years ago by
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 6 years ago by
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.
All tests passed on sage 4.7