Opened 10 years ago
Closed 9 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 )
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:
- Open notebook (tested on sage-4.7.2.alpha2 and alpha3)
- create matrix, e.g. (without the sage:)
sage: m = random_matrix(ZZ,100); m
- 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)
Change History (23)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Cc SimonKing added
comment:2 Changed 10 years ago by
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 10 years ago by
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 10 years ago by
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: ↓ 6 Changed 10 years ago by
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 10 years ago by
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: ↓ 8 Changed 10 years ago by
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 10 years ago by
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 9 years ago by
- 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 9 years ago by
- Description modified (diff)
comment:11 Changed 9 years ago by
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.
comment:12 Changed 9 years ago by
- 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 9 years ago by
- Dependencies set to #11080
- Milestone changed from sage-5.1 to sage-5.2
comment:14 Changed 9 years ago by
- 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 9 years ago by
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 9 years ago by
- 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 9 years ago by
- Status changed from needs_review to positive_review
comment:18 Changed 9 years ago by
(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 9 years ago by
- Milestone changed from sage-5.2 to sage-pending
comment:20 Changed 9 years ago by
- Milestone changed from sage-pending to sage-5.4
comment:21 Changed 9 years ago by
- Merged in set to sage-5.4.beta0
- Resolution set to fixed
- Status changed from positive_review to closed
Screenshot of the problem