Opened 9 years ago

Closed 8 years ago

#11913 closed defect (fixed)

Notebook hang in ?? source display Trackback

Reported by: vbraun Owned by: jason, mpatel, was
Priority: major Milestone: sage-5.4
Component: notebook Keywords: sd41
Cc: SimonKing Merged in: sage-5.4.beta0
Authors: John Palmieri Reviewers: Keshav Kini
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #13121 Stopgaps:

Description (last modified by jhpalmieri)

In the notebook, looking at the source of some methods ends with a Traceback and then the notebook keeps being busy until I interrupt the computation.

Steps to reproduce:

  1. Open notebook (tested on sage-4.7.2.alpha2 and alpha3)
  1. create matrix, e.g. (without the sage:)
    sage: m = random_matrix(ZZ,100);  m
    
  1. look at source of the determinant() method:
    sage: m.determinant??
    

It seems like the source formatter gets confused by the traceback in the method's docstring. See attached screenshot for illustration.


Apply trac_11913-sage.patch to the Sage library. Also merge the pull request for sagenb.

Attachments (2)

screenshot.png (145.7 KB) - added by vbraun 9 years ago.
Screenshot of the problem
trac_11913-sage.patch (1.6 KB) - added by jhpalmieri 8 years ago.
Sage library

Download all attachments as: .zip

Change History (23)

Changed 9 years ago by vbraun

Screenshot of the problem

comment:1 Changed 8 years ago by SimonKing

  • Cc SimonKing added

comment:2 Changed 8 years ago by SimonKing

The traceback is hidden by the method "format_exception" in sage.server.notebook.cell. That function is called by the method parse_html, if the cell is not interactive (line 1500 of sage.server.notebook.cell).

comment:3 Changed 8 years ago by SimonKing

Strange. I modified the function, such that some protocol output is put into a file when it is called. But apparently it is NOT called when requesting the source of an object.

Is there code for the notebook that can not be found with search_src?

comment:4 Changed 8 years ago by SimonKing

Meanwhile it seems to me that the notebook is reinventing the wheel. Namely, devel/sagenb/sagenb contains files that seem to be old snapshots of corresponding files in devel/sage/sage. For example, misc/sageinspect.py exists in both repositories -- and they are different, the version in devel/sagenb lacks some important patches.

Probably, for this ticket, working on the "format_exception" function will be enough. But I do think that the code duplication and the lack of synchronisation between devel/sage and devel/sagenb is a bug that deserves its own ticket.

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

The notebook no longer uses sage.server.notebook.cell; see SAGE_ROOT/devel/sagenb/... instead.

It looks like this issue is also being tracked for the notebook at https://github.com/sagemath/sagenb/issues/34. Not that they've made any progress on it...

At one point, I also thought I had opened a sagenb ticket saying that they needed to synchronize with the newest version of Sage's sageinspect, but I couldn't find that one.

comment:6 in reply to: ↑ 5 Changed 8 years ago by SimonKing

Replying to jhpalmieri:

The notebook no longer uses sage.server.notebook.cell; see SAGE_ROOT/devel/sagenb/... instead.

Yep. Meanwhile I figured that out.

It looks like this issue is also being tracked for the notebook at https://github.com/sagemath/sagenb/issues/34. Not that they've made any progress on it...

That is really an awkward situation. I would try to fix the problem using trac. But I would certainly not learn how to use github just for fixing a notebook problem: After all, I am hardly using the notebook, myself.

At one point, I also thought I had opened a sagenb ticket saying that they needed to synchronize with the newest version of Sage's sageinspect, but I couldn't find that one.

Sagenb ticket meaning that it is a github thingy? Or is it here?

I think, if there is new stuff in devel/sagenb/sagenb/misc and in devel/sagenb/sagenb/interfaces, then it should be moved to devel/sage/sage/misc and devel/sage/sage/interfaces. Afterwards, devel/sagenb/sagenb/misc and devel/sagenb/sagenb/interfaces should be deleted and the stuff from the sage repository should be used instead.

In other words, I think that synchronisation is the wrong approach, since synchronisation would preserve the duplication of code.

Code that is found elsewhere in sage or even new code that is likely to be useful elsewhere in sage does not belong to devel/sagenb/sagenb/ and should be (re-)moved.

comment:7 follow-up: Changed 8 years ago by jhpalmieri

But sagenb is supposed to be a stand-alone project; people should be able to use it for other purposes, not just within Sage. So from that standpoint, maybe this code should all be moved out of the Sage library and just into sagenb; since sagenb is part of Sage, Sage could still use it. (I'm not sure I agree with this, but perhaps we should consider it.)

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

Replying to jhpalmieri:

But sagenb is supposed to be a stand-alone project; people should be able to use it for other purposes, not just within Sage.

sageinspect.py is designed to inspect Sage source code. It first tries to use python's inspect module to do the job, but when it fails (and this is likely the case in Cython code) then it tries to find the source files - and these source files are searched for in the devel/sage/sage repository.

Hence, if what you say is true, then the notebook should not use sageinspect.py at all.

comment:9 Changed 8 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Status changed from new to needs_review

I've submitted a pull request for sagenb. Go to this link to see the changes (very much like looking at an attached patch on trac). Fixing this also requires a small modification to the Sage library, related to #10860: as far as I can tell, the patch at #10860 relied on sagenb's somewhat broken version of sageinspect.py, so once we use Sage's version, the patch at #10860 no longer quite works. Hence the patch to the Sage library here.

comment:10 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:11 Changed 8 years ago by jhpalmieri

I'm not sure mine is a perfect solution, but it basically works. I still see the "Traceback", but nothing hangs. If I click to the left of the docstring, it shows the rest of the source.

Edit: maybe what I mean to say is this: I think my changes are beneficial and should be merged anyway. Then we can tinker with how the notebook displays source code involving Tracebacks afterwards.

Last edited 8 years ago by jhpalmieri (previous) (diff)

comment:12 Changed 8 years ago by kini

  • Keywords sd41 added
  • Reviewers set to Keshav Kini
  • Status changed from needs_review to positive_review

Tests pass, and I like your solution, though you're right that it's only a solution to the sageinspect duplication stuff and the hang, and not of issue #34.

comment:13 Changed 8 years ago by jdemeyer

  • Dependencies set to #11080
  • Milestone changed from sage-5.1 to sage-5.2

comment:14 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_info

Is this sagenb patch merged in the spkg at #11080? If not, this ticket needs a proper sagenb spkg.

comment:15 Changed 8 years ago by jhpalmieri

The patch to the Sage library can be merged independently of any changes to sagenb, and that patch does fix a bug (as described in the new doctest). The changes to sagenb actually attempt to fix the problem described in the ticket.

comment:16 Changed 8 years ago by jhpalmieri

  • Dependencies changed from #11080 to #13121
  • Status changed from needs_info to needs_review

The relevant sagenb spkg will be at #13121.

comment:17 Changed 8 years ago by jhpalmieri

  • Status changed from needs_review to positive_review

Changed 8 years ago by jhpalmieri

Sage library

comment:18 Changed 8 years ago by jhpalmieri

(I just attached a new version of the patch which uses proper Sage/Sphinx markup for trac ticket references. Keshav, who is sitting next to me, still gives it a positive review.)

comment:19 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-5.2 to sage-pending

comment:20 Changed 8 years ago by jdemeyer

  • Milestone changed from sage-pending to sage-5.4

comment:21 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.4.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.