Replace sage_wraps by decorator library
tobiasdiez opened this issue · 47 comments
Replace the sage custom sage_wrap and decorator_defaults methods by the well-maintained decorator library https://github.com/micheles/decorator.
My motivation is to replace sage's autodoc sphinx extension with the built-in one. For this, the Phyton introspection code needs to work without sage's customization. This ticket is a first step towards this goal. But independently of this motivation the approach using decorator library already provides cleaner code and better introspection results with respect to annotations (e.g. typing information). See the following test script for a comparison:
import inspect
from pathlib import Path
from sage.misc.decorators import decorator_keywords, sage_wraps
from decorator import decorator, getfullargspec
from sage.misc.sageinspect import sage_getargspec
@decorator
def warn_slow(func, timelimit=60, *args, **kw):
print('%s took %d seconds', func.__name__, timelimit)
return func(*args, **kw)
@warn_slow
def preprocess_input_files(inputdir: Path, tempdir):
pass
@warn_slow(timelimit=600)
def run_calculation(tempdir, outdir):
pass
print("With decorator")
print(getfullargspec(preprocess_input_files))
# FullArgSpec(args=['inputdir', 'tempdir'], varargs=None, varkw=None, defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={'inputdir': <class 'pathlib.Path'>})
print(inspect.getfullargspec(preprocess_input_files))
# FullArgSpec(args=['inputdir', 'tempdir'], varargs=None, varkw=None, defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={'inputdir': <class 'pathlib.Path'>})
# !! Yields full information as expected
print(sage_getargspec(preprocess_input_files))
# ArgSpec(args=['inputdir', 'tempdir'], varargs=None, keywords=None, defaults=None)
# !! No annotation information
print(getfullargspec(run_calculation))
# FullArgSpec(args=['tempdir', 'outdir'], varargs=None, varkw=None, defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
print(inspect.getfullargspec(run_calculation))
# FullArgSpec(args=['tempdir', 'outdir'], varargs=None, varkw=None, defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
print(sage_getargspec(preprocess_input_files))
# ArgSpec(args=['inputdir', 'tempdir'], varargs=None, keywords=None, defaults=None)
@decorator_keywords
def warn_slow_sage(func=None, timelimit=60):
@sage_wraps(func)
def wrapper(*args, **kwargs):
print('%s took %d seconds', func.__name__, timelimit)
return func(*args, **kw)
return wrapper
@warn_slow_sage
def preprocess_input_files_sage(inputdir: Path, tempdir):
pass
@warn_slow_sage(timelimit=600)
def run_calculation_sage(tempdir, outdir):
pass
print("With sage")
print(getfullargspec(preprocess_input_files_sage))
# FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
print(inspect.getfullargspec(preprocess_input_files_sage))
# FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
# !! No args nor annotation
print(sage_getargspec(preprocess_input_files_sage))
# ArgSpec(args=['inputdir', 'tempdir'], varargs=None, keywords=None, defaults=None)
print(getfullargspec(run_calculation_sage))
# !! No annotation info
# FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
print(inspect.getfullargspec(run_calculation_sage))
# FullArgSpec(args=[], varargs='args', varkw='kwargs', defaults=None, kwonlyargs=[], kwonlydefaults=None, annotations={})
print(sage_getargspec(preprocess_input_files_sage))
# ArgSpec(args=['inputdir', 'tempdir'], varargs=None, keywords=None, defaults=None)
Replacing sage_getargspec with inspect.getfullargspec will be done in a follow-up ticket (as this is already big enough).
Todo (as follow-up): Migrate cachefunction etc to decorator framework. The documentation https://github.com/micheles/decorator/blob/master/docs/documentation.md#the-solution contains an example (memoize) of how this may look like.
Depends on #26254
Component: doctest framework
Work Issues: Fix patchbot warnings
Author: Tobias Diez
Branch/Commit: public/documentation/replaceDecorator @ 1b656a3
Issue created by migration from https://trac.sagemath.org/ticket/30884
Branch pushed to git repo; I updated commit sha1. New commits:
dbe456a | Remove unused decorator_defaults |
Branch pushed to git repo; I updated commit sha1. New commits:
0547957 | Fix stupid mistakes |
Functions such as sage_wraps cannot just be removed, as they may be used by user code. They need to go through deprecation
Good to know! I've readded and marked as deprecated them.
This should be now ready for review.
Changed work issues from Fix patchbot warnings to none
Better squash changes like this so that git blame is more accurate
Does the decorator library really misspell decorator as decorater?
Branch pushed to git repo; I updated commit sha1. New commits:
2c781d9 | Fix imports |
Branch pushed to git repo; I updated commit sha1. New commits:
3f04a50 | Fix spelling |
Work Issues: Fix patchbot warnings
Can you explain the purpose of the change
diff --git a/src/sage_setup/docbuild/__init__.py b/src/sage_setup/docbuild/__init__.py
index b07e9c1..a4ba68e 100644
--- a/src/sage_setup/docbuild/__init__.py
+++ b/src/sage_setup/docbuild/__init__.py
@@ -564,7 +564,7 @@ class ReferenceBuilder(AllBuilder):
if not os.path.exists(refdir):
continue
logger.info('Building bibliography')
- self._build_bibliography(lang, format, *args, **kwds)
+ #self._build_bibliography(lang, format, *args, **kwds)
logger.info('Bibliography finished, building dependent manuals')
self._build_everything_except_bibliography(lang, format, *args, **kwds)
# The html refman must be build at the end to ensure correct?
Branch pushed to git repo; I updated commit sha1. New commits:
f7c1b7e | Pass arguments along in cached decorators |
That's only a temporary fix (that I accidentally committed) since I don't have latex installed.
Branch pushed to git repo; I updated commit sha1. New commits:
9338637 | Pass by keywords |
Branch pushed to git repo; I updated commit sha1. New commits:
7cd66a6 | Expand keywrod argument... |
Branch pushed to git repo; I updated commit sha1. New commits:
a9a758c | Remove indication for keyword argurments |
Branch pushed to git repo; I updated commit sha1. New commits:
fc23e73 | Proper syntax for decorators with arguments |
What's the status of this ticket? Just came across a sage_getargspec bug (#31309), so it would be great if we can get rid of it
It is working in principle, but I still need to fix a few things here and there. The biggest problem is that there are some changes to cython files needed, for which it would be really handy if we could get #30371 merged.
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.
Changed work issues from Fix patchbot warnings to Wait for micheles/decorator#137 to be merged and released, update decorator lib, x patchbot warnings
Dependencies: #26254
Changed work issues from Wait for micheles/decorator#137 to be merged and released, update decorator lib, x patchbot warnings to Fix patchbot warnings
Branch pushed to git repo; I updated commit sha1. New commits:
5751b7d | Merge branch 'develop' of https://github.com/sagemath/sage into public/documentation/replaceDecorator |
ca86cc9 | build/pkgs/decorator: Update to 5.1.1 |
146bc0b | Revert changes to cache functions |
bff4f3a | Replace deprecated decorator by message |
e3d5bc0 | Fix deprecation wrapper |
89487a7 | Activate binding=true compiler directive |
Description changed:
---
+++
@@ -72,3 +72,6 @@
```
Replacing `sage_getargspec` with `inspect.getfullargspec` will be done in a follow-up ticket (as this is already big enough).
+
+
+Todo (as follow-up): Migrate cachefunction etc to decorator framework. The documentation https://github.com/micheles/decorator/blob/master/docs/documentation.md#the-solution contains an example (memoize) of how this may look like.