Opened 3 years ago

Last modified 6 weeks ago

#26254 needs_info defect

Use Cython directive binding=True to get signatures in help for cythonized built-in methods

Reported by: klee Owned by:
Priority: major Milestone: sage-9.5
Component: user interface Keywords:
Cc: jdemeyer, tscrim Merged in:
Authors: Kwankyu Lee Reviewers:
Report Upstream: N/A Work issues:
Branch: public/26254 (Commits, GitHub, GitLab) Commit: 36ec4930832d840a73f1bcefe1d22bb27c566e49
Dependencies: #32509 Stopgaps:

Status badges

Description

In requests for help for cythonized built-in methods, the signature of the method is not shown, unlike normal python methods. For an example,

sage: a=17 
sage: a.quo_rem?

Docstring:     
   Returns the quotient and the remainder of self divided by other.
   Note that the remainder returned is always either zero or of the
   same sign as other.

   INPUT:

   * "other" - the divisor

   OUTPUT:

   * "q" - the quotient of self/other

   * "r" - the remainder of self/other

   EXAMPLES:

      sage: z = Integer(231)
      sage: z.quo_rem(2)
      (115, 1)
...

Simon:

There is sage_getargspec, which is supposed to find the signature of *all* functions/methods in Sage. If it doesn't, I believe it's a bug. And in the example, it does:

  sage: from sage.misc.sageinspect import sage_getargspec 
  sage: a = 17 
  sage: sage_getargspec(a.quo_rem) 
  ArgSpec(args=['self', 'other'], varargs=None, keywords=None, defaults=None) 

Actually I thought that ? and ?? would use sage.misc.sageinspect rather than Python's "inspect" module, and I am surprised that apparently it doesn't.

In addition there are certain special methods (_sage_doc_ and so on) that are used in sage_inspect, IIRC, so that wrappers such as @cached_method can forward the signature of the method being wrapped.

Erik:

It still won't work, say, for the built-in methods written in pure C, but there are few (if any?) of those in Sage itself.

Related tickets: #19100, #20860, #18192

Change History (47)

comment:1 Changed 3 years ago by klee

It seems this file

https://github.com/ipython/ipython/blob/master/IPython/core/oinspect.py

is responsible for this issue. For me, it would take some time to scrutinize what this does though.

comment:2 follow-up: Changed 3 years ago by klee

A fix is to redefine IPython.core.oinspect.getargspec to use sage.misc.sageinspect.sage_getargspec

comment:3 in reply to: ↑ 2 Changed 3 years ago by klee

Replying to klee:

A fix is to redefine IPython.core.oinspect.getargspec to use sage.misc.sageinspect.sage_getargspec

This is already done in sage.repl.ipython_extension.init_inspector. But apparently with no effect, strangely.

comment:4 Changed 3 years ago by klee

It turns out that the problem is with IPython.core.oinspect.inspector._get_def, which calls Python's inspect.signature via IPython.utils.signatures module.

This problem is nothing to do with sage_getargspec.

comment:5 Changed 3 years ago by klee

We may just wait for future sage based on Python 3 with inspect.signature supporting cython.

comment:6 Changed 2 years ago by dimpase

  • Milestone changed from sage-8.4 to sage-8.9

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

I don't see the signature in a Python 3 build of Sage, either.

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

Replying to jhpalmieri:

I don't see the signature in a Python 3 build of Sage, either.

Right.

I don't remember exactly what I meant in my last comment. Perhaps I expected Cython someday support the new signature module shipped with Python 3. Now I don't have any clear idea what should be done on what side.

comment:9 Changed 2 years ago by klee

  • Branch set to public/26254
  • Commit set to 0c78cdf26d45948216e6491ef2090396e782ac8f

New commits:

0c78cdfTurn on cython directive binding

comment:11 Changed 2 years ago by klee

  • Status changed from new to needs_info

comment:12 Changed 2 years ago by klee

  • Authors set to Kwankyu Lee
  • Status changed from needs_info to needs_review

comment:13 Changed 2 years ago by dimpase

With your patch I get a bunch of doctest errors, of the kind

sage -t --warn-long 55.8 src/sage/graphs/strongly_regular_db.pyx
**********************************************************************
File "src/sage/graphs/strongly_regular_db.pyx", line 1156, in sage.graphs.strongly_regular_db.is_RSHCD
Failed example:
    t = is_RSHCD(64,27,10,12); t
Expected:
    [<built-in function SRG_from_RSHCD>, 64, 27, 10, 12]
Got:
    [<cyfunction SRG_from_RSHCD at 0x7f8616d09890>, 64, 27, 10, 12]
**********************************************************************
sage -t --warn-long 55.8 src/sage/misc/latex.py
**********************************************************************
File "src/sage/misc/latex.py", line 561, in sage.misc.latex.has_latex_attr
Failed example:
    T._latex_()
Expected:
    Traceback (most recent call last):
    ...
    TypeError: descriptor '_latex_' of 'sage.matrix.matrix0.Matrix' object needs an argument
Got:
    <BLANKLINE>
    Traceback (most recent call last):
      File "/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 681, in _run
        self.compile_and_execute(example, compiler, test.globs)
      File "/home/scratch2/dimpase/sage/sage/local/lib/python2.7/site-packages/sage/doctest/forker.py", line 1105, in compile_and_execute
        exec(compiled, globs)
      File "<doctest sage.misc.latex.has_latex_attr[5]>", line 1, in <module>
        T._latex_()
    TypeError: unbound method cython_function_or_method object must be called with Matrix_integer_dense instance as first argument (got nothing instead)

--- this is of course not a surpise, but it needs to be fixed on this ticket.

Otherwise, I like it - e.g. notice how much more informative the error messages are.

comment:14 follow-up: Changed 2 years ago by nbruin

This comes with the penalty of producing a wrapped object every time a method is accessed on a cython object. I suspect cythonized access avoids that, so it may be that in most scenarios this doesn't come with a performance penalty, but one should check carefully that sage doesn't rely on the situations where it does.

Also, there is a reason why cython.binding==False by default: that's the behaviour built-in methods exhibit: [].insert returns a built-in method insert of list object ... rather than a bound method, and cython by default does the same. If you want more informative tracemacks, wouldn't it be better to solve it in such a way that straight-up CPython (and its C extension classes; of which cython is a special case) also benefit?

comment:15 in reply to: ↑ 14 ; follow-up: Changed 2 years ago by klee

Replying to nbruin:

This comes with the penalty of producing a wrapped object every time a method is accessed on a cython object. I suspect cythonized access avoids that, so it may be that in most scenarios this doesn't come with a performance penalty, but one should check carefully that sage doesn't rely on the situations where it does.

Also, there is a reason why cython.binding==False by default: that's the behaviour built-in methods exhibit: [].insert returns a built-in method insert of list object ... rather than a bound method, and cython by default does the same.

[].insert? shows correct signature. So built-in methods can behave well with respect to introspection. Then why cythonized built-in methods do not? How can we make cythonized built-in methos behave well like standard built-in methods of python?

comment:16 in reply to: ↑ 15 Changed 2 years ago by klee

[].insert? shows correct signature. So built-in methods can behave well with respect to introspection. Then why cythonized built-in methods do not? How can we make cythonized built-in methods behave well like standard built-in methods of python?

An answer can be found here:

https://stackoverflow.com/questions/1104823/python-c-extension-method-signatures-for-documentation/1104893

and

https://docs.python.org/3/howto/clinic.html

Now it seems to me that cython should do a better job in making cythonized built-ins more introspectable.

comment:17 Changed 2 years ago by klee

To summarize the current situation, there are two options:

Option 1: We accept the current patch, which turns on cython directive "binding=True" so that all cythonized methods become bound methods that already support the inspect.signature module well. If we take this path, then there is nothing for us to do except fixing a few doctests.

Option 2: We wait for upstream cython fixes that will make all cythonized built-in methods properly support the inspect.signature module. This is the path that standard built-in methods follow. We don't know when the upstream fix would be available.

Please give your preference and why.

comment:18 Changed 2 years ago by klee

  • Status changed from needs_review to needs_info

comment:19 follow-up: Changed 2 years ago by dimpase

To go with option 1, we need benchmarking results on whether it affects the performance a lot.

comment:20 in reply to: ↑ 19 Changed 2 years ago by klee

Replying to dimpase:

To go with option 1, we need benchmarking results on whether it affects the performance a lot.

If it affects any bit of the runtime performance in other aspect than introspection, then option 1 should be discarded. I think this should be decided not by experiments but analysis of how python and cython works.

comment:21 follow-up: Changed 2 years ago by dimpase

We can configure this in build time, to begin with. It is helpful for debugging - I would not care about a 5% or 15% performance hit, if error messages made more sense.

comment:22 in reply to: ↑ 21 Changed 2 years ago by SimonKing

Replying to dimpase:

We can configure this in build time, to begin with. It is helpful for debugging - I would not care about a 5% or 15% performance hit, if error messages made more sense.

I would.

comment:23 follow-ups: Changed 2 years ago by embray

  • Cc jdemeyer added

This is what Jeroen has been working on for like, literally the last year, perhaps longer :)

Yes, the solution is to use binding=True to enable use of cyfunctions. However, using cyfunctions across the board can introduce a significant performance penalty in many cases, as the Python interpreter has some built-in optimizations for built-in functions that don't work for cyfunctions.

Jeroen has been fighting for a series of PEPs that would overhaul Python's function type hierarchy in such a way that the basic function type can be extended (e.g. as with Cython's cyfunction) while still keeping those optimizations working.

So while this seems like it should be an easy problem to solve, it's completely non-trivial.

Point being, let's not duplicate effort here.

comment:24 in reply to: ↑ 23 Changed 2 years ago by klee

Replying to embray:

Point being, let's not duplicate effort here.

Thanks for the expert advice.

comment:25 Changed 22 months ago by embray

  • Milestone changed from sage-8.9 to sage-9.1

Ticket retargeted after milestone closed

comment:26 Changed 18 months ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.

comment:27 in reply to: ↑ 23 ; follow-up: Changed 16 months ago by klee

Replying to embray:

This is what Jeroen has been working on for like, literally the last year, perhaps longer :)

Jeroen has been fighting for a series of PEPs that would overhaul Python's function type hierarchy in such a way that the basic function type can be extended (e.g. as with Cython's cyfunction) while still keeping those optimizations working.

I searched for these PEPs, and reached to

I am curious if and how theses PEPs would eventually solve the problem of this ticket. I only guess that after the PEPs made into CPython, Cython is updated to use the new CPython features, and then the signature issue in Sage is automatically fixed. Am I right?

comment:28 Changed 15 months ago by mkoeppe

It would be interesting to know whether the upcoming Cython 3 (#29863) has improvements in this direction

comment:29 in reply to: ↑ 27 Changed 14 months ago by embray

Replying to klee:

Replying to embray:

This is what Jeroen has been working on for like, literally the last year, perhaps longer :)

Jeroen has been fighting for a series of PEPs that would overhaul Python's function type hierarchy in such a way that the basic function type can be extended (e.g. as with Cython's cyfunction) while still keeping those optimizations working.

I searched for these PEPs, and reached to

I am curious if and how theses PEPs would eventually solve the problem of this ticket. I only guess that after the PEPs made into CPython, Cython is updated to use the new CPython features, and then the signature issue in Sage is automatically fixed. Am I right?

That's correct--this would allow us to use Cython's own function subclass, which includes support for better signature documentation among other things, without losing any performance.

comment:30 Changed 12 months ago by mkoeppe

  • Milestone changed from sage-9.2 to sage-9.3

comment:31 Changed 7 months ago by mkoeppe

  • Milestone changed from sage-9.3 to sage-9.4

Setting new milestone based on a cursory review of ticket status, priority, and last modification date.

comment:32 Changed 3 months ago by mkoeppe

  • Milestone changed from sage-9.4 to sage-9.5

Setting a new milestone for this ticket based on a cursory review.

comment:33 Changed 7 weeks ago by mkoeppe

  • Summary changed from No signature shown in help for cythonized built-in methods to Use Cython directive binding=True to get signatures in help for cythonized built-in methods

comment:34 Changed 7 weeks ago by mkoeppe

binding=True will be the default in Cython 3 (https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#compiler-directives), so eventually we will make this switch anyway; so why not now.

Last edited 7 weeks ago by mkoeppe (previous) (diff)

comment:35 Changed 7 weeks ago by git

  • Commit changed from 0c78cdf26d45948216e6491ef2090396e782ac8f to f36275a78691ff374b8aab5d972cd72db6ce5268

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f36275aTurn on cython directive binding

comment:36 Changed 7 weeks ago by mkoeppe

  • Cc tscrim added
  • Status changed from needs_info to needs_review

comment:37 Changed 7 weeks ago by mkoeppe

Rebased on 9.5.beta0

comment:38 Changed 7 weeks ago by mkoeppe

I have set it to "needs review" so that the patchbot runs on it.

comment:39 Changed 7 weeks ago by mkoeppe

The old ticket #22747 attempted to use binding as well

comment:40 follow-up: Changed 7 weeks ago by jhpalmieri

Are there performance penalties for doing this, perhaps that Cython 3 is going to address?

comment:41 Changed 7 weeks ago by git

  • Commit changed from f36275a78691ff374b8aab5d972cd72db6ce5268 to 36ec4930832d840a73f1bcefe1d22bb27c566e49

Branch pushed to git repo; I updated commit sha1. New commits:

36ec493Reformat the comment

comment:42 in reply to: ↑ 40 ; follow-up: Changed 7 weeks ago by klee

Replying to jhpalmieri:

Are there performance penalties for doing this...?

How can we see the performance penalty?

comment:43 in reply to: ↑ 42 ; follow-up: Changed 7 weeks ago by jhpalmieri

Replying to klee:

Replying to jhpalmieri:

Are there performance penalties for doing this...?

How can we see the performance penalty?

Try builds with and without and compare some timings?

comment:44 in reply to: ↑ 43 ; follow-up: Changed 7 weeks ago by klee

Replying to jhpalmieri:

Replying to klee:

Replying to jhpalmieri:

Are there performance penalties for doing this...?

How can we see the performance penalty?

Try builds with and without and compare some timings?

I tried a very simple script like: timeit('a=17;a.quo_rem(5); del a'), and find no difference.

I wonder what is a proper way to see the difference...

comment:45 Changed 6 weeks ago by mkoeppe

  • Dependencies set to #32509
  • Status changed from needs_review to needs_info

comment:46 in reply to: ↑ 44 Changed 6 weeks ago by jhpalmieri

Replying to klee:

Replying to jhpalmieri:

Replying to klee:

Replying to jhpalmieri:

Are there performance penalties for doing this...?

How can we see the performance penalty?

Try builds with and without and compare some timings?

I tried a very simple script like: timeit('a=17;a.quo_rem(5); del a'), and find no difference.

I wonder what is a proper way to see the difference...

I ran ./sage -t --long src/sage/matrix/*.pyx a few times:

Develop: average time 83.3 seconds.

This ticket: average time 93.1 seconds.

I also tried ./sage -t --long src/sage/matrix/matrix_gfpn_dense.pyx a few times:

Develop: average time 28.4 seconds

This ticket: average time 37.0 seconds

Last edited 6 weeks ago by jhpalmieri (previous) (diff)

comment:47 Changed 6 weeks ago by jhpalmieri

Some other files in matrix showed very little difference, so maybe the slowdown only occurs in certain types of operations.

Note: See TracTickets for help on using tickets.