Use Cython directive binding=True to get signatures for cython methods

[kwankyu]

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)
...

To fix this, we set Cython directive binding=True. Thus we buy

• better support of introspection into cython methods,
• consistent help messages for cython methods like python methods,
• better tracebacks on exceptions,
• better behaving for documentation

for slight performance degradation due to increased overhead cost of calling cython methods.

Related tickets: #19100, #20860, #18192

Depends on #32509
Depends on #33864

CC: @jdemeyer @tscrim @mkoeppe

Component: user interface

Author: Kwankyu Lee, Tobias Diez

Branch/Commit: public/26254 @ 326f19c

Reviewer: Tobias Diez, ...

Issue created by migration from https://trac.sagemath.org/ticket/26254

[kwankyu]

comment:1

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.

[kwankyu]

comment:2

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

[kwankyu]

comment:3

Replying to @kwankyu:

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.

[kwankyu]

comment:4

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.

[kwankyu]

comment:5

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

[jhpalmieri]

comment:7

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

[kwankyu]

comment:8

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.

[kwankyu]

New commits:

0c78cdf

Turn on cython directive binding

[kwankyu]

Commit: 0c78cdf

[kwankyu]

Branch: public/26254

[kwankyu]

comment:10

Based on this discussion:

https://stackoverflow.com/questions/46033277/how-to-introspect-a-function-defined-in-a-cython-c-extension-module

and consulting:

https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html#compiler-directives

I made the last commit. Please checkout and try.

[kwankyu]

Author: Kwankyu Lee

[dimpase]

comment:13

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.

[nbruin]

comment:14

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?

[kwankyu]

comment:15

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?

[kwankyu]

comment:16

[].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.

[kwankyu]

comment:17

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.

[dimpase]

comment:19

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

[kwankyu]

comment:20

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.

[kwankyu]

comment:126

Replying to Nils Bruin:

I don't think _xxx is necessarily a good indicator of the cost of the call.

I agree. I guess there is no good indicator.

[kwankyu]

comment:127

Replying to Nils Bruin:

A routine like matrix.nrows() on the other hand is about as cheap as it gets and that's not an underscored method.

After this ticekt, that method should be only for a user. For internal use (as accessor in a tight loop), matrix._nrows or matrix._nrows() should be used (or implemented if not exist yet).

[sagetrac-git]

Changed commit from 31718eb to 398a910

[sagetrac-git]

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

398a910

Merge branch 'develop' into t/26254/cython-binding

[nbruin]

comment:130

Reported upstream:
cython/cython#5144

Preliminary timings there suggest the problem disappears with cython 3. So it would be instructive to check if the problem above persists in sage with an upgraded cython. If not, then we can just close this ticket and not do anything!

[kwankyu]

comment:131

Thanks for the report.

It would be interesting to see a real-world nontrivial (unlike base_ring()) example that has perceivable performance degradation.

[nbruin]

comment:132

With Cython 3.0.0a11 it looks like "Binding" performs uniformly better. So I think we should wait for Cython 3 to come out and when we transition we can close this ticket, because the default changes there anyway.

[kwankyu]

comment:133

Replying to Nils Bruin:

With Cython 3.0.0a11 it looks like "Binding" performs uniformly better.

That's good news!

So I think we should wait for Cython 3 to come out and when we transition we can close this ticket, because the default changes there anyway.

Except one line that changes the default, the other changes of this ticket (doctest fixes and adding @cython.binding(False)) will still be necessary and useful. This ticket paves the way for Cython 3. So do you mean "merge" by "close"?

[kwankyu]

comment:134

I looked at the upstream discussion. They say "Binding" is faster than "Unbinding"! That is almost unbelievably good news :)

Then it means that adding @cython.binding(False) is actually harmful with Cython 3!?

If that is really true, then we can close this ticket and merge the commit for fixing doctest failures to the Cython 3 ticket.

[tobiasdiez]

comment:135

Are there any news when Cython 3 will finally be released? If not, in light of the finding above that the bindings don't lead to (serious) performance decreases in "real life" even with cython 0.2x, I would suggest to merge this ticket without the manual binding(false). In this way, we would already profit from the better debugging experience and documentation and once Cython 3 comes around gain back the performance loss.

[kwankyu]

comment:136

Replying to Tobias Diez:

Are there any news when Cython 3 will finally be released?

It seems to go in beta status soon.

If not, in light of the finding above that the bindings don't lead to (serious) performance decreases in "real life" even with cython 0.2x, I would suggest to merge this ticket without the manual binding(false). In this way, we would already profit from the better debugging experience and documentation and once Cython 3 comes around gain back the performance loss.

+1 from me. But I doubt if we could agree on this. If there is a disagreement, we cannot help but go to the way Nils suggested.

[sagetrac-git]

Changed commit from 398a910 to 326f19c

[sagetrac-git]

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

326f19c

Merge branch 'develop' into t/26254/cython-binding

[kwankyu]

comment:138

Peeled off the commit of adding @cython.binding(False).

[nbruin]

comment:139

I'd say wait for Cython 3 and, in the mean time, use ​0c78cdf for development work if you're testing for the consequences of binding=True. Or perhaps just make that change manually if you want a debugging session with more details.

In any case, the branch here as it is shouldn't be merged: its net effect is an addition of 1 line (which can be reverted when cython 3 is merged). Rebase the ​0c78cdf branch instead (or perhaps just force push it to the ticket) There's no need to pollute the change history with an incredible amount of line-addition-then-removal changes.

[kwankyu]

comment:140

Replying to Nils Bruin:

Rebase the ​0c78cdf branch instead (or perhaps just force push it to the ticket) There's no need to pollute the change history with an incredible amount of line-addition-then-removal changes.

Did you see comment:138 and the latest branch? It contains changes that will be necessary also for Cython 3.

[nbruin]

comment:141

Replying to Kwankyu Lee:

Did you see comment:138 and the latest branch? It contains changes that will be necessary also for Cython 3.

OK, sorry. It looks like you already cleaned up the branch (I now see the last push was indeed a "forced push"). Yes, this branch is fine then, but due to performance ramifications I think we should still wait until we're ready to upgrade to Cython 3 .

Note that the cython folk pointed to cython/cython#4735 which may be affecting our performance with Python 3.10+ . It may be extra reason to upgrade to Cython 3. I don't know if upgrading to an "alpha" is warranted. Apparently :

... if you want better performance, use Cython 3. If you value extreme language stability instead, stay with Cython 0.29.x for now. But be aware that maintenance for 0.29.x will probably end soon after Cython 3.0 is out. And that won't take as forever as it seemed for the last few years.

So ... less forever until we can upgrade to a released Cython 3!

[tobiasdiez]

comment:142

Replying to Nils Bruin:

... if you want better performance, use Cython 3. If you value extreme language stability instead, stay with Cython 0.29.x for now. But be aware that maintenance for 0.29.x will probably end soon after Cython 3.0 is out. And that won't take as forever as it seemed for the last few years.

So ... less forever until we can upgrade to a released Cython 3!

Well, this was comment is already more than half a year old. The most recent comment about a beta of Cython 3 that I could find is cython/cython#4022 (comment), which is also 5 months old with no update since. Taking into account that the upgrade from v0.2x to v3 in Sage is also non-trivial (#29863), I would be surprised if Cython 3 will be fully supported in sage in the next few months.

[kwankyu]

comment:143

This ticket is blocking other tickets (#27578, #30884, #31309, ...) for documentation improvement because of cython methods behaving differently with python methods.

We may endure slight performance degradation (which, I think, no one will notice in real computing life) to enjoy conveniences brought by this ticket and allow progress of the mentioned tickets, in the meantime we wait for Cython 3 to arrive.