Remove monkey patching of inspect.isfunction in sage.__init__

[mkoeppe]

"Monkey-patch inspect.isfunction() to support Cython functions."

Introduced in 2018 (8.3.beta2) in #25373 (for #24994).

However, this change has been rejected by upstream (https://bugs.python.org/issue30071, https://www.python.org/dev/peps/pep-0575/, https://www.python.org/dev/peps/pep-0580/).

In this ticket:

• we remove the monkey-patching

• replace unnecessary uses of inspect in ordinary Sage library code by other solutions

• replace remaining uses of inspect.isfunction by the new name of our monkey-patched variant: sage.misc.sageinspect.is_function_or_cython_function

(To be checked whether https://www.python.org/dev/peps/pep-0590, superseding some of the above rejected PEPs, offers help; see also the informational PEP https://www.python.org/dev/peps/pep-0579/)

Related:

• Use Cython directive binding=True to get signatures for cython methods #26254 No signature shown in help for cythonized built-in methods
• Use sphinx.util.inspect.Signature not sphinx.ext.autodoc.inspector.formatargspec #27578 Use sphinx.util.inspect.Signature, not sphinx.ext.autodoc.inspector.formatargspec
• sage_getargspec mishandles keyword-only arguments #31309 sage_getargspec mishandles keyword-only arguments

Depends on #31420

CC: @fchapoton @kwankyu @jhpalmieri @orlitzky

Component: refactoring

Author: Matthias Koeppe

Branch/Commit: 2ba4c43

Reviewer: Michael Orlitzky

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

[mkoeppe]

Branch: u/mkoeppe/remove_monkey_patching_of_inspect_isfunction_in_sage___init__

[sagetrac-git]

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

903c6d4

src/sage/algebras/steenrod/steenrod_algebra_misc.py: Use callable(...) instead of inspect.isfunction(...)

[sagetrac-git]

Commit: 903c6d4

[sagetrac-git]

Changed commit from 903c6d4 to 1ac057d

[sagetrac-git]

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

1ac057d

sage.modules.{free_module_homspace,vector_space_homspace,vector_space_morphism}: Use callable(...) instead of inspect.isfunction(...)

[sagetrac-git]

Changed commit from 1ac057d to dcce32d

[sagetrac-git]

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

e123a92	sage.calculus.integration.monte_carlo_integral: Remove use of inspect.isfunction
dcce32d	sage.symbolic.expression.get_dynamic_class_for_function: Use callable(...) instead of inspect.isfunction(...)

[mkoeppe]

comment:8

Setting to "needs review" so that the patchbot runs

[mkoeppe]

Author: Matthias Koeppe, ...

[mkoeppe]

comment:9

sage -t --long --random-seed=0 src/sage/misc/sagedoc.py  # 8 doctests failed
sage -t --long --random-seed=0 src/sage/misc/rest_index_of_methods.py  # 1 doctest failed
sage -t --long --random-seed=0 src/sage/docs/conf.py  # 1 doctest failed

[sagetrac-git]

Changed commit from dcce32d to 3bc20b8

[sagetrac-git]

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

3bc20b8

sage.misc.sageinspect.is_function_or_cython_function: New, use it instead of relying on monkey patching of inspect.isfunction

[sagetrac-git]

Changed commit from 3bc20b8 to b5dd567

[sagetrac-git]

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

b5dd567

src/sage/misc/sageinspect.py: Fixup doctest

[mkoeppe]

comment:14

There are a few uses of inspect.isfunction in sage_docbuild. I don't know if these should be replaced by sage.misc.sageinspect.is_function_or_cython_function

src/sage_docbuild/ext/sage_autodoc.py:        return (inspect.isfunction(member) or inspect.isbuiltin(member)
src/sage_docbuild/ext/sage_autodoc.py:                not(inspect.ismethod(initmeth) or inspect.isfunction(initmeth)):
src/sage_docbuild/ext/sage_autodoc.py:        return inspect.isfunction(obj) or inspect.isbuiltin(obj) or inspect.ismethod(obj)

[mkoeppe]

comment:15

I would also like to get rid of these uses of inspect.isfunction in plotting code.

src/sage/plot/plot3d/plot3d.py:    if inspect.isfunction(func):
src/sage/plot/plot_field.py:    if isfunction(f):
src/sage/plot/streamline_plot.py:        if isfunction(f_g):

Help on this would be welcome

[sagetrac-git]

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

9675b69

src/sage_docbuild/ext/sage_autodoc.py: Use sage.misc.sageinspect.is_function_or_cython_function instead of inspect.isfunction

[orlitzky]

comment:20

What's an example of a cython function that isn't callable()?

[orlitzky]

comment:21

Replying to @orlitzky:

What's an example of a cython function that isn't callable()?

In other words, what goes wrong if you change the plot/sagedoc code to use callable() too? The only difference I see is that methods are callable() but the new is_function_or_cython_function() will return False on them. I don't see a moral problem with plotting a class method, though. And the sagedoc code is also checking for methods.

[mkoeppe]

comment:22

Note that

sage: type(x)                                                                                                                                                 
<class 'sage.symbolic.expression.Expression'>
sage: callable(x)                                                                                                                                             
True
sage: import inspect                                                                                                                                          
sage: inspect.isfunction(x)                                                                                                                                   
False
sage: from sage.misc.sageinspect import is_function_or_cython_function                                                                                        
sage: is_function_or_cython_function(x)                                                                                                                       
False

... so callable would not be the right replacement for the uses in sage.plot.

Regarding sagedoc I really don't have an idea about the code (in particular I look at the formatted documentation too rarely to be sure that changes don't break anything). Hence this safe replacement.

[mkoeppe]

comment:23

(All Cython functions are callable but not all objects that are callable satisfy is_function_or_cython_function.)

[orlitzky]

comment:24

Ok, let's just merge the easy part then (I don't really want to look at plots either).

This bit in integration.pyx would be cleaner/faster with hasattr() I think:

try:
    vars = func.arguments()
except AttributeError:
    try:
        vars = func.variables()
    except AttributeError:
        vars = sage_getargspec(func)[0]

Mismatched parentheses in docstring:

"""
Think twice before using this function (or any function from the
``inspect`` or ``sage.misc.sageinspect`` modules.  Most uses of
``isfunction`` in ordinary library code can be replaced by ``callable``.
"""

You could also use Sphinx cross-module references there. Finally,

"""
Verify that ipywidgets can correctly determine signatures of Cython
functions::

    sage: from ipywidgets.widgets.interaction import signature
    sage: from sage.dynamics.complex_dynamics.mandel_julia_helper import fast_mandelbrot_plot
    sage: signature(fast_mandelbrot_plot)  # random
    <IPython.utils._signatures.Signature object at 0x7f3ec8274e10>
"""

I think that should go under a new TESTS:: block, since it's verifying some internal detail that a user reading the reference manual won't care about.

[mkoeppe]

comment:25

Replying to @orlitzky:

Ok, let's just merge the easy part then (I don't really want to look at plots either).

This bit in integration.pyx would be cleaner/faster with hasattr()

I haven't timed this particular one in Cython but at least in plain Python try: except: is faster than doing the attribute access a second time.

[sagetrac-git]

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

f9154a9

sage.misc.sageinspect.is_function_or_cython_function: Improve docstring markup

[sagetrac-git]

Changed commit from a03c459 to f9154a9

[orlitzky]

Reviewer: Michael Orlitzky

[orlitzky]

comment:27

Ok then.

[mkoeppe]

comment:28

Thanks for reviewing!

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

91ec0f2	src/sage/__init__.py: Remove __all__
949d787	src/sage/__init__.py: Remove __version__
f77a98e	src/sage/__init__.py: Move 'import zlib' to src/sage/cpython/__init__.py
2e9ea56	src/sage/__init__.py: Move monkey-patch of ExtensionFileLoader to src/sage/cpython/__init__.py
ad587cf	src/sage/__init__.py [Cygwin]: Move monkey-patch of sqlite to src/sage/cpython/__init__.py
af6642f	src/sage/cpython/__init__.py: Clean up sage.cpython module
2ba4c43	Merge #31420

[sagetrac-git]

Changed commit from f9154a9 to 2ba4c43

[mkoeppe]

comment:30

Merged #31420 to remove the trivial merge conflict in src/sage/__init__.py.

[mkoeppe]

Dependencies: #31420

[vbraun]

Changed branch from u/mkoeppe/remove_monkey_patching_of_inspect_isfunction_in_sage___init__ to 2ba4c43