Open kwankyu opened 6 years ago
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.
A fix is to redefine IPython.core.oinspect.getargspec
to use sage.misc.sageinspect.sage_getargspec
Replying to @kwankyu:
A fix is to redefine
IPython.core.oinspect.getargspec
to usesage.misc.sageinspect.sage_getargspec
This is already done in sage.repl.ipython_extension.init_inspector
. But apparently with no effect, strangely.
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
.
We may just wait for future sage based on Python 3 with inspect.signature
supporting cython.
I don't see the signature in a Python 3 build of Sage, either.
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.
Branch: public/26254
Based on this discussion:
and consulting:
I made the last commit. Please checkout and try.
Author: Kwankyu Lee
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.
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?
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 abuilt-in method insert of list object ...
rather than abound 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?
[].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:
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.
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.
To go with option 1, we need benchmarking results on whether it affects the performance a lot.
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.
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.
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.
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.
Replying to @embray:
Point being, let's not duplicate effort here.
Thanks for the expert advice.
Ticket retargeted after milestone closed
Moving tickets to milestone sage-9.2 based on a review of last modification date, branch status, and severity.
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'scyfunction
) 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?
It would be interesting to know whether the upcoming Cython 3 (#29863) has improvements in this direction
Replying to @kwankyu:
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'scyfunction
) 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.
Setting new milestone based on a cursory review of ticket status, priority, and last modification date.
Setting a new milestone for this ticket based on a cursory review.
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.
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f36275a | Turn on cython directive binding |
Rebased on 9.5.beta0
I have set it to "needs review" so that the patchbot runs on it.
The old ticket #22747 attempted to use binding
as well
Are there performance penalties for doing this, perhaps that Cython 3 is going to address?
Branch pushed to git repo; I updated commit sha1. New commits:
36ec493 | Reformat the comment |
Replying to @jhpalmieri:
Are there performance penalties for doing this...?
How can we see the performance penalty?
Replying to @kwankyu:
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?
Replying to @jhpalmieri:
Replying to @kwankyu:
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...
Dependencies: #32509
Replying to @kwankyu:
Replying to @jhpalmieri:
Replying to @kwankyu:
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
Some other files in matrix
showed very little difference, so maybe the slowdown only occurs in certain types of operations.
From:
https://cython.readthedocs.io/en/latest/src/userguide/source_files_and_compilation.html
... When enabled, functions will bind to an instance when looked up as a class attribute
I don't know what triggers the binding behaviour, but I imagine there may be a code path that runs into this and perhaps ends up not binding anyway (thus creating overhead) or ends up binding in a way that was previously done in a more efficient way (cached perhaps?)
The timings above show the impact can be quite significant: I think too high a penalty to incur in general. Note that the documentation also says:
Changed in version 3.0.0: Default changed from False to True
so figuring out what's causing the slowdown is a prereq to upgrading to 3.0.0 (once that finally is released). If there's a particular scenario where it's bad to have binding, we might just be able to turn it off in those cases.
What's the status here? The performance issues don't seem to be too bad, especially since they apparently only affect certain functions/modules.
Binding=true is also required for #30884 since the decorator library internally uses inspection on the decorated function. So if one wants to decorate cython functions, then they have to be "bound".
Replying to @tobiasdiez:
What's the status here? The performance issues don't seem to be too bad, especially since they apparently only affect certain functions/modules.
I agree with comment:48. The penalty seems significant to me. We need to know how much damage we would get and where, and to see if there is a way to reduce the damage.
Branch pushed to git repo; I updated commit sha1. New commits:
52b5089 | Merge remote-tracking branch 'origin/develop' into public/26254 |
In requests for help for cythonized built-in methods, the signature of the method is not shown, unlike normal python methods. For an example,
To fix this, we set Cython directive
binding=True
. Thus we buyfor 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