msg392623 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-01 19:49 |
When the C implementation of functools.lru_cache was added in bpo/issue14373, it appears to have omitted setting `.__defaults__` on its wrapped function.
```
Python 3.10.0a7+ (heads/master-dirty:823fbf4e0e, May 1 2021, 11:10:30) [Clang 12.0.0 (clang-1200.0.32.29)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import functools
>>> def func(b=5): pass
...
>>> @functools.lru_cache
... def cached_func(b=5): pass
...
>>> func.__defaults__
(5,)
>>> cached_func.__defaults__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'functools._lru_cache_wrapper' object has no attribute '__defaults__'
```
functools.update_wrapper() does set __defaults__ so this appears to just be an oversight in Modules/_functoolsmodule.c.
|
msg392627 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2021-05-01 20:12 |
Where does functools.update_wrapper() set __defaults__?
|
msg392643 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-01 23:52 |
That was anecdotal evidence:
```
Python 3.9.1 (v3.9.1:1e5d33e9b9, Dec 7 2020, 12:10:52)
[Clang 6.0 (clang-600.0.57)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> def func(arg=1, *, kwarg=2): pass
...
>>> import functools
>>> from functools import lru_cache, update_wrapper
>>> @lru_cache
... def cached_func(arg=1, *, kwargs=2): pass
...
>>> def x(*args, **kwargs): func(*args, **kwargs)
...
>>> updated_x = update_wrapper(func, x)
>>> x
<function x at 0x7feff5892b80>
>>> updated_x
<function x at 0x7feff5828a60>
>>> updated_x.__defaults__
(1,)
>>> updated_x.__kwdefaults__
{'kwarg': 2}
>>> func.__defaults__
(1,)
>>> func.__kwdefaults__
{'kwarg': 2}
>>> cached_func.__defaults__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'functools._lru_cache_wrapper' object has no attribute '__defaults__'
>>> cached_func.__kwdefaults__
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: 'functools._lru_cache_wrapper' object has no attribute '__kwdefaults__'
```
|
msg392644 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-01 23:57 |
the pure python functools.lru_cache doesn't get this right either. here's a desirable testcase for this bug:
```
def test_lru_defaults_bug44003(self):
@self.module.lru_cache(maxsize=None)
def func(arg='ARG', *, kw='KW'):
return arg, kw
self.assertEqual(func.__wrapped__.__defaults__, ('ARG',))
self.assertEqual(func.__wrapped__.__kwdefaults__, {'kw': 'KW'})
self.assertEqual(func.__defaults__, ('ARG',))
self.assertEqual(func.__kwdefaults__, {'kw': 'KW'})
```
results in
```
ERROR: test_lru_defaults_bug44003 (test.test_functools.TestLRUC)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/greg/oss/python/gpshead/Lib/test/test_functools.py", line 1738, in test_lru_defaults_bug44003
self.assertEqual(func.__defaults__, ('ARG',))
AttributeError: 'functools._lru_cache_wrapper' object has no attribute '__defaults__'
======================================================================
FAIL: test_lru_defaults_bug44003 (test.test_functools.TestLRUPy)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/Users/greg/oss/python/gpshead/Lib/test/test_functools.py", line 1738, in test_lru_defaults_bug44003
self.assertEqual(func.__defaults__, ('ARG',))
AssertionError: None != ('ARG',)
```
|
msg392645 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2021-05-02 00:17 |
I don't think this should be done. We want the lru_cache to be a pass-through. Applying defaults or keyword-only/positional-only restrictions is the responsibility of the inner function.
FWIW, here are the fields that Nick selected to be included in update_wrapper(): ('__module__', '__name__', '__qualname__', '__doc__', '__annotations__').
Those are sufficient to get help() to work which is all we were aiming for:
>>> from functools import *
>>> @lru_cache
def cached_func(b=5):
pass
>>> help(cached_func)
Help on _lru_cache_wrapper in module __main__:
cached_func(b=5)
|
msg392646 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-02 00:31 |
__defaults__ and __kwdefaults__ get used for code introspection. Just as __annotations__ does. __annotations__ is already available on the lru_cache wrapped function. All of those seem to go together from a runtime inspection point of view.
|
msg392647 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-02 00:34 |
An inner function can't know if somebody else might want to inspect it.
This is a decorator that does not change anything about the argument signature of the wrapped function, carrying over the reference to meta-information about that by default seems to make sense.
|
msg392649 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-02 01:05 |
https://bugs.python.org/issue41232 covers the more general case of suggesting changing update_wrapper's behavior. That would alleviate the need to fix the pure python implementation within my own PR.
|
msg392651 - (view) |
Author: Raymond Hettinger (rhettinger) * |
Date: 2021-05-02 01:29 |
I don't really like it. Carrying forward these attributes isn't the norm for wrapping functions.
The __defaults__ argument is normally only used where it has an effect rather than in a wrapper where it doesn't. Given that it is mutable, it invites a change that won't work. For example:
>>> def pow(base, exp=2):
return base ** exp
>>> pow.__defaults__
(2,)
>>> pow.__defaults__ = (3,)
>>> pow(2)
8
Also, an introspection function can only meaningfully use defaults when accompanied by the names of the fields:
>>> pow.__code__.co_varnames
('base', 'exp')
However, these aren't visible by directly introspecting the wrapper.
FWIW, we've never had a user reported issue regarding the absence of __defaults__. If ain't broke, let's don't "fix" it.
Nick and Serhiy, any thoughts?
|
msg392652 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-02 02:06 |
Oh, I didn't realize mutating those would actually change the code runtime behavior. But it makes sense, those are needed before the code object is entered.
Yeah that is different, and suggests making this the default is not actually desired. (this issue and the other one)
I guess our rule is that introspection code really must check for and be ready to handle .__wrapped__ if its goal is robustness?
|
msg392662 - (view) |
Author: Gregory P. Smith (gregory.p.smith) * |
Date: 2021-05-02 04:35 |
rejecting. code trying to make direct use of __defaults__ is likely better off using inspect.signature(). there might be an issue with inspect in some cases (https://bugs.python.org/issue41232) but I do not believe that is true for lru_cache wrapped things.
|
msg392668 - (view) |
Author: Serhiy Storchaka (serhiy.storchaka) * |
Date: 2021-05-02 06:04 |
I meant that I looked up the code of functools.update_wrapper() and did not see that it sets the __defaults__ attribute.
In your example in msg392643 you use functools.update_wrapper() incorrectly. The first argument is wrapper, and the second argument is wrapped. updated_x is func.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:59:45 | admin | set | github: 88169 |
2021-05-02 06:04:28 | serhiy.storchaka | set | messages:
+ msg392668 |
2021-05-02 04:35:18 | gregory.p.smith | set | status: open -> closed resolution: rejected messages:
+ msg392662
stage: patch review -> resolved |
2021-05-02 02:06:12 | gregory.p.smith | set | messages:
+ msg392652 |
2021-05-02 01:29:01 | rhettinger | set | messages:
+ msg392651 |
2021-05-02 01:05:55 | gregory.p.smith | set | type: behavior -> enhancement versions:
- Python 3.9, Python 3.10 |
2021-05-02 01:05:11 | gregory.p.smith | set | messages:
+ msg392649 |
2021-05-02 00:56:06 | rhettinger | set | nosy:
+ ncoghlan
|
2021-05-02 00:34:51 | gregory.p.smith | set | messages:
+ msg392647 versions:
+ Python 3.11 |
2021-05-02 00:31:59 | gregory.p.smith | set | messages:
+ msg392646 |
2021-05-02 00:24:27 | gregory.p.smith | set | keywords:
+ patch stage: needs patch -> patch review pull_requests:
+ pull_request24487 |
2021-05-02 00:17:47 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg392645
|
2021-05-01 23:57:27 | gregory.p.smith | set | messages:
+ msg392644 |
2021-05-01 23:52:06 | gregory.p.smith | set | messages:
+ msg392643 |
2021-05-01 20:12:28 | serhiy.storchaka | set | messages:
+ msg392627 |
2021-05-01 19:49:53 | gregory.p.smith | create | |