Opened 4 years ago
Last modified 14 hours 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.7 |
Component: | user interface | Keywords: | |
Cc: | jdemeyer, tscrim, mkoeppe | Merged in: | |
Authors: | Kwankyu Lee | Reviewers: | |
Report Upstream: | N/A | Work issues: | |
Branch: | public/26254 (Commits, GitHub, GitLab) | Commit: | f5bc80d0d33902c088753ee9592972b72e6619e7 |
Dependencies: | #32509, #33864 | Stopgaps: |
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 usesage.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 insage_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.
Attachments (1)
Change History (78)
comment:1 Changed 4 years ago by
comment:2 follow-up: ↓ 3 Changed 4 years ago by
A fix is to redefine IPython.core.oinspect.getargspec
to use sage.misc.sageinspect.sage_getargspec
comment:3 in reply to: ↑ 2 Changed 4 years ago by
Replying to klee:
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.
comment:4 Changed 4 years ago by
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 4 years ago by
We may just wait for future sage based on Python 3 with inspect.signature
supporting cython.
comment:6 Changed 3 years ago by
- Milestone changed from sage-8.4 to sage-8.9
comment:7 follow-up: ↓ 8 Changed 3 years ago by
I don't see the signature in a Python 3 build of Sage, either.
comment:8 in reply to: ↑ 7 Changed 3 years ago by
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 3 years ago by
- Branch set to public/26254
- Commit set to 0c78cdf26d45948216e6491ef2090396e782ac8f
New commits:
0c78cdf | Turn on cython directive binding
|
comment:10 Changed 3 years ago by
Based on this discussion:
and consulting:
I made the last commit. Please checkout and try.
comment:11 Changed 3 years ago by
- Status changed from new to needs_info
comment:12 Changed 3 years ago by
- Status changed from needs_info to needs_review
comment:13 Changed 3 years ago by
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: ↓ 15 Changed 3 years ago by
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: ↓ 16 Changed 3 years ago by
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?
comment:16 in reply to: ↑ 15 Changed 3 years ago by
[].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.
comment:17 Changed 3 years ago by
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 3 years ago by
- Status changed from needs_review to needs_info
comment:19 follow-up: ↓ 20 Changed 3 years ago by
To go with option 1, we need benchmarking results on whether it affects the performance a lot.
comment:20 in reply to: ↑ 19 Changed 3 years ago by
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: ↓ 22 Changed 3 years ago by
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 3 years ago by
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: ↓ 24 ↓ 27 Changed 3 years ago by
- 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 3 years ago by
comment:25 Changed 3 years ago by
- Milestone changed from sage-8.9 to sage-9.1
Ticket retargeted after milestone closed
comment:26 Changed 2 years ago by
- 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: ↓ 29 Changed 2 years ago by
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?
comment:28 Changed 2 years ago by
It would be interesting to know whether the upcoming Cython 3 (#29863) has improvements in this direction
comment:29 in reply to: ↑ 27 Changed 2 years ago by
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'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.
comment:30 Changed 22 months ago by
- Milestone changed from sage-9.2 to sage-9.3
comment:31 Changed 17 months ago by
- 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 13 months ago by
- 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 11 months ago by
- 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 11 months ago by
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.
comment:35 Changed 11 months ago by
- Commit changed from 0c78cdf26d45948216e6491ef2090396e782ac8f to f36275a78691ff374b8aab5d972cd72db6ce5268
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f36275a | Turn on cython directive binding
|
comment:36 Changed 11 months ago by
- Cc tscrim added
- Status changed from needs_info to needs_review
comment:37 Changed 11 months ago by
Rebased on 9.5.beta0
comment:38 Changed 11 months ago by
I have set it to "needs review" so that the patchbot runs on it.
comment:39 Changed 11 months ago by
The old ticket #22747 attempted to use binding
as well
comment:40 follow-up: ↓ 42 Changed 11 months ago by
Are there performance penalties for doing this, perhaps that Cython 3 is going to address?
comment:41 Changed 11 months ago by
- Commit changed from f36275a78691ff374b8aab5d972cd72db6ce5268 to 36ec4930832d840a73f1bcefe1d22bb27c566e49
Branch pushed to git repo; I updated commit sha1. New commits:
36ec493 | Reformat the comment
|
comment:42 in reply to: ↑ 40 ; follow-up: ↓ 43 Changed 11 months ago by
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: ↓ 44 Changed 11 months ago by
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: ↓ 46 Changed 11 months ago by
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 11 months ago by
- Dependencies set to #32509
- Status changed from needs_review to needs_info
comment:46 in reply to: ↑ 44 Changed 11 months ago by
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
comment:47 Changed 11 months ago by
Some other files in matrix
showed very little difference, so maybe the slowdown only occurs in certain types of operations.
comment:48 Changed 9 months ago by
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.
comment:49 Changed 8 months ago by
- Milestone changed from sage-9.5 to sage-9.6
comment:50 follow-up: ↓ 51 Changed 7 months ago by
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".
comment:51 in reply to: ↑ 50 Changed 7 months ago by
Replying to gh-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.
comment:52 Changed 5 months ago by
- Milestone changed from sage-9.6 to sage-9.7
comment:53 Changed 5 months ago by
- Commit changed from 36ec4930832d840a73f1bcefe1d22bb27c566e49 to 52b5089bfc0c5793f7ad809578d1170826a5d861
Branch pushed to git repo; I updated commit sha1. New commits:
52b5089 | Merge remote-tracking branch 'origin/develop' into public/26254
|
comment:54 follow-up: ↓ 64 Changed 5 months ago by
- Cc mkoeppe added
After merging the latest dev branch, this now throws
File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/all_cmdline.py", line 19, in <module> from sage.all import * File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/all.py", line 135, in <module> from sage.symbolic.all import * File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/symbolic/all.py", line 8, in <module> import sage.symbolic.expression # initialize pynac before .ring File "sage/symbolic/expression.pyx", line 1, in init sage.symbolic.expression # -*- coding: utf-8 -*- File "sage/symbolic/function.pyx", line 144, in init sage.symbolic.function from .expression import ( ImportError: cannot import name call_registered_function
when importing sage.all
, see eg github build workflow. Any idea on how to fix it?
This method was introduced in #32386 (or one of its dependencies).
comment:55 Changed 4 months ago by
- Commit changed from 52b5089bfc0c5793f7ad809578d1170826a5d861 to 0517de6d6b8c614ecd863527e6106db27205eaf5
Branch pushed to git repo; I updated commit sha1. New commits:
0517de6 | Merge remote-tracking branch 'origin/develop' into public/26254
|
comment:56 Changed 4 months ago by
I tried to investigate this is a bit, but didn't get far.
With bindings=true, the expression.cpp
file contains the new lines
... static PyMethodDef __pyx_mdef_4sage_8symbolic_10expression_121call_registered_function = {"call_registered_function", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_121call_registered_function, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_120call_registered_function}; .... static PyMethodDef __pyx_mdef_4sage_8symbolic_10expression_123find_registered_function = {"find_registered_function", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_123find_registered_function, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_122find_registered_function}; ....
However, it no longer contains
static PyMethodDef __pyx_methods[] = { {"is_Expression", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_79is_Expression, METH_O, __pyx_doc_4sage_8symbolic_10expression_78is_Expression}, {"is_SymbolicEquation", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_81is_SymbolicEquation, METH_O, __pyx_doc_4sage_8symbolic_10expression_80is_SymbolicEquation}, {"_is_SymbolicVariable", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_83_is_SymbolicVariable, METH_O, __pyx_doc_4sage_8symbolic_10expression_82_is_SymbolicVariable}, {"_repr_Expression", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_91_repr_Expression, METH_O, __pyx_doc_4sage_8symbolic_10expression_90_repr_Expression}, {"_latex_Expression", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_93_latex_Expression, METH_O, __pyx_doc_4sage_8symbolic_10expression_92_latex_Expression}, {"new_Expression", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_99new_Expression, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_98new_Expression}, {"new_Expression_from_pyobject", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_101new_Expression_from_pyobject, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_100new_Expression_from_pyobject}, {"new_Expression_wild", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_103new_Expression_wild, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_102new_Expression_wild}, {"new_Expression_symbol", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_105new_Expression_symbol, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_104new_Expression_symbol}, {"print_order", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_107print_order, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_106print_order}, {"print_sorted", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_109print_sorted, METH_O, __pyx_doc_4sage_8symbolic_10expression_108print_sorted}, {"math_sorted", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_111math_sorted, METH_O, __pyx_doc_4sage_8symbolic_10expression_110math_sorted}, {"mixed_order", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_113mixed_order, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_112mixed_order}, {"mixed_sorted", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_115mixed_sorted, METH_O, __pyx_doc_4sage_8symbolic_10expression_114mixed_sorted}, {"call_registered_function", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_121call_registered_function, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_120call_registered_function}, {"find_registered_function", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_123find_registered_function, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_122find_registered_function}, {"register_or_update_function", (PyCFunction)(void*)(PyCFunctionWithKeywords)__pyx_pw_4sage_8symbolic_10expression_125register_or_update_function, METH_VARARGS|METH_KEYWORDS, __pyx_doc_4sage_8symbolic_10expression_124register_or_update_function}, {"get_sfunction_from_serial", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_127get_sfunction_from_serial, METH_O, __pyx_doc_4sage_8symbolic_10expression_126get_sfunction_from_serial}, {"get_sfunction_from_hash", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_129get_sfunction_from_hash, METH_O, __pyx_doc_4sage_8symbolic_10expression_128get_sfunction_from_hash}, {"make_map", (PyCFunction)__pyx_pw_4sage_8symbolic_10expression_131make_map, METH_O, __pyx_doc_4sage_8symbolic_10expression_130make_map}, {0, 0, 0, 0} };
which are present with bindings=false
.
Maybe its related to https://github.com/cython/cython/issues/1658.
After replacing expression.cpp
with the one from develop
everything seems to work fine.
Does anyone has an idea how to properly fix this? I tried setting bindings=false
for expression.pyx
, but then the compilation doesn't succeed anymore.
comment:57 follow-up: ↓ 71 Changed 4 months ago by
I took a few doctest runs as benchmarks, and could only find a very small negative impact (about 1%) if at all.
bindings=True
hyperfine -i --warmup 3 './sage -t --long src/sage/manifolds/differentiable/pseudo_riemannian.py' --show-output Time (mean ± σ): 13.183 s ± 0.266 s [User: 14.732 s, System: 1.173 s] Range (min … max): 12.937 s … 13.730 s 10 runs hyperfine -i --warmup 3 './sage -t --long src/sage/matrix/matrix0.pyx' --show-output Time (mean ± σ): 7.719 s ± 0.176 s [User: 7.039 s, System: 0.918 s] Range (min … max): 7.472 s … 7.993 s 10 runs
develop
hyperfine -i --warmup 3 './sage -t --long src/sage/manifolds/differentiable/pseudo_riemannian.py' --show-output Time (mean ± σ): 13.096 s ± 0.199 s [User: 14.618 s, System: 1.167 s] Range (min … max): 12.889 s … 13.524 s 10 runs hyperfine -i --warmup 3 './sage -t --long src/sage/matrix/matrix0.pyx' --show-output Time (mean ± σ): 7.675 s ± 0.165 s [User: 6.940 s, System: 0.970 s] Range (min … max): 7.364 s … 7.841 s 10 runs
(disclaimer: these were run on gitpod, so there is no guarantee that the same resources were available)
comment:58 Changed 3 months ago by
- Commit changed from 0517de6d6b8c614ecd863527e6106db27205eaf5 to 8d9bad02c5241d8ba5eea9f97e0b45ab0dc57d94
Branch pushed to git repo; I updated commit sha1. New commits:
8d9bad0 | Merge branch 'develop' into public/26254
|
comment:59 Changed 3 months ago by
- Commit changed from 8d9bad02c5241d8ba5eea9f97e0b45ab0dc57d94 to bea228b1437527a91478e539b1db9ab57e5b45fc
Branch pushed to git repo; I updated commit sha1. New commits:
bea228b | Try with localized import
|
comment:60 Changed 3 months ago by
- Commit changed from bea228b1437527a91478e539b1db9ab57e5b45fc to 9bbf574712ba9cb674a52a850fc6d10abfecac5b
comment:61 Changed 3 months ago by
- Commit changed from 9bbf574712ba9cb674a52a850fc6d10abfecac5b to 803804f82c0356ecf77094c636ce77a12c4b4589
Branch pushed to git repo; I updated commit sha1. New commits:
803804f | and one more test
|
comment:62 Changed 3 months ago by
This is now mostly working for me. A few doctests are still failing, mostly around cached functions and latex expressions. For some reason, some of the TypeErrors? are now IndexErrors? complaining that the index is out of bounds. Any idea where this is coming from?
Changed 3 months ago by
comment:63 Changed 3 months ago by
I tried to track down the source of the following errors:
File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/repl/display/pretty_print.py", line 144, in pretty ok = representation(obj, self, cycle) File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/repl/display/fancy_repr.py", line 348, in __call__ ascii_art_repr = ascii_art_repr or o.parent()._repr_option('element_ascii_art') IndexError: tuple index out of range
Debugging the following test script
import sage.all from sage.combinat.root_system.cartan_type import CartanType from sage.repl.rich_output import pretty_print C = CartanType(["A", 3, 1]) class MyCartanType: def my_method(self): return "I am here!" C._add_abstract_superclass(MyCartanType) pretty_print(C.__class__) # <class 'sage.combinat.root_system.type_A_affine.CartanType_with_superclass_with_superclass'> pretty_print(C.__class__.__bases__) # This triggers the index error, but should print # (<class 'sage.combinat.root_system.type_A_affine.CartanType_with_superclass'>, # <class ...__main__.MyCartanType...>)
leads to the following output in fancy_repr.py
, line 348:
For some reason calling parent
throws the IndexError?. The python debugger doesn't give more insights, and I have no experience with cython debugging to see where the indexerror is thrown. So it would be nice if someone with more cython experience can take this from here.
comment:64 in reply to: ↑ 54 ; follow-up: ↓ 65 Changed 3 months ago by
Replying to gh-tobiasdiez:
After merging the latest dev branch, this now throws
File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/all_cmdline.py", line 19, in <module> from sage.all import * File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/all.py", line 135, in <module> from sage.symbolic.all import * File "/__w/sagetrac-mirror/sagetrac-mirror/src/sage/symbolic/all.py", line 8, in <module> import sage.symbolic.expression # initialize pynac before .ring File "sage/symbolic/expression.pyx", line 1, in init sage.symbolic.expression # -*- coding: utf-8 -*- File "sage/symbolic/function.pyx", line 144, in init sage.symbolic.function from .expression import ( ImportError: cannot import name call_registered_functionwhen importing
sage.all
, see eg github build workflow. Any idea on how to fix it?This method was introduced in #32386 (or one of its dependencies).
It seems that cython fails to import the function call_registered_function
from sage.symbolic.expression
via the src/sage/symbolic/pynac_function_impl.pxi
file, where it is defined. I am not sure if we could regard this as a bug of cython as .pxi
files are not supposed to contain function definitions. I wonder why it is moved there in the first place. Anyway I think we should ask to cython community about this first before we refactor our code.
comment:65 in reply to: ↑ 64 ; follow-up: ↓ 68 Changed 3 months ago by
Replying to klee:
Anyway I think we should ask to cython community about this first before we refactor our code.
I filed an issue:
https://github.com/cython/cython/issues/4775
expecting any input from cython experts.
comment:66 follow-up: ↓ 69 Changed 3 months ago by
Thanks for opening the issue.
Do you have an idea where the IndexError? is coming from?
comment:67 Changed 3 months ago by
- Commit changed from 803804f82c0356ecf77094c636ce77a12c4b4589 to a23ffe9b9fd43e43ecacd73eaf6e21577b114a60
Branch pushed to git repo; I updated commit sha1. New commits:
a23ffe9 | Tiny edits
|
comment:68 in reply to: ↑ 65 Changed 3 months ago by
Replying to klee:
Replying to klee:
Anyway I think we should ask to cython community about this first before we refactor our code.
I filed an issue:
https://github.com/cython/cython/issues/4775
expecting any input from cython experts.
According to the answer, there was a circular import that somehow was hidden but revealed only with binding=True
. Hence I think changing global imports to local imports are the right solution.
comment:69 in reply to: ↑ 66 ; follow-ups: ↓ 70 ↓ 72 Changed 3 months ago by
Replying to gh-tobiasdiez:
Do you have an idea where the IndexError is coming from?
Note that with binding=False
:
class NewClass: def parent(self): return 1 NewClass.parent() ... TypeError: parent() missing 1 required positional argument: 'self'
%%cython cdef class NewClass: def parent(self): return 1 NewClass.parent() ... TypeError: unbound method NewClass.parent() needs an argument
With binding=True
:
%%cython cimport cython @cython.binding(True) cdef class NewClass: def parent(self): return 1 NewClass.parent() ... IndexError: tuple index out of range NewClass.parent(2) 1
In the latter, NewClass
is of course not an object but a class, hence NewClass.parent()
invokes unbound method .parent
which is a cython function that thinks it is bound. Hence it tries to get "self" from its dictionary of arguments. As the dictionary is empty, it raises IndexError
. I think this is a bug of cython. It should raise TypeError
as it should follow Python behavior.
Cython issue:
comment:70 in reply to: ↑ 69 Changed 3 months ago by
Replying to klee:
Cython issue:
They are fixing this bug. I think we should wait for the new Cython instead of "fixing it" on our side.
comment:71 in reply to: ↑ 57 Changed 3 months ago by
Replying to gh-tobiasdiez:
I took a few doctest runs as benchmarks, and could only find a very small negative impact (about 1%) if at all.
Now I think we should regard this 1% as our debt (it was not ours), or as something to pay to get proper introspection to cython functions, which weighs more than the 1%.
Perhaps we may selectively turn on "binding=False" for some cython functions critical for performance.
comment:72 in reply to: ↑ 69 Changed 3 months ago by
Replying to klee:
Replying to gh-tobiasdiez:
Do you have an idea where the IndexError is coming from?
Note that with
binding=False
:class NewClass: def parent(self): return 1 NewClass.parent() ... TypeError: parent() missing 1 required positional argument: 'self'%%cython cdef class NewClass: def parent(self): return 1 NewClass.parent() ... TypeError: unbound method NewClass.parent() needs an argumentWith
binding=True
:%%cython cimport cython @cython.binding(True) cdef class NewClass: def parent(self): return 1 NewClass.parent() ... IndexError: tuple index out of range NewClass.parent(2) 1In the latter,
NewClass
is of course not an object but a class, henceNewClass.parent()
invokes unbound method.parent
which is a cython function that thinks it is bound. Hence it tries to get "self" from its dictionary of arguments. As the dictionary is empty, it raisesIndexError
. I think this is a bug of cython. It should raiseTypeError
as it should follow Python behavior.Cython issue:
Nice! Thanks for doing the analysis and finding a nice minimal example reproducing the issue.
comment:73 Changed 3 months ago by
- Dependencies changed from #32509 to #32509, #33864
Should be fixed in the new cython release. Updating to it is tracked at #33864.
comment:74 Changed 3 months ago by
- Commit changed from a23ffe9b9fd43e43ecacd73eaf6e21577b114a60 to 570058c7004ac04286e409a5edda802bf2e20bb0
comment:75 Changed 3 months ago by
Seems like the update of cython to 0.29.30 didn't help, the index errors are still present (at least in the github workflow).
comment:76 Changed 4 weeks ago by
- Commit changed from 570058c7004ac04286e409a5edda802bf2e20bb0 to 02d62826b25d8ed817ef67ca0b77be983719a020
Branch pushed to git repo; I updated commit sha1. New commits:
02d6282 | Merge remote-tracking branch 'origin/develop' into public/26254
|
comment:77 Changed 14 hours ago by
- Commit changed from 02d62826b25d8ed817ef67ca0b77be983719a020 to f5bc80d0d33902c088753ee9592972b72e6619e7
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.