Opened 6 years ago

Closed 5 years ago

#13916 closed defect (fixed)

Fix inspection of interactive Cython code

Reported by: SimonKing Owned by: jason
Priority: major Milestone: sage-6.2
Component: misc Keywords: days57
Cc: robertwb Merged in:
Authors: Simon King Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: 69169ab (Commits) Commit: 69169ab524e3273bfa36cad94dc735fe80cf029b
Dependencies: Stopgaps:

Description

The following happens with sage-5.6.b2, at least in the debug version from #13864 (I could imagine that the new Cython version from #13896 could be related as well):

sage: cython('''cpdef test_funct(x,y): return''')
sage: from sage.misc.sageinspect import sage_getfile
sage: os.path.exists(sage_getfile(test_funct))
False

IIRC, it used to work. hence, I suppose some paths have changed.

Attachments (1)

trac13916_inspect_interactive_cython.patch (2.1 KB) - added by SimonKing 6 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 6 years ago by SimonKing

Could it be that simply Cython is writing the wrong information into the doc?

sage: _sage_getdoc_unformatted(test_funct)
'File: _home_simon__sage_temp_linux_sqwp_site_6004_tmp_rJ1Nyc_spyx_0.pyx (starting at line 6)'

But the file is at

~/.sage/temp/linux_sqwp.site/6004/tmp_rJ1Nyc.spyx

or

~/.sage/temp/linux_sqwp.site/6004/spyx/_home_simon__sage_temp_linux_sqwp_site_6004_tmp_rJ1Nyc_spyx/tmp_rJ1Nyc.spyx

(they coincide bit-wise). Note that even the ".pyx" information is wrong.

comment:2 Changed 6 years ago by SimonKing

  • Cc robertwb added

comment:3 Changed 6 years ago by SimonKing

Actually there is a pyx file (I am in a new session now, hence, the names have changed):

~/.sage/temp/linux_sqwp.site/6593/spyx/_home_simon__sage_temp_linux_sqwp_site_6593_tmp_mqjIHk_spyx/_home_simon__sage_temp_linux_sqwp_site_6593_tmp_mqjIHk_spyx_0.pyx

The question is: Do we want to see the spyx file or the pyx file? Anyway, I think I'll manage to make sageinspect find that pyx file.

comment:4 Changed 6 years ago by SimonKing

  • Authors set to Simon King
  • Status changed from new to needs_review

With the attached patch, the following doctest works.

        sage: cython('''cpdef test_funct(x,y): return''')
        sage: print open(_extract_embedded_position(inspect.getdoc(test_funct))[1]).read()
        <BLANKLINE>
        include "interrupt.pxi"  # ctrl-c interrupt block support
        include "stdsage.pxi"  # ctrl-c interrupt block support
        <BLANKLINE>
        include "cdefs.pxi"
        cpdef test_funct(x,y): return

The tests of sage/misc/sageinspect.py pass. Needs review!

comment:5 follow-up: Changed 6 years ago by Snark

First, about your patch:

  1. it makes sage_getfile work on the box where I have doctest issues, which is positive ;
  2. but shouldn't os.path.join(SAGE_ROOT,'devel/sage', raw_filename) be os.path.join(SAGE_ROOT, 'devel', 'sage', raw_filename)? You're basically assuming that '/' is the path separator, which is perhaps a portability problem.

On the box where I patched paths, the source file looks like this:

include "sage/ext/stdsage.pxi"  # ctrl-c interrupt block support

include "cdefs.pxi"
cpdef test_funct(x,y): return

while on another, unpatched, box (again, initial empty line is eaten by trac) it is:

include "interrupt.pxi"  # ctrl-c interrupt block support
include "stdsage.pxi"  # ctrl-c interrupt block support

include "cdefs.pxi"
cpdef test_funct(x,y): return

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

Replying to Snark:

  1. but shouldn't os.path.join(SAGE_ROOT,'devel/sage', raw_filename) be os.path.join(SAGE_ROOT, 'devel', 'sage', raw_filename)? You're basically assuming that '/' is the path separator, which is perhaps a portability problem.

Yes, you are right. I'll update the patch in a minute.

On the box where I patched paths, the source file looks like this:

include "sage/ext/stdsage.pxi"  # ctrl-c interrupt block support

include "cdefs.pxi"
cpdef test_funct(x,y): return

while on another, unpatched, box (again, initial empty line is eaten by trac) it is:

include "interrupt.pxi"  # ctrl-c interrupt block support
include "stdsage.pxi"  # ctrl-c interrupt block support

include "cdefs.pxi"
cpdef test_funct(x,y): return

OK. That explains why the line number has changed, because the number of lines in front of the function definition has changed. And I suppose the change is indeed related with your patch from #12728, isn't it?

The patch from here is self-contained, but one old test and the new test from here will change with #12728. Hence, I suggest to use this ticket as a new dependency for #12728, and you add a patch on #12728 that fixes the doctests.

Changed 6 years ago by SimonKing

comment:7 Changed 6 years ago by SimonKing

The patch is updated, still needs review.

comment:8 follow-up: Changed 6 years ago by Snark

I found a problem in my own patch, so this trac ticket is independent from #12728.

Simon, your patch looks nice, applies nice and compiles nice ; but since I'm going to get another round of rebuilding and full doctesting sage, I'll report what happens later (I'm not doing that on the arm box, so it shouldn't take two days :-P).

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

Replying to Snark:

I found a problem in my own patch, so this trac ticket is independent from #12728.

Simon, your patch looks nice, applies nice and compiles nice ; but since I'm going to get another round of rebuilding and full doctesting sage, I'll report what happens later (I'm not doing that on the arm box, so it shouldn't take two days :-P).

Now two weeks are over. Did it work?

comment:10 Changed 6 years ago by SimonKing

Since the coverage script complains, I'll try to add some new tests in sageinspect.py

comment:11 Changed 6 years ago by SimonKing

PS: The error reported by the patchbot seems unrelated.

comment:12 Changed 6 years ago by SimonKing

PPS: The coverage script reports nonsense. It says that the number of tests did not increase, but the number of functions did increase. But that's plain wrong: My patch does not introduce new functions, it only fixes existing functions. And it does add tests. So, I'll keep it like that, if you don't mind.

comment:13 Changed 6 years ago by jdemeyer

  • Milestone changed from sage-5.11 to sage-5.12

comment:14 Changed 5 years ago by vbraun_spam

  • Milestone changed from sage-6.1 to sage-6.2

comment:15 Changed 5 years ago by mmezzarobba

  • Branch set to u/mmezzarobba/13916-inspect_interactive_cython
  • Commit set to 69169ab524e3273bfa36cad94dc735fe80cf029b

Didn't apply cleanly to 6.0+; updated; needs review again.

comment:16 Changed 5 years ago by vbraun

  • Keywords days57 added
  • Reviewers set to Volker Braun

Looks good to me

comment:17 Changed 5 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:18 Changed 5 years ago by vbraun

  • Branch changed from u/mmezzarobba/13916-inspect_interactive_cython to 69169ab524e3273bfa36cad94dc735fe80cf029b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.